752676 - Control pdf.js and Other PDF Plugins using Application Preferences

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect)

Description

[Brendan Dahl [:bdahl]]

The extension will no longer show up in the add-ons UI after bug 738674, but we still need a way to disable it.  The plan is to use Preferences->Applications and add a "Preview in Firefox" option for the pdf content type.

Comment 1

[Brendan Dahl [:bdahl]]

Attachment: preview pdfs in ff

Comment 2

[Brendan Dahl [:bdahl]]

This still needs more work after the add-on blocker issues are taken care of.  
Things this adds:

1) In preferences->Applications under Portable Document Format (PDF) there is now an option for "Preview in Firefox" (like rss feeds). This code should also make it easy for us to do this for more content types in the future(e.g. shumway).

2) A new pref to make it so pdf.js can be disabled completely. This will be the flag we can use so we don't have to back pdf.js completely out of aurora.  Disabling is currently handled in the boostrap.js startup code, but we may want to move this after the add-on blocker code so pdf.js doesn't get installed at all.

Some more thoughts:
I'm not sure yet how we want to handle when the user disables/enables pdf.js in preferences->applications.  We could either: 
1) disable the add-on
2) make it disabled by setting the preferred action to something that is not nsIHandlerInfo.handleInternally (this is currently how it works, but the above may be preferred)

One fix that could have farther reaching implications is the typo I fixed in uriloader/exthandler/nsHandlerService.js

Comment 3

[Brendan Dahl [:bdahl]]

Attachment: preview pdfs in ff

remove dumps

Comment 4

[Brendan Dahl [:bdahl]]

Attachment: preview pdfs in ff

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 622609 [details] [diff] [review]
preview pdfs in ff

Brendan says this isn't yet ready for review.

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> Comment on attachment 622609 [details] [diff] [review]
> preview pdfs in ff
> 
> Brendan says this isn't yet ready for review.

I'm sorry. I misread.

Comment 8

[Lars Viklund]

Bah... the constant duping of bugs that are not dupes. My bug 759171 is about how pdf.js suddenly becomes a default, which went completely unaddressed and will be lost in behind the duplicate "resolve".

Yet again, is it intended that this completely unwanted component steals defaults without notice and is utterly impossible to find where to disable/destroy/obliterate?

Comment 11

[u123541]

Not only does it steal default, as my "duped" bug 759353 indicates:  PDF viewer which has no scroll bars.  There is no way to view/print[1] the bottom of pages -- resizing gives unacceptable roach-sized text. When a document arrives, the viewer sometimes, very briefly flashes a red bar with "Error" (not sure of actual text as it's too fast).

I agree with Lars: "is it intended that this completely unwanted component steals defaults without notice and is utterly impossible to find where to disable/destroy/obliterate?"  Notwithstanding the above issues, my main point too, was the forcible imposition of this "default" which is going to be a huge target if not addressed from a user perspective.

Normally, I use Adobe Reader, okular and xpdf depending on what each provides.  Not having a choice but to use something without scroll bars, or being able to reliably print via this new default is a show stopper.

[1] Trying to print these docs produces only what will print on the first sheet -- if a page spills over to a second sheet (portrait on landscape, or legal on letter), the second sheet is totally blank.

Comment 12

[Jim Jeffery not reading bug-mail 1/2/11]

Shouldn't this be a blocker for next weeks uplift?  With PDF.js enabled, those upgrading to Aurora are not going to be able to turn off this feature or control what PDF viewer they want.

I would think this needs to be priority before the uplift to get in some quick Trunk/Nighly testing.

Comment 13

[Brendan Dahl [:bdahl]]

Attachment: pdf.js changes (most unrelated to this)

Comment 14

[Brendan Dahl [:bdahl]]

Attachment: firefox changes

Comment 15

[Brendan Dahl [:bdahl]]

I've broken the patch into two parts to hopefully separate out what is relevant to this patch and what is coming from upstream pdf.js.

Updating my previous comment to reflect the current state:

1) In Preferences->Applications, Content Type=Portable Document Format (PDF) there is now an option for "Preview in Firefox" (like rss feeds). To support this I've added a new InternalHandlerWrapper object in application.js which should make it easy to add more optional content type handlers in the future(e.g. shumway).

2) A new pref to make it so pdf.js can be disabled completely. PdfJs.jsm listens for changes to this pref and registers/un-registers our stream converter.  Also, the option for "Preview in Firefox" isn't shown if pdf.js is disabled.

3) One fix that could have farther reaching implications is the typo I fixed in uriloader/exthandler/nsHandlerService.js that has been there for a long time.

Comment 16

[Brendan Dahl [:bdahl]]

Two more things:
There are new mochi tests that Artur wrote for this.

Try run going at:
https://tbpl.mozilla.org/?tree=Try&rev=814a7878097e

Comment 17

[Brendan Dahl [:bdahl]]

Attachment: firefox changes

I've added it so pdf.js becomes the default pdf handler and updated the mochi tests to reflect this.  I've also made it so it stores the previous values for handlerInfo so we can revert back to the users previous state if we need to.

Comment 18

[Brendan Dahl [:bdahl]]

New try run:
https://tbpl.mozilla.org/?tree=Try&rev=ac0c16d315d4

Comment 19

[Masatoshi Kimura [:emk]]

You need to set a reviewer.

Comment 20

[Brendan Dahl [:bdahl]]

Attachment: firefox changes

Comment 21

[Brendan Dahl [:bdahl]]

Attachment: firefox changes

Comment 22

[Brendan Dahl [:bdahl]]

The chrome.manifest file in the first patch should probably be in the second patch as it impacts FF.  See https://bugzilla.mozilla.org/attachment.cgi?id=628876&action=diff#a/browser/extensions/pdfjs/chrome.manifest_sec1

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 629232 [details] [diff] [review]
firefox changes

Review of attachment 629232 [details] [diff] [review]:
-----------------------------------------------------------------

globally looks fine, though I think you should also update the in-content preferences version of applications.js, see below.

::: browser/app/profile/firefox.js
@@ +1158,5 @@
>  pref("toolkit.startup.max_resumed_crashes", 3);
>  
> +// Allow pdf.js to preview pdfs within firefox. Note: if this is true it does
> +// not necessarily mean pdf.js is the pdf handler, just that it is an option.
> +pref("pdfjs.enabled", true);

the note there makes me think the name of the pref is just misleading. I think this is one of the rare cases where .disabled makes more sense. pdfjs.disabled would make clear it won't/can't be used or selected when true.

@@ +1165,5 @@
> +pref("pdfjs.firstRun", true);
> +// The values of preferredAction and alwaysAskBeforeHandling before pdf.js
> +// became the default.
> +pref("pdfjs.previousAction", 0);
> +pref("pdfjs.previousAsk", false);

it's clearer if you keep the name of the original value here, like
pdfjs.previousHandler.preferredAction
pdfjs.previousHandler.alwaysAskBeforeHandling

::: browser/components/preferences/applications.js
@@ +9,1 @@
>  // Constants & Enumeration Values

you are changing application.js in the preferences dialog, though now we also have browser/components/preferences/in-content/applications.js for in-content preferences ui, they should probably be kept in sync for now otherwide that ui would do something different from what we expect.

@@ +13,5 @@
>  */
>  var Cc = Components.classes;
>  var Ci = Components.interfaces;
>  var Cr = Components.results;
> +var Cu = Components.utils;

looks like it's only used in the import... no point in adding it, just use Components.utils there

@@ +25,5 @@
>  const TYPE_MAYBE_AUDIO_FEED = "application/vnd.mozilla.maybe.audio.feed";
> +const TYPE_PDF = "application/pdf";
> +
> +const PREF_PDFJS_ENABLED = "pdfjs.enabled";
> +const PDFJS_HANDLER_CHANGED = "pdfjs:handlerChanged";

TOPIC_PDFJS_HANDLER_CHANGED

@@ +822,5 @@
> + * menu.
> + */
> +function InternalHandlerInfoWrapper(aMIMEType) {
> +  var handlerInfo = this._mimeSvc.getFromTypeAndExtension(aMIMEType, null);
> +  handlerInfo.QueryInterface(Ci.nsIHandlerInfo);

why is this needed, doesn't getFromTypeAndExtension already return an object QI to nsIHandlerInfo?

@@ +829,5 @@
> +}
> +
> +InternalHandlerInfoWrapper.prototype = {
> +  __proto__: HandlerInfoWrapper.prototype,
> +  _mimeSvc: Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService),

only used in the constructor, doesn't make sense to cache it

@@ +839,5 @@
> +    Services.obs.notifyObservers(null, this._handlerChanged, null);
> +  },
> +
> +  get enabled() {
> +    return this._prefSvc.getBoolPref(this._prefEnabled);

may use Services.prefs at this point, likely the original wrapper will do one one, as well.

this looks really related to the specific pdf.js implementation, the enabled function should be provided in the info object imo, it may be anything, not just a simple bool pref...

@@ +1065,5 @@
> +   */
> +  _loadInternalHandlers: function() {
> +    var internalHandlers = [pdfHandlerInfo];
> +    for (var i = 0, ii = internalHandlers.length; i < ii; i++) {
> +      var internalHandler = internalHandlers[i];

for (let handler of internalHandlers) {

@@ +1280,5 @@
>                                                        [this._brandShortName]);
>  
> +        if (aHandlerInfo instanceof InternalHandlerInfoWrapper)
> +          return this._prefsBundle.getFormattedString("previewInApp",
> +                                                      [this._brandShortName]);

please brace these multiline ifs

@@ +1386,5 @@
> +    if (handlerInfo instanceof InternalHandlerInfoWrapper) {
> +      var internalMenuItem = document.createElement("menuitem");
> +      internalMenuItem.setAttribute("action", Ci.nsIHandlerInfo.handleInternally);
> +      let label = this._prefsBundle.getFormattedString("previewInApp",
> +                                                     [this._brandShortName]);

bogus indentation

::: browser/extensions/pdfjs/components/PdfStreamConverter.js
@@ +8,5 @@
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cr = Components.results;
>  const Cu = Components.utils;
> +const MOZ_CENTRAL = true;

please document what this is about and how it's supposed to be used, looks strange and a bit too much specific... maybe it's just bad named if it's supposed to distinguish the extension from the built-in code... IS_EXTENSION = false; ?

@@ +24,5 @@
>  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);

defineLazyServiceGetter

@@ +82,5 @@
> +    // selected in the Application preferences.
> +    var handlerInfo = mimeService.
> +                        getFromTypeAndExtension('application/pdf', 'pdf');
> +    return handlerInfo && (handlerInfo.alwaysAskBeforeHandling == false &&
> +           handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally);

why the null check?

@@ +196,5 @@
> +    var domWindow = this.domWindow;
> +    var strings = getLocalizedStrings('chrome.properties');
> +    var message = getLocalizedString(strings, 'unsupported_feature');
> +
> +    var win = Services.wm.getMostRecentWindow('navigator:browser');

this may be wrong, depending on what you plan to do with it (is it fine to get closing windows? popups?), see http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1473

@@ +202,5 @@
> +    var notificationBox = win.gBrowser.getNotificationBox(browser);
> +
> +    var buttons = [{
> +      label: getLocalizedString(strings, 'open_with_different_viewer'),
> +      accessKey: null,

is there a specific reason to not have an accessKey?

::: browser/extensions/pdfjs/content/PdfJs.jsm
@@ +10,5 @@
> +const PREF_ENABLED = PREF_PREFIX + '.enabled';
> +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';

you may use a topic prefix here, like TOPIC_PDFJS_HANDLER_CHANGED

@@ +16,5 @@
> +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);

may defineLazyServiceGetter this one, doesn't look like it's needed on module loading

@@ +18,5 @@
> +
> +let mimeService = Cc["@mozilla.org/mime;1"]
> +                    .getService(Ci.nsIMIMEService);
> +
> +function getBoolPref(pref, def) {

safeGetBoolPref, just make clearer in later code it does something clever

nit: we usually prefix arguments with a, so here would be like aPref, aDefaultValue

@@ +29,5 @@
> +
> +// Register/unregister a class as a component.
> +let Factory = {
> +  registrar: null,
> +  aClass: null,

please don't name a property as if it was an argument, it's just going to make the code confusing.

nit: class is sort of confusing in js, you could name this targetConstructor or whatever

@@ +30,5 @@
> +// Register/unregister a class as a component.
> +let Factory = {
> +  registrar: null,
> +  aClass: null,
> +  register: function(aClass) {

nit: space after "function" for anonymous functions, or name them (usually we suggest to name functions for stacks readability)

@@ +33,5 @@
> +  aClass: null,
> +  register: function(aClass) {
> +    if (this.aClass) {
> +      return;
> +    }

how can this happen? this check looks like it may just hide bugs form us. plus you are already tracking the _registered status in the module object

@@ +34,5 @@
> +  register: function(aClass) {
> +    if (this.aClass) {
> +      return;
> +    }
> +    this.registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);

considered you just use it for (un)registration, it's likely not really useful to cache and keep a ref to it, just use it directly

@@ +38,5 @@
> +    this.registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);
> +    this.aClass = aClass;
> +    var proto = aClass.prototype;
> +    this.registrar.registerFactory(proto.classID, proto.classDescription,
> +      proto.contractID, this);

please indent arguments as
function(a, b
         c, d);

@@ +43,5 @@
> +  },
> +  unregister: function() {
> +    if (!this.aClass) {
> +      return;
> +    }

ditto, this is just hiding possible bugs

@@ +48,5 @@
> +    var proto = this.aClass.prototype;
> +    this.registrar.unregisterFactory(proto.classID, this);
> +    this.aClass = null;
> +  },
> +  // nsIFactory::createInstance

just // nsIFactory
also because if you actually implement nsIFactory you should implement all of it... I know lockFactory is going to be killed, but for now it's there and you should add it

you are also missing QueryInterface for nsIFactory

@@ +64,5 @@
> +    if (getBoolPref(PREF_ENABLED, false) && getBoolPref(PREF_FIRST_RUN, false)) {
> +      Services.prefs.setBoolPref(PREF_FIRST_RUN, false);
> +
> +      let handlerService = Cc['@mozilla.org/uriloader/handler-service;1'].
> +                            getService(Ci.nsIHandlerService);

please define just before use

nit: either indent aligning Cc with get, or . with [

@@ +70,5 @@
> +
> +      // Store the previous settings of preferredAction and
> +      // alwaysAskBeforeHandling in case we need to fall back to it.
> +      Services.prefs.setIntPref(PREF_PREVIOUS_ACTION, handlerInfo.preferredAction);
> +      Services.prefs.setBoolPref(PREF_PREVIOUS_ASK, handlerInfo.alwaysAskBeforeHandling);

where do we restore these saved prefs values? Is it something for future or it is just elsewhere?

@@ +86,5 @@
> +
> +    // 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);

I'm not going to whine about these not being removed since regardless this is a module object, but usually it's a good idea to either use weak observers or remove them

@@ +90,5 @@
> +    Services.obs.addObserver(this, PDFJS_HANDLER_CHANGED, false);
> +  },
> +  observe: function(subject, topic, data) {
> +    if (topic != 'nsPref:changed' && topic != PDFJS_HANDLER_CHANGED)
> +      return;

how can this happen? looks useless check, still covering possible bugs.

@@ +97,5 @@
> +      this._register();
> +    else
> +      this._unregister();
> +  },
> +  // pdf.js is only enabled if we're both selected as the pdf viewer and if the 

please add a newline between methods, if possible use proper javadocs for them

@@ +104,5 @@
> +    var handlerInfo = mimeService.
> +                        getFromTypeAndExtension('application/pdf', 'pdf');
> +
> +    var selectedAsHandler = handlerInfo && (handlerInfo.alwaysAskBeforeHandling == false &&
> +           handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally);

Why the null check? Either you should null check in both places (included init()) or none.
Looks like getFromTypeAndExtension always returns a nsIHandlerInfo even if an handler doesn't exist (http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/unit/test_handlerService.js#82)

@@ +105,5 @@
> +                        getFromTypeAndExtension('application/pdf', 'pdf');
> +
> +    var selectedAsHandler = handlerInfo && (handlerInfo.alwaysAskBeforeHandling == false &&
> +           handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally);
> +    return getBoolPref(PREF_ENABLED, false) && selectedAsHandler;

so you may just:

return getBoolPref(PREF_ENABLED, false) &&
       handlerInfo.alwaysAskBeforeHandling == false &&
       handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally;

@@ +109,5 @@
> +    return getBoolPref(PREF_ENABLED, false) && selectedAsHandler;
> +  },
> +  _register: function() {
> +    if (this._registered)
> +      return;

please annotate this fact through Component.utils.reportError before returning, since it's not expected

@@ +116,5 @@
> +    this._registered = true;
> +  },
> +  _unregister: function() {
> +    if (!this._registered)
> +      return;

as well as here

::: browser/extensions/pdfjs/test/browser_pdfjs_main.js
@@ +8,5 @@
>  function test() {
>    var tab;
>  
> +  const Cc = Components.classes;
> +  const Ci = Components.interfaces;

these should not be needed

@@ +36,5 @@
>      // Runs tests after all 'load' event handlers have fired off
>      setTimeout(function() {
> +      runTests(document, window, function() {
> +        finish();
> +      });

may just pass ", finish)"

::: browser/extensions/pdfjs/test/browser_pdfjs_savedialog.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */

please use the proper formatting from http://www.mozilla.org/MPL/headers/

@@ +6,5 @@
> +const TESTROOT = "http://example.com/browser/" + RELATIVE_DIR;
> +
> +function test() {
> +  const Cc = Components.classes;
> +  const Ci = Components.interfaces;

not needed

@@ +7,5 @@
> +
> +function test() {
> +  const Cc = Components.classes;
> +  const Ci = Components.interfaces;
> +  var tab;

define on first use, actually you may just move the code from the bottom to here... the order doesn't really matter to the observers you add later

@@ +24,5 @@
> +    gBrowser.removeTab(tab);
> +  });
> +
> +  tab = gBrowser.addTab(TESTROOT + "file_pdfjs_test.pdf");
> +  var newTabBrowser = gBrowser.getBrowserForTab(tab);

this looks unused

@@ +29,5 @@
> +}
> +
> +function changeMimeHandler(preferredAction, alwaysAskBeforeHandling) {
> +  const Cc = Components.classes;
> +  const Ci = Components.interfaces;

unneeded

@@ +43,5 @@
> +
> +  Services.obs.notifyObservers(null, 'pdfjs:handlerChanged', null);
> +
> +  // Refresh data
> +  mimeService = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService);

you did this just above...

@@ +57,5 @@
> +}
> +
> +function addWindowListener(aURL, aCallback) {
> +  const Cc = Components.classes;
> +  const Ci = Components.interfaces;

unneeded

Comment 24

[Brendan Dahl [:bdahl]]

Comments below on sections that I didn't change or need feedback. Everything else should be fixed.

> ::: browser/components/preferences/applications.js
> @@ +9,1 @@
> >  // Constants & Enumeration Values
> 
> you are changing application.js in the preferences dialog, though now we
> also have browser/components/preferences/in-content/applications.js for
> in-content preferences ui, they should probably be kept in sync for now
> otherwide that ui would do something different from what we expect.

I've copied applicaitons.js to the in-content one, but should these files be identical?

I noticed before my changes that these files are not completely synced up.  They are close but not the same.  This seems like a bad design.  Is there a reason for having two identical files?

> ::: browser/extensions/pdfjs/components/PdfStreamConverter.js
> @@ +8,5 @@
> >  const Cc = Components.classes;
> >  const Ci = Components.interfaces;
> >  const Cr = Components.results;
> >  const Cu = Components.utils;
> > +const MOZ_CENTRAL = true;
> 
> please document what this is about and how it's supposed to be used, looks
> strange and a bit too much specific... maybe it's just bad named if it's
> supposed to distinguish the extension from the built-in code... IS_EXTENSION
> = false; ?

It is very specific and in pdf.js land we refer to this build as the mozilla central build so I'd like to leave this as is. We also have a chrome extension.  I've added a comment to hopefully clarify this.

> 
> @@ +24,5 @@
> >  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);
> 
> defineLazyServiceGetter

I fixed this but there seems to be several styles of doing this, hopefully the one I chose is okay.

> 
> @@ +82,5 @@
> > +    // selected in the Application preferences.
> > +    var handlerInfo = mimeService.
> > +                        getFromTypeAndExtension('application/pdf', 'pdf');
> > +    return handlerInfo && (handlerInfo.alwaysAskBeforeHandling == false &&
> > +           handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally);
> 
> why the null check?

Not needed, removed. This was before I knew that it always returned a mime handler.

> 
> @@ +196,5 @@
> > +    var domWindow = this.domWindow;
> > +    var strings = getLocalizedStrings('chrome.properties');
> > +    var message = getLocalizedString(strings, 'unsupported_feature');
> > +
> > +    var win = Services.wm.getMostRecentWindow('navigator:browser');
> 
> this may be wrong, depending on what you plan to do with it (is it fine to
> get closing windows? popups?), see
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> nsBrowserGlue.js#1473

I'm not sure on this one and look to your expertise.  We only use it for getting a notification box which I don't know if any window will do ( you do have to give the notification box the domWindow).

> 
> @@ +202,5 @@
> > +    var notificationBox = win.gBrowser.getNotificationBox(browser);
> > +
> > +    var buttons = [{
> > +      label: getLocalizedString(strings, 'open_with_different_viewer'),
> > +      accessKey: null,
> 
> is there a specific reason to not have an accessKey?
>

No, fixed. The updated string will be in bug 742099.
 
> ::: browser/extensions/pdfjs/content/PdfJs.jsm
> @@ +70,5 @@
> > +
> > +      // Store the previous settings of preferredAction and
> > +      // alwaysAskBeforeHandling in case we need to fall back to it.
> > +      Services.prefs.setIntPref(PREF_PREVIOUS_ACTION, handlerInfo.preferredAction);
> > +      Services.prefs.setBoolPref(PREF_PREVIOUS_ASK, handlerInfo.alwaysAskBeforeHandling);
> 
> where do we restore these saved prefs values? Is it something for future or
> it is just elsewhere?

We don't restore these anywhere currently. It was required that we be able to turn off pdf.js with a hotfix so I stored them in case we need to restore in the hotfix.

> @@ +109,5 @@
> > +    return getBoolPref(PREF_ENABLED, false) && selectedAsHandler;
> > +  },
> > +  _register: function() {
> > +    if (this._registered)
> > +      return;
> 
> please annotate this fact through Component.utils.reportError before
> returning, since it's not expected
> 
> @@ +116,5 @@
> > +    this._registered = true;
> > +  },
> > +  _unregister: function() {
> > +    if (!this._registered)
> > +      return;
> 
> as well as here

It is expected that these get called and it should not error.  There can be multiple events that trigger the observe which will call these while they may be in the same state.

Comment 25

[Brendan Dahl [:bdahl]]

Attachment: pdf.js changes (most unrelated to this)

Remove head.js

Comment 26

[Brendan Dahl [:bdahl]]

Attachment: firefox changes

Addresses mak's comments.

Comment 27

[Marco Bonardo [:mak]]

(In reply to Brendan Dahl from comment #24)
> I've copied applicaitons.js to the in-content one, but should these files be
> identical?
> 
> I noticed before my changes that these files are not completely synced up. 
> They are close but not the same.  This seems like a bad design.  Is there a
> reason for having two identical files?

Honestly I'm not up-to-date regarding that, I found surprising we duplicated all the code as well, but still we should try to keep both of them working, cause users testing the new in-content preferences would have broken functionality.  We should clarify the intentions with Jared who drove that effort.

> > ::: browser/extensions/pdfjs/components/PdfStreamConverter.js
> It is very specific and in pdf.js land we refer to this build as the mozilla
> central build so I'd like to leave this as is. We also have a chrome
> extension.  I've added a comment to hopefully clarify this.

ok, thanks. For us is confusing having just MOZ_CENTRAL cause central is just one of the many trees this code goes through.  The comment is appreciated.

> > @@ +196,5 @@
> I'm not sure on this one and look to your expertise.  We only use it for
> getting a notification box which I don't know if any window will do ( you do
> have to give the notification box the domWindow).

I don't know well the internals of notificationBox, but looks like a bunch of code just does the same you do, so let's keep it as it is for now.

> > ::: browser/extensions/pdfjs/content/PdfJs.jsm
> > where do we restore these saved prefs values? Is it something for future or
> > it is just elsewhere?
> 
> We don't restore these anywhere currently. It was required that we be able
> to turn off pdf.js with a hotfix so I stored them in case we need to restore
> in the hotfix.

please add a code comment saying that.

> It is expected that these get called and it should not error.  There can be
> multiple events that trigger the observe which will call these while they
> may be in the same state.

ok, maybe rename the methods to _ensureRegistered and _ensureUnregistered then?

Comment 28

[Marco Bonardo [:mak]]

Comment on attachment 629438 [details] [diff] [review]
firefox changes

Review of attachment 629438 [details] [diff] [review]:
-----------------------------------------------------------------

Looks much better, I assume you also manually tested this properly and already verified there are no added refcnt leaks reported in browser-chrome tests or manual testing.
That said, the code changes don't look particularly scary and are well contained, so I feel like I can mark this, based on the previous assumptions.

::: browser/components/preferences/applications.js
@@ +830,5 @@
> +InternalHandlerInfoWrapper.prototype = {
> +  __proto__: HandlerInfoWrapper.prototype,
> +
> +  // Override store so we can notify any code that is listening for if it needs
> +  // to be registered or unregistered.

...so we can notify any code listening for registration or unregistration of this handler.

@@ +838,5 @@
> +  },
> +
> +  get description() {
> +    return this.element("bundlePreferences").getString(this._appPrefLabel);
> +  }

still keep an "enabled" implementation here that just throws Cr.NS_ERROR_NOT_IMPLEMENTED here, and add a comment that it must be overridden.

::: browser/components/preferences/in-content/applications.js
@@ +2,5 @@
> +# -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */

please don't change this, the previous format was correct

@@ +15,5 @@
> +var Ci = Components.interfaces;
> +var Cr = Components.results;
> +/*
> +#endif
> +*/

Are these really needed in the in-content version? I think they're not, cause it's running in the browser scope where these are defined already, so please remove these changes

@@ +848,5 @@
> +  _appPrefLabel: "portableDocumentFormat",
> +  get enabled() {
> +    return !Services.prefs.getBoolPref(PREF_PDFJS_DISABLED);
> +  },
> +};

did you test this in-content ui working properly too?

::: browser/extensions/pdfjs/components/PdfStreamConverter.js
@@ +133,1 @@
>                          getFromTypeAndExtension('application/pdf', 'pdf');

nit: align dots:
Svc.mime
   .getFrom...

::: browser/extensions/pdfjs/content/PdfJs.jsm
@@ +32,5 @@
> +
> +// Register/unregister a constructor as a component.
> +let Factory = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFactory]),
> +  targetConstructor: null,

nit: could be "private" (as _targetConstructor)

@@ +34,5 @@
> +let Factory = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFactory]),
> +  targetConstructor: null,
> +
> +  register: function(targetConstructor) {

please name the functions for more readable stacks

@@ +45,5 @@
> +
> +  unregister: function() {
> +    var proto = this.targetConstructor.prototype;
> +    
> +    var registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);

nit: unneeded newline

@@ +59,5 @@
> +  },
> +
> +  // nsIFactory
> +  lockFactory: function(lock) { 
> +    // No longer used as of gecko 1.7.

please make it throw Cr.NS_ERROR_NOT_IMPLEMENTED;

@@ +76,5 @@
> +      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.
> +      Services.prefs.setIntPref(PREF_PREVIOUS_ACTION, handlerInfo.preferredAction);
> +      Services.prefs.setBoolPref(PREF_PREVIOUS_ASK, handlerInfo.alwaysAskBeforeHandling);

improve the comment saying that restoring may be implemented in future but it's not currently

@@ +109,5 @@
> +  
> +  /**
> +   * pdf.js is only enabled if it is both selected as the pdf viewer and if the 
> +   * global switch enabling it is true.
> +   * @returns {boolean} Wether or not it's enabled.

the correct form is @return (no s)

@@ +117,5 @@
> +    if (disabled)
> +      return false;
> +
> +    var handlerInfo = Svc.mime.
> +                        getFromTypeAndExtension('application/pdf', 'pdf');

nit: align dots
Svc.mime
   .getFrom...

@@ +122,5 @@
> +    return handlerInfo.alwaysAskBeforeHandling == false &&
> +           handlerInfo.preferredAction == Ci.nsIHandlerInfo.handleInternally;
> +  },
> +
> +  _register: function() {

I suggested in previous comments making these _ensureRegistered and _ensureUnregistered, cause as you said they may be invoked multiple times

Comment 29

[Marco Bonardo [:mak]]

Comment on attachment 629437 [details] [diff] [review]
pdf.js changes (most unrelated to this)

Review of attachment 629437 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know much of the pdf.js details here, the browser parts seems sane though

Comment 30

[Brendan Dahl [:bdahl]]

(In reply to Marco Bonardo [:mak] from comment #28)
> ::: browser/components/preferences/in-content/applications.js
> 
> @@ +15,5 @@
> > +var Ci = Components.interfaces;
> > +var Cr = Components.results;
> > +/*
> > +#endif
> > +*/
> 
> Are these really needed in the in-content version? I think they're not,
> cause it's running in the browser scope where these are defined already, so
> please remove these changes

No not needed. Like I said I just copied the regular app.js to the in-content one.  I'll update and only copy my changes.

> 
> @@ +848,5 @@
> > +  _appPrefLabel: "portableDocumentFormat",
> > +  get enabled() {
> > +    return !Services.prefs.getBoolPref(PREF_PDFJS_DISABLED);
> > +  },
> > +};
> 
> did you test this in-content ui working properly too?

Yes, it seems fine. However, I've discovered a bug that I can reproduce without my patches applied.  The second time you go to application preferences one of the options no longer works e.g. you can't change it.  With pdf.js patches applied it always seems to be PDF, without it applied it some other content type.  I'll go search for/file a bug for this.

> ::: browser/extensions/pdfjs/components/PdfStreamConverter.js
> @@ +133,1 @@
> >                          getFromTypeAndExtension('application/pdf', 'pdf');
> 
> nit: align dots:
> Svc.mime
>    .getFrom...
> 
> ::: browser/extensions/pdfjs/content/PdfJs.jsm
> @@ +34,5 @@
> > +let Factory = {
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFactory]),
> > +  targetConstructor: null,
> > +
> > +  register: function(targetConstructor) {
> 
> please name the functions for more readable stacks

I've named the anonymous functions in pdfjs.jsm. Do you want me to name every anonymous function I added or just in this file? In applicaitons.js I followed the style it already had with no anonymous function names.

Comment 31

[Brendan Dahl [:bdahl]]

Attachment: firefox changes

Comment 32

[Brendan Dahl [:bdahl]]

Hopefully last try run:
https://tbpl.mozilla.org/?tree=Try&rev=2268de4b8a7c

New bug filed as mentioned above:
bug 760842

Comment 33

[Anant Narayanan [:anant]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb64eb2b9a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb64eb2b9a1

Comment 34

[Anant Narayanan [:anant]]

Oops, second link was supposed to be:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83eb4c680cb8

Comment 35

[Matt Brubeck (:mbrubeck)]

It looks like this landing caused a memory usage regression in some Talos benchmarks:

http://graphs.mozilla.org/graph.html#tests=[[29,63,18]]&sel=1338818805921.5896,1338829385513.4265&displayrange=7&datatype=running

https://groups.google.com/d/msg/mozilla.dev.tree-management/sN60GtP4aBc/WxksjMpaAroJ

Comment 36

[Alex Keybl [:akeybl]]

Comment on attachment 629437 [details] [diff] [review]
pdf.js changes (most unrelated to this)

[Triage Comment]

Comment 37

[Alex Keybl [:akeybl]]

(In reply to Matt Brubeck (:mbrubeck) from comment #35)
> It looks like this landing caused a memory usage regression in some Talos
> benchmarks:
> 
> http://graphs.mozilla.org/graph.html#tests=[[29,63,18]]&sel=1338818805921.
> 5896,1338829385513.4265&displayrange=7&datatype=running
> 
> https://groups.google.com/d/msg/mozilla.dev.tree-management/sN60GtP4aBc/
> WxksjMpaAroJ

We need bugs on file for any perf regressions we find. Please also nominate for tracking-firefox15:? once filed.

Comment 38

[Matt Brubeck (:mbrubeck)]

(In reply to Alex Keybl [:akeybl] from comment #37)
> We need bugs on file for any perf regressions we find. Please also nominate
> for tracking-firefox15:? once filed.

Filed bug 761334.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ab0e7683fdf
https://hg.mozilla.org/releases/mozilla-aurora/rev/6770b09f974d

Comment 40

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/mozilla-central/rev/ecb64eb2b9a1
https://hg.mozilla.org/mozilla-central/rev/83eb4c680cb8

Comment 41

[u123541]

FF16.0a1: This pdf.js viewer still doesn't print a Google Calendar (not a pdf URL; generated?) -- the error (see bug 759002) is gone; but after displaying "Loading.. 100%"...  nothing -- seems stalled there.  Can still use the Save As... workaround to save it to okular and print from there.  However, going to a link containing a pdf file works.

Comment 42

[u123541]

Print is finally sending calendar to okular again as expected  :)

Comment 43

[[e-mail]]

Comment on attachment 629492 [details] [diff] [review]
firefox changes

>+// True only if this is the version of pdf.js that is included with firefox.
>+const MOZ_CENTRAL = true;
I wish there was a better way of achieving this that avoided having to fork the file.

Comment 44

[Brendan Dahl [:bdahl]]

(In reply to [e-mail] from comment #43)
> Comment on attachment 629492 [details] [diff] [review]
> firefox changes
> 
> >+// True only if this is the version of pdf.js that is included with firefox.
> >+const MOZ_CENTRAL = true;
> I wish there was a better way of achieving this that avoided having to fork
> the file.

What are you trying to do?

Comment 45

[Alfredos-Panagiotis Damkalis [:fredy]]

The option for pdf.js is now at Application Preferences as "preview in Aurora".

I'll change this bug's status to verified fixed