Addresses review feedback from mozilla central. See bugzilla bug 752676.

diff --git a/extensions/firefox/components/PdfStreamConverter.js b/extensions/firefox/components/PdfStreamConverter.js
index dd8f2c190c516b765e872e7545be41461982f6ee..9e5dcbe61a1980cbbc8c1bafd27eed054b4b7cbf 100644 (file)
--- a/extensions/firefox/components/PdfStreamConverter.js
+++ b/extensions/firefox/components/PdfStreamConverter.js
@@ -9,6 +9,7 @@ const Cc = Components.classes;
 const Ci = Components.interfaces;
 const Cr = Components.results;
 const Cu = Components.utils;
+// True only if this is the version of pdf.js that is included with firefox.
 const MOZ_CENTRAL = PDFJSSCRIPT_MOZ_CENTRAL;
 const PDFJS_EVENT_ID = 'pdf.js.message';
 const PDF_CONTENT_TYPE = 'application/pdf';
@@ -21,11 +22,14 @@ Cu.import('resource://gre/modules/XPCOMUtils.jsm');
 Cu.import('resource://gre/modules/Services.jsm');
 Cu.import('resource://gre/modules/NetUtil.jsm');
 
+
 let appInfo = Cc['@mozilla.org/xre/app-info;1']
                   .getService(Ci.nsIXULAppInfo);
 let privateBrowsing, inPrivateBrowsing;
-let mimeService = Cc['@mozilla.org/mime;1']
-                    .getService(Ci.nsIMIMEService);
+let Svc = {};
+XPCOMUtils.defineLazyServiceGetter(Svc, 'mime',
+                                   '@mozilla.org/mime;1',
+                                   'nsIMIMEService');
 
 if (appInfo.ID === FIREFOX_ID) {
   privateBrowsing = Cc['@mozilla.org/privatebrowsing;1']
@@ -75,15 +79,15 @@ function getDOMWindow(aChannel) {
 
 function isEnabled() {
   if (MOZ_CENTRAL) {
-    var enabled = getBoolPref(PREF_PREFIX + '.enabled', false);
-    if (!enabled)
+    var disabled = getBoolPref(PREF_PREFIX + '.disabled', false);
+    if (disabled)
       return false;
     // To also be considered enabled the "Preview in Firefox" option must be
     // selected in the Application preferences.
-    var handlerInfo = mimeService.
-                        getFromTypeAndExtension('application/pdf', 'pdf');
-    return handlerInfo && (handlerInfo.alwaysAskBeforeHandling == false &&
-           handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally);
+    var handlerInfo = Svc.mime
+                         .getFromTypeAndExtension('application/pdf', 'pdf');
+    return handlerInfo.alwaysAskBeforeHandling == false &&
+           handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally;
   }
   // Always returns true for the extension since enabling/disabling is handled
   // by the add-on manager.
@@ -111,9 +115,10 @@ function getLocalizedStrings(path) {
   }
   return map;
 }
-function getLocalizedString(strings, id) {
+function getLocalizedString(strings, id, property) {
+  property = property || 'textContent';
   if (id in strings)
-    return strings[id]['textContent'];
+    return strings[id][property];
   return id;
 }
 
@@ -124,9 +129,8 @@ function ChromeActions(domWindow) {
 
 ChromeActions.prototype = {
   download: function(data) {
-    let mimeService = Cc['@mozilla.org/mime;1'].getService(Ci.nsIMIMEService);
-    var handlerInfo = mimeService.
-                        getFromTypeAndExtension('application/pdf', 'pdf');
+    var handlerInfo = Svc.mime
+                         .getFromTypeAndExtension('application/pdf', 'pdf');
     var uri = NetUtil.newURI(data);
 
     var extHelperAppSvc =
@@ -200,10 +204,10 @@ ChromeActions.prototype = {
     var win = Services.wm.getMostRecentWindow('navigator:browser');
     var browser = win.gBrowser.getBrowserForDocument(domWindow.top.document);
     var notificationBox = win.gBrowser.getNotificationBox(browser);
-
     var buttons = [{
       label: getLocalizedString(strings, 'open_with_different_viewer'),
-      accessKey: null,
+      accessKey: getLocalizedString(strings, 'open_with_different_viewer',
+                                    'accessKey'),
       callback: function() {
         self.download(url);
       }

diff --git a/extensions/firefox/content/PdfJs.jsm b/extensions/firefox/content/PdfJs.jsm
index cd208c7fe3c93dd9343eaab609041238941e800b..9d112f5b2b856259f84ec1a733b7ab1e45e74eb9 100644 (file)
--- a/extensions/firefox/content/PdfJs.jsm
+++ b/extensions/firefox/content/PdfJs.jsm
@@ -7,72 +7,82 @@ const Cm = Components.manager;
 const Cu = Components.utils;
 
 const PREF_PREFIX = 'pdfjs';
-const PREF_ENABLED = PREF_PREFIX + '.enabled';
+const PREF_DISABLED = PREF_PREFIX + '.disabled';
 const PREF_FIRST_RUN = PREF_PREFIX + '.firstRun';
-const PREF_PREVIOUS_ACTION = PREF_PREFIX + '.previousAction';
-const PREF_PREVIOUS_ASK = PREF_PREFIX + '.previousAsk';
-const PDFJS_HANDLER_CHANGED = 'pdfjs:handlerChanged';
+const PREF_PREVIOUS_ACTION = PREF_PREFIX + '.previousHandler.preferredAction';
+const PREF_PREVIOUS_ASK = PREF_PREFIX + '.previousHandler.alwaysAskBeforeHandling';
+const TOPIC_PDFJS_HANDLER_CHANGED = 'pdfjs:handlerChanged';
 
+Cu.import('resource://gre/modules/XPCOMUtils.jsm');
 Cu.import('resource://gre/modules/Services.jsm');
 Cu.import('resource://pdf.js.components/PdfStreamConverter.js');
 
-let mimeService = Cc["@mozilla.org/mime;1"]
-                    .getService(Ci.nsIMIMEService);
+let Svc = {};
+XPCOMUtils.defineLazyServiceGetter(Svc, 'mime',
+                                   '@mozilla.org/mime;1',
+                                   'nsIMIMEService');
 
-function getBoolPref(pref, def) {
+function getBoolPref(aPref, aDefaultValue) {
   try {
-    return Services.prefs.getBoolPref(pref);
+    return Services.prefs.getBoolPref(aPref);
   } catch (ex) {
-    return def;
+    return aDefaultValue;
   }
 }
 
-// Register/unregister a class as a component.
+// Register/unregister a constructor as a component.
 let Factory = {
-  registrar: null,
-  aClass: null,
-  register: function(aClass) {
-    if (this.aClass) {
-      return;
-    }
-    this.registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);
-    this.aClass = aClass;
-    var proto = aClass.prototype;
-    this.registrar.registerFactory(proto.classID, proto.classDescription,
-      proto.contractID, this);
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFactory]),
+  _targetConstructor: null,
+
+  register: function register(targetConstructor) {
+    this._targetConstructor = targetConstructor;
+    var proto = targetConstructor.prototype;
+    var registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);
+    registrar.registerFactory(proto.classID, proto.classDescription,
+                              proto.contractID, this);
   },
-  unregister: function() {
-    if (!this.aClass) {
-      return;
-    }
-    var proto = this.aClass.prototype;
-    this.registrar.unregisterFactory(proto.classID, this);
-    this.aClass = null;
+
+  unregister: function unregister() {
+    var proto = this._targetConstructor.prototype;
+    var registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);
+    registrar.unregisterFactory(proto.classID, this);
+    this._targetConstructor = null;
   },
-  // nsIFactory::createInstance
-  createInstance: function(outer, iid) {
-    if (outer !== null)
+
+  // nsIFactory
+  createInstance: function createInstance(aOuter, iid) {
+    if (aOuter !== null)
       throw Cr.NS_ERROR_NO_AGGREGATION;
-    return (new (this.aClass)).QueryInterface(iid);
+    return (new (this._targetConstructor)).QueryInterface(iid);
+  },
+
+  // nsIFactory
+  lockFactory: function lockFactory(lock) { 
+    // No longer used as of gecko 1.7.
+    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
   }
 };
 
 let PdfJs = {
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
   _registered: false,
-  init: function() {
+
+  init: function init() {
     // On first run make pdf.js the default handler.
-    if (getBoolPref(PREF_ENABLED, false) && getBoolPref(PREF_FIRST_RUN, false)) {
+    if (!getBoolPref(PREF_DISABLED, true) && getBoolPref(PREF_FIRST_RUN, false)) {
       Services.prefs.setBoolPref(PREF_FIRST_RUN, false);
 
-      let handlerService = Cc['@mozilla.org/uriloader/handler-service;1'].
-                            getService(Ci.nsIHandlerService);
-      let handlerInfo = mimeService.getFromTypeAndExtension('application/pdf', 'pdf');
-
+      let handlerInfo = Svc.mime.getFromTypeAndExtension('application/pdf', 'pdf');
       // Store the previous settings of preferredAction and
-      // alwaysAskBeforeHandling in case we need to fall back to it.
+      // alwaysAskBeforeHandling in case we need to revert them in a hotfix that
+      // would turn pdf.js off.
       Services.prefs.setIntPref(PREF_PREVIOUS_ACTION, handlerInfo.preferredAction);
       Services.prefs.setBoolPref(PREF_PREVIOUS_ASK, handlerInfo.alwaysAskBeforeHandling);
 
+      let handlerService = Cc['@mozilla.org/uriloader/handler-service;1'].
+                           getService(Ci.nsIHandlerService);
+
       // Change and save mime handler settings.
       handlerInfo.alwaysAskBeforeHandling = false;
       handlerInfo.preferredAction = Ci.nsIHandlerInfo.handleInternally;
@@ -80,42 +90,49 @@ let PdfJs = {
     }
 
     if (this.enabled)
-      this._register();
+      this._ensureRegistered();
     else
-      this._unregister();
+      this._ensureUnregistered();
 
     // Listen for when pdf.js is completely disabled or a different pdf handler
     // is chosen.
-    Services.prefs.addObserver(PREF_ENABLED, this, false);
-    Services.obs.addObserver(this, PDFJS_HANDLER_CHANGED, false);
+    Services.prefs.addObserver(PREF_DISABLED, this, false);
+    Services.obs.addObserver(this, TOPIC_PDFJS_HANDLER_CHANGED, false);
   },
-  observe: function(subject, topic, data) {
-    if (topic != 'nsPref:changed' && topic != PDFJS_HANDLER_CHANGED)
-      return;
 
+  // nsIObserver
+  observe: function observe(aSubject, aTopic, aData) {
     if (this.enabled)
-      this._register();
+      this._ensureRegistered();
     else
-      this._unregister();
+      this._ensureUnregistered();
   },
-  // pdf.js is only enabled if we're both selected as the pdf viewer and if the 
-  // global switch enabling it is true.
+  
+  /**
+   * pdf.js is only enabled if it is both selected as the pdf viewer and if the 
+   * global switch enabling it is true.
+   * @return {boolean} Wether or not it's enabled.
+   */
   get enabled() {
-    var handlerInfo = mimeService.
-                        getFromTypeAndExtension('application/pdf', 'pdf');
+    var disabled = getBoolPref(PREF_DISABLED, true);
+    if (disabled)
+      return false;
 
-    var selectedAsHandler = handlerInfo && (handlerInfo.alwaysAskBeforeHandling == false &&
-           handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally);
-    return getBoolPref(PREF_ENABLED, false) && selectedAsHandler;
+    var handlerInfo = Svc.mime.
+                        getFromTypeAndExtension('application/pdf', 'pdf');
+    return handlerInfo.alwaysAskBeforeHandling == false &&
+           handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally;
   },
-  _register: function() {
+
+  _ensureRegistered: function _ensureRegistered() {
     if (this._registered)
       return;
 
     Factory.register(PdfStreamConverter);
     this._registered = true;
   },
-  _unregister: function() {
+
+  _ensureUnregistered: function _ensureUnregistered() {
     if (!this._registered)
       return;
 

diff --git a/test/mozcentral/browser_pdfjs_main.js b/test/mozcentral/browser_pdfjs_main.js
index f4bb26effa3af212ca54ce19d0ad60e55763dd42..e3d7c8babca12f9dd060079319de1ea95f6f90f4 100644 (file)
--- a/test/mozcentral/browser_pdfjs_main.js
+++ b/test/mozcentral/browser_pdfjs_main.js
@@ -1,6 +1,5 @@
 /* Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/publicdomain/zero/1.0/
- */
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 const RELATIVE_DIR = "browser/extensions/pdfjs/test/";
 const TESTROOT = "http://example.com/browser/" + RELATIVE_DIR;
@@ -8,8 +7,6 @@ const TESTROOT = "http://example.com/browser/" + RELATIVE_DIR;
 function test() {
   var tab;
 
-  const Cc = Components.classes;
-  const Ci = Components.interfaces;
   let handlerService = Cc["@mozilla.org/uriloader/handler-service;1"].getService(Ci.nsIHandlerService);
   let mimeService = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService);
   let handlerInfo = mimeService.getFromTypeAndExtension('application/pdf', 'pdf');
@@ -35,9 +32,7 @@ function test() {
 
     // Runs tests after all 'load' event handlers have fired off
     setTimeout(function() {
-      runTests(document, window, function() {
-        finish();
-      });
+      runTests(document, window, finish);
     }, 0);
   }, true);
 }

diff --git a/test/mozcentral/browser_pdfjs_savedialog.js b/test/mozcentral/browser_pdfjs_savedialog.js
index 3a7148ed51791a58a867c8a230be95cc7740819b..a6564e59199b3cd230fd55c3f93aa5e64a2ad1b4 100644 (file)
--- a/test/mozcentral/browser_pdfjs_savedialog.js
+++ b/test/mozcentral/browser_pdfjs_savedialog.js
@@ -1,17 +1,12 @@
 /* Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/publicdomain/zero/1.0/
- */
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 const RELATIVE_DIR = "browser/extensions/pdfjs/test/";
 const TESTROOT = "http://example.com/browser/" + RELATIVE_DIR;
 
 function test() {
-  const Cc = Components.classes;
-  const Ci = Components.interfaces;
-  var tab;
-
   var oldAction = changeMimeHandler(Ci.nsIHandlerInfo.useSystemDefault, true);
-
+  var tab = gBrowser.addTab(TESTROOT + "file_pdfjs_test.pdf");
   //
   // Test: "Open with" dialog comes up when pdf.js is not selected as the default
   // handler.
@@ -23,14 +18,9 @@ function test() {
     changeMimeHandler(oldAction[0], oldAction[1]);
     gBrowser.removeTab(tab);
   });
-
-  tab = gBrowser.addTab(TESTROOT + "file_pdfjs_test.pdf");
-  var newTabBrowser = gBrowser.getBrowserForTab(tab);
 }
 
 function changeMimeHandler(preferredAction, alwaysAskBeforeHandling) {
-  const Cc = Components.classes;
-  const Ci = Components.interfaces;
   let handlerService = Cc["@mozilla.org/uriloader/handler-service;1"].getService(Ci.nsIHandlerService);
   let mimeService = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService);
   let handlerInfo = mimeService.getFromTypeAndExtension('application/pdf', 'pdf');
@@ -44,7 +34,6 @@ function changeMimeHandler(preferredAction, alwaysAskBeforeHandling) {
   Services.obs.notifyObservers(null, 'pdfjs:handlerChanged', null);
 
   // Refresh data
-  mimeService = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService);
   handlerInfo = mimeService.getFromTypeAndExtension('application/pdf', 'pdf');
 
   //
@@ -57,8 +46,6 @@ function changeMimeHandler(preferredAction, alwaysAskBeforeHandling) {
 }
 
 function addWindowListener(aURL, aCallback) {
-  const Cc = Components.classes;
-  const Ci = Components.interfaces;
   Services.wm.addListener({
     onOpenWindow: function(aXULWindow) {
       info("window opened, waiting for focus");