mozilla/pdf.js — Issue #1165: AMO Review Comments Round 2
Author: brendandahl
Created: 2012-02-03 02:52:48+00:00
State: open

Reviewer:
Nils Maier

Comments:
Your preliminary review request has been approved.

First of all: awesome work

Some is issues/suggestions in no particular order:

- Bootstrapped add-ons are require to fully clean up after themselves when disabled/uninstalled in order to receive a full review.
Your add-on does not unload the registered stuff (via manifest), however.

- Does not work for (all?) ftp, e.g:
ftp://ftp.mpi-sb.mpg.de/pub/tex/mirror/ftp.dante.de/pub/tex/macros/latex/contrib/currvita/currvita.pdf

- The message passing between the component and the content windows looks kinda icky, with creating a textNode and all that. Maybe the regular postMessage API would be more appropriate?
https://developer.mozilla.org/en/DOM/window.postMessage

- The message passing initialization code incorrectly assumes the load will happen in the foreground window:
Consider trying to get the DOMWindow via different means, e.g. through nsIWebProgress:
Simplified example:
let wp = null;
if (channel.loadGroup && channel.loadGroup.groupObserver) {
wp = channel.loadGroup.groupObserver.QueryInterface(Ci.nsIWebProgress);
}
if (!wp) {
wp = channel.notificationCallbacks.getInterface(Ci.nsIWebProgress);
}
wp.DOMWindow

- Use Services.jsm stuff wherever possible. E.g. there is still a getService(Ci.nsIIOService) call.

- Automatic chrome registration for bootstrapped add-ons is coming and is actually already partly there:
https://developer.mozilla.org/en/Extensions/Bootstrapped_extensions#Adding_user_interface_with_a_chrome.manifest
Make sure that your custom registration code does not interfere with it (in the future)

- The setDatabase() function is potentially vulnerable to DOS attacks by trying to write huge strings to the corresponding pref. You should add some safe-guard against that.

Thank you.