We should add built-in PDF support to Firefox using PDF.js. Built-in PDF support has been an enhancement request with a lot of support for a long time. Using PDF.js we can add support while avoiding or mitigating many of the security, stability, and portability issues associated with binary PDF layout engines. We can leave the feature pref'd off until we're confident about quality and performance, but PDF.js is far enough along that it's probably time to start working on full integration and getting more widespread testing.

This is a proposed goal for 2012 Q1 as well. We'll be discussing this more as we nail down our goals and what we think needs to be done before we can ship it. https://github.com/mozilla/pdf.js/issues/970

As you probably know, the pdf.js team is shipping pdf.js as an extension currently. The biggest blockers in my mind to shipping pdf.js by default with firefox are - option to load PDF in default system viewer when we hit NYI features. - visual design love. The current UI is very gnome, doesn't integrate with system well. - firefox URL bar needs to remain the original PDF URL, and probably update with navigation (to foo.pdf#page2, e.g.) for bookmarking/sharing/etc. We might need pdf.js to register a content handler or something to implement this. - no responsiveness jank. pdf.js is still jerky at times on my very powerful server machine. - confirm that pdf.js works in embedded contexts, like nested <iframe>s. I think it should, but this needs to be checked. - tests of pdf.js integration in ff UI. This is orthogonal to the full renderer tests, which we should absolutely keep running on ec/2. Need to ensure that FF UI changes don't break pdf.js's UI. What would satisfy me are mochitest-browser-chrome tests that load a helloworld.pdf with multiple pages, and ensure that navigation, "save as", scrolling, etc. work correctly. In-UI reporting of broken PDFs is a nice-to-have, but not a blocker for ff integration IMHO.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2) > - no responsiveness jank. pdf.js is still jerky at times on my very > powerful server machine. > (This is of course to the extent possible. We can't have no-jank without out-of-process content or off-main-thread canvas.) > - tests of pdf.js integration in ff UI. This is orthogonal to the full > renderer tests, which we should absolutely keep running on ec/2. Need to > ensure that FF UI changes don't break pdf.js's UI. What would satisfy me > are mochitest-browser-chrome tests that load a helloworld.pdf with multiple > pages, and ensure that navigation, "save as", scrolling, etc. work correctly. > This needs to include tests that pdf.js works in nested browsing contexts.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #2) > > - no responsiveness jank. pdf.js is still jerky at times on my very > > powerful server machine. > > > > (This is of course to the extent possible. We can't have no-jank without > out-of-process content or off-main-thread canvas.) Can't we create a <browser remote="true"/> element uses for parsing/rendering the PDF. And when it is rendered we can send a message to the main thread with the needed infos to show it in the current tab?

> no responsiveness jank. pdf.js is still jerky at times on my very powerful server machine. I think a lot of that jerkiness comes from our text selection algorithm. I'm creating a new option "?disableTextLayer=true" so we can see the difference: https://github.com/mozilla/pdf.js/pull/1024 It's tough on the UI since we have to add so many divs synchronously to the DOM on the fly. This algo is already evolved from a previous one, and uses setInterval() for every div in order to free up the UI. There's a discussion here on why the current algo can't lump all divs into one before adding them to the DOM: https://github.com/mozilla/pdf.js/pull/937#issuecomment-3162583 We might have to revisit this though if that's the main culprit. > Can't we create a <browser remote="true"/> element uses for parsing/rendering the PDF. Anything that's out-of-process (the <browser> suggestion, or canvas-in-Workers) would be fantastic. The ideal solution IMO would be the latter, since that would enable others to take advantage of it without requiring access to FF's chrome. Is it realistic to expect either solution any time soon? Or, is it easier/faster to implement one versus the other in FF? As for modifying pdf.js to implement this, in the former we'd have to write a custom viewer for FF (less work for us), whereas in the latter I think we'd have to essentially rewrite our current Worker implementation (unless we're willing to work with two layers of Workers, including a Worker-within-a-Worker, instead of just one).

(In reply to Artur Adib from comment #5) > Is it realistic to expect either solution any time soon? Or, is it > easier/faster to implement one versus the other in FF? Don't expect canvas-in-a-worker anytime soon. That relies on a new DOM bindings framework being created. <browser remote="true"> might work but it wouldn't integrate very well since PDF.js would basically have to live in its own tab. You wouldn't be able to navigate to a PDF in the same tab, view a PDF in an IFRAME, etc.

<browser remote="true"> is a really clever idea, but it also has the problem that it'll break extensions unless we can hide all ways extensions can detect it, which I doubt we can.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > (In reply to Artur Adib from comment #5) > > <browser remote="true"> might work but it wouldn't integrate very well since > PDF.js would basically have to live in its own tab. You wouldn't be able to > navigate to a PDF in the same tab, view a PDF in an IFRAME, etc. <browser remote="true"> will just be used as a place to do the computing intensive task of PDF.js. Viewing the pdf will still be in the same tab. For example the remote tab will read/decode the pdf, renders it onto a canvas and use the messageManager API to send the image data to some PDF.js code running in the chrome process. This code will then dispatch the image data to the tab uses to view/navigate the document. Does it sounds correct?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7) > <browser remote="true"> is a really clever idea, but it also has the problem > that it'll break extensions unless we can hide all ways extensions can > detect it, which I doubt we can. I've heard that the JetPack team use the content process for loading some of their extensions. We should try to get some JetPack folks there to see how they have done it.

(In reply to Vivien Nicolas (:vingtetun) from comment #8) > For example the remote tab will read/decode the pdf, renders it onto a > canvas and use the messageManager API to send the image data to some PDF.js > code running in the chrome process. > This code will then dispatch the image data to the tab uses to view/navigate > the document. > > Does it sounds correct? Yes, that would work, although transferring the image from one process to another won't be super fast at present. We could accelerate it at some point.

(In reply to Vivien Nicolas (:vingtetun) from comment #9) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #7) > > <browser remote="true"> is a really clever idea, but it also has the problem > > that it'll break extensions unless we can hide all ways extensions can > > detect it, which I doubt we can. > > I've heard that the JetPack team use the content process for loading some of > their extensions. We should try to get some JetPack folks there to see how > they have done it. You're right. We started working on jetpack addons living in remote processes by using <browser remote=true>. Our code is now ready to start using it. But as electrolisys support has been set "on hold", we decided to stop this work as we weren't sure that <browser remote=true> will continue to work and being maintained by the platform! So until any e10s revival, we are still using <browser> but without remote=true attribute. About extension compatibilty, you may simply create a xul document that will load a new <browser remote="true"> inside of it. But you will face the same question than us: is <browser remote=true> maintained and will platform team improve or debug it if you face any issue with it?

(In reply to Alexandre Poirot (:ochameau) from comment #11) > About extension compatibilty, you may simply create a xul document that will > load a new <browser remote="true"> inside of it. But you will face the same > question than us: is <browser remote=true> maintained and will platform team > improve or debug it if you face any issue with it? Yes, we will, partly because B2G depends on it.

Re:<browser remote="true">... I was really hoping to come up with a web-standards solution to the jerkiness, even if we have to wait for the platform to evolve (which I thought was one of the goals of the project). If we start using chrome solutions my fear is that we will be less motivated to push for a web solution. Meanwhile one thing we can do to improve responsiveness is to disable text selection. We can compare the effect of text selection on the UI: http://mozilla.github.com/pdf.js/web/viewer.html http://mozilla.github.com/pdf.js/web/viewer.html?disableTextLayer=true Would that be enough until we can render off-process?

On 06/01/2012 14:58, bugzilla-daemon@mozilla.org wrote: > https://bugzilla.mozilla.org/show_bug.cgi?id=714712 > > --- Comment #13 from Artur Adib <aadib@mozilla.com> 2012-01-06 05:58:17 PST --- > Re:<browser remote="true">... > > I was really hoping to come up with a web-standards solution to the jerkiness, > even if we have to wait for the platform to evolve (which I thought was one of > the goals of the project). If we start using chrome solutions my fear is that > we will be less motivated to push for a web solution. I agree on the idea behind that. But PDF.js is not the only motivation for having canvas on a worker so its not clear that using a remote browser will slow down the efforts to do canvas-in-a-worker. ( See for example bug 709490, but roc may know much better than me) IMHO doing the computing/rendering task into a <browser remote="true"/> will shape the code of PDF.js to be ready to switch to canvas-in-a-worker when it will be ready. Not instantiating a second process should be a good motivation to switch as soon as possible too! > > Meanwhile one thing we can do to improve responsiveness is to disable text > selection. We can compare the effect of text selection on the UI: > > http://mozilla.github.com/pdf.js/web/viewer.html > http://mozilla.github.com/pdf.js/web/viewer.html?disableTextLayer=true > > Would that be enough until we can render off-process? > I think you should keep it. AFAIK it slow down the rendering only because this is done during the rendering loop. Can't you create text only when the user starts an actual selection (like using the mouse button or hitting some keys)?

(In reply to Vivien Nicolas (:vingtetun) from comment #8) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > > (In reply to Artur Adib from comment #5) > > > > <browser remote="true"> might work but it wouldn't integrate very well since > > PDF.js would basically have to live in its own tab. You wouldn't be able to > > navigate to a PDF in the same tab, view a PDF in an IFRAME, etc. > > <browser remote="true"> will just be used as a place to do the computing > intensive task of PDF.js. Viewing the pdf will still be in the same tab. > That's what I understood you to mean. The issue is, you're going to run into the same problem of having to completely hide the <browser remote> from other addons, because if they try to touch it they'll break. I don't know what that entails, but I suspect it would be hard and possibly turn into whack-a-mole. But I also think you know more about what's required for that than me :). (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > (In reply to Vivien Nicolas (:vingtetun) from comment #8) > > For example the remote tab will read/decode the pdf, renders it onto a > > canvas and use the messageManager API to send the image data to some PDF.js > > code running in the chrome process. > > This code will then dispatch the image data to the tab uses to view/navigate > > the document. > > > > Does it sounds correct? > > Yes, that would work, although transferring the image from one process to > another won't be super fast at present. We could accelerate it at some point. pdf.js won't be usable on mobile devices if we serialize canvas data. That was tried with fennec-2.0alpha and it didn't work.

Logistical point: can we get a project page created for this so PMs can help track this and get resources as needed? Similarly, who's going to be driving this and handling the actual integration into Firefox (and all the process-related bits that entails)?

On 06/01/2012 21:15, bugzilla-daemon@mozilla.org wrote: > https://bugzilla.mozilla.org/show_bug.cgi?id=714712 > > --- Comment #15 from Chris Jones [:cjones] [:warhammer] <jones.chris.g@gmail.com> 2012-01-06 12:15:18 PST --- > (In reply to Vivien Nicolas (:vingtetun) from comment #8) >> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) >>> (In reply to Artur Adib from comment #5) >>> >>> <browser remote="true"> might work but it wouldn't integrate very well since >>> PDF.js would basically have to live in its own tab. You wouldn't be able to >>> navigate to a PDF in the same tab, view a PDF in an IFRAME, etc. >> <browser remote="true"> will just be used as a place to do the computing >> intensive task of PDF.js. Viewing the pdf will still be in the same tab. >> > That's what I understood you to mean. The issue is, you're going to run into > the same problem of having to completely hide the <browser remote> from other > addons, because if they try to touch it they'll break. I don't know what that > entails, but I suspect it would be hard and possibly turn into whack-a-mole. > But I also think you know more about what's required for that than me :). IMHO binding the remote browser with a tag name different than 'browser' should be enough. For example instead of: browser[remote="true"] { -moz-binding: url("chrome://browser/content/bindings/browser.xml#remote-browser"); } we could use something like: pdfrunner { -moz-binding: url("chrome://browser/content/bindings/browser.xml#remote-browser"); } It's very unlikely that extension would look for such a tag (but this tag name can be improved!)

Items to do: - Decide what tests are needed? (install,UI) - Disable by default

(In reply to Yury (:yury) from comment #18) > Created attachment 594840 [details] [diff] [review] > Adding pdf.js to the mozilla-central > > Items to do: > - Decide what tests are needed? (install,UI) > - Disable by default Is it possible to split this patch in two parts? One that add the necessary files to add pdf.js into the the extensions/ directory of mozilla-central and one that add the actual pdf.js's files. This will be much easier to f?.

Comment on attachment 594840 [details] [diff] [review] Adding pdf.js to the mozilla-central Packaging this code as a default extension seems like a reasonable way to begin integration. Provided it doesn't regress perf and other tests when packaged this way and disabled by default, getting it into the tree like this will make it easier for folks to test the functionality. So f+ for the overall approach. I tried building and testing with the patch, and Firefox does build and start (modulo some crashes that I can't reproduce anymore and suspect are unrelated), but it doesn't actually load the extension. It looks like that's because the extension relies on the XPI being unpacked so that bootstrap.js can dynamically load the chrome.manifest file, but the Makefile, unlike the extensions manager itself, doesn't pay attention to that flag and unconditionally packs extensions. A default extension doesn't need to be restartless, which means it doesn't need to load chrome.manifest dynamically, which means it doesn't need to be unpacked. And packing the extension is good for perf. So rather than teaching the Makefile that this extension should remain unpacked, turn this into a restartful extension by removing the bootstrap.js file and the unpack=true flag in install.rdf. It looks like bootstrap.js only loads the manifest and sets 'extensions.pdf.js.active', so its code isn't needed once you make the extension restartful. The only thing I'm unsure about is whether registering the stream converter component, and calling it when loading a PDF document, by itself regresses performance noticeably, even though the component can return early if the feature is disabled. If so, then you'll want to avoid registering the component via chrome.manifest and instead register it dynamically at runtime if the feature is enabled, f.e. via another component that registers for a startup notification and conditionally registers the stream converter component.

The extension is disabled by default. (In reply to Vivien Nicolas (:vingtetun) from comment #19) > Is it possible to split this patch in two parts? One that add the necessary > files to add pdf.js into the the extensions/ directory of mozilla-central > and one that add the actual pdf.js's files. The patch is split to the pdf.js and mozilla-central specific code. P.S. https://tbpl.mozilla.org/?tree=Try&rev=bb5db053104d

Comment on attachment 595244 [details] [diff] [review] Adding pdf.js to the mozilla-central (without pdf.js) Thanks for splitting the patch quickly, it much more easier to give a feedback. Overall it sounds good to me. Just a few comments: >diff --git a/browser/app/profile/extensions/Makefile.in b/browser/app/profile/extensions/Makefile.in >--- a/browser/app/profile/extensions/Makefile.in >+++ b/browser/app/profile/extensions/Makefile.in >@@ -39,17 +39,20 @@ DEPTH = ../../../.. > topsrcdir = @top_srcdir@ > srcdir = @srcdir@ > VPATH = @srcdir@ The indentation is strange. > > DISTROEXT = $(call core_abspath,$(DIST))/bin/distribution/extensions > > include $(DEPTH)/config/autoconf.mk > >-DIRS = {972ce4c6-7e08-4474-a285-3208198ce6fd} >+DIRS = \ >+ {972ce4c6-7e08-4474-a285-3208198ce6fd} \ >+ uriloader@pdf.js \ >+ $(NULL) I wonder if the name uriloader@pdf.js still make sense, maybe pdf.js@mozilla.com ? >diff --git a/browser/app/profile/extensions/uriloader@pdf.js/Makefile.in b/browser/app/profile/extensions/uriloader@pdf.js/Makefile.in >new file mode 100644 >--- /dev/null >+++ b/browser/app/profile/extensions/uriloader@pdf.js/Makefile.in >@@ -0,0 +1,85 @@ >+# >+# ***** BEGIN LICENSE BLOCK ***** >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+# >+# The contents of this file are subject to the Mozilla Public License Version >+# 1.1 (the "License"); you may not use this file except in compliance with >+# the License. You may obtain a copy of the License at >+# http://www.mozilla.org/MPL/ >+# >+# Software distributed under the License is distributed on an "AS IS" basis, >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+# for the specific language governing rights and limitations under the >+# License. >+# >+# The Original Code is mozilla.org code. >+# >+# The Initial Developer of the Original Code is >+# Netscape Communications Corporation. >+# Portions created by the Initial Developer are Copyright (C) 2001 >+# the Initial Developer. All Rights Reserved. >+# >+# Contributor(s): >+# >+# Alternatively, the contents of this file may be used under the terms of >+# either the GNU General Public License Version 2 or later (the "GPL"), or >+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+# in which case the provisions of the GPL or the LGPL are applicable instead >+# of those above. If you wish to allow use of your version of this file only >+# under the terms of either the GPL or the LGPL, and not to allow others to >+# use your version of this file under the terms of the MPL, indicate your >+# decision by deleting the provisions above and replace them with the notice >+# and other provisions required by the GPL or the LGPL. If you do not delete >+# the provisions above, a recipient may use your version of this file under >+# the terms of any one of the MPL, the GPL or the LGPL. >+# >+# ***** END LICENSE BLOCK ***** >+ Use the brand new MPL 2.0 boilerplate :) http://www.mozilla.org/MPL/headers/ >+DEPTH = ../../../../.. >+topsrcdir = @top_srcdir@ >+srcdir = @srcdir@ >+VPATH = @srcdir@ The indentation seems strange here >+ >+include $(DEPTH)/config/autoconf.mk >+include $(topsrcdir)/config/rules.mk >+ >+FILES := \ >+ install.rdf \ >+ bootstrap.js \ Same here >diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in > @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/preview.png > #ifdef SHIP_FEEDBACK > @BINPATH@/distribution/extensions/testpilot@labs.mozilla.com.xpi > #endif >+@BINPATH@/distribution/extensions/uriloader@pdf.js/* Maybe you want to ifdef that so it will be only available in the nightly by default and you can remove it once pdf.js is ready to be shipped in the wild - see http://mxr.mozilla.org/mozilla-central/source/browser/installer/Makefile.in#96 Also it missed the components/PDFStreamConverter.js file in this patch :)

Comment on attachment 595244 [details] [diff] [review] Adding pdf.js to the mozilla-central (without pdf.js) Review of attachment 595244 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/extensions/uriloader@pdf.js/Makefile.in @@ +43,5 @@ > +include $(DEPTH)/config/autoconf.mk > +include $(topsrcdir)/config/rules.mk > + > +FILES := \ > + install.rdf \ You've got a stray tab here (and on the line with $(NULL).) @@ +76,5 @@ > + > +endef # do not remove the blank line! > + > +libs:: > + $(foreach file,$(FILES),$(_INSTALL_FILE)) This seems like overkill. (I assume you copy-pasted it from somewhere else?) You can just do: libs:: $(FILES) $(INSTALL) $^ $(DIST)/bin/extensions/uriloader@pdf.js (putting $(FILES) in the dependencies and using $^ means make will use the VPATH to find those files and use the full paths in $^.)

(In reply to Vivien Nicolas (:vingtetun) from comment #24) > Comment on attachment 595244 [details] [diff] [review] > Adding pdf.js to the mozilla-central (without pdf.js) > > > topsrcdir = @top_srcdir@ > > srcdir = @srcdir@ > > VPATH = @srcdir@ > > The indentation is strange. Fixed > > Use the brand new MPL 2.0 boilerplate :) http://www.mozilla.org/MPL/headers/ > Added MPL 2.0 for Makefile.in > >+DEPTH = ../../../../.. > >+topsrcdir = @top_srcdir@ > >+srcdir = @srcdir@ > >+VPATH = @srcdir@ > > > The indentation seems strange here > >+FILES := \ > >+ install.rdf \ > >+ bootstrap.js \ Fixed > > Maybe you want to ifdef that so it will be only available in the nightly by > default and you can remove it once pdf.js is ready to be shipped in the wild > - see > http://mxr.mozilla.org/mozilla-central/source/browser/installer/Makefile. > in#96 > I don't think we need to limit that to nightly only. Is it question about which version of FF will have the pdf.js? > I wonder if the name uriloader@pdf.js still make sense, maybe pdf.js@mozilla.com ? Would we also need to change the extension name and id to match the ff extension name? (In reply to Ted Mielczarek [:ted, :luser] from comment #25) > > +FILES := \ > > + install.rdf \ > > You've got a stray tab here (and on the line with $(NULL).) Fixed > This seems like overkill. (I assume you copy-pasted it from somewhere else?) > You can just do: > libs:: $(FILES) > $(INSTALL) $^ $(DIST)/bin/extensions/uriloader@pdf.js > > (putting $(FILES) in the dependencies and using $^ means make will use the > VPATH to find those files and use the full paths in $^.) Is there a way to preserve recursive structure of the FILES as well?

Oh, in that case you might just want to use $(DIR_INSTALL).

Just thinking of it the viewer should probably be xhtml and use a .dtd file to be easy to translate if there is any string. CC'ing Axel.

Added simple mochitest for pdf.js extension integration: tests if extension is in disabled state and loading of the first page of the tracemonkey.pdf. Try build run: https://tbpl.mozilla.org/?tree=Try&rev=e04ba8a5c1fe

Concerning pdf display UI, I think that we should look at Google Chrome to have an idea of what is good to do (as in many other UX areas).

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2) > As you probably know, the pdf.js team is shipping pdf.js as an extension > currently. The biggest blockers in my mind to shipping pdf.js by default > with firefox are > How good are we in terms of accessibility? The text selection feature added basic support for screen readers, but there might be more to do to get to match the usual Firefox accessibility standards.

I don't know.

(In reply to Vivien Nicolas (:vingtetun) from comment #28) > Just thinking of it the viewer should probably be xhtml and use a .dtd file > to be easy to translate if there is any string. CC'ing Axel. Right. Not sure about the strings in the js code, are those developer-focused or shown to the user? Another question I have, should we hook this up in a way that thunderbird/fennec can use it, too? Seems rather desktop-only at this point.

(In reply to Julian Viereck from comment #33) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #2) > > As you probably know, the pdf.js team is shipping pdf.js as an extension > > currently. The biggest blockers in my mind to shipping pdf.js by default > > with firefox are > > > > How good are we in terms of accessibility? One big problem I can see is caret navigation mode doesn't work. But in general I think it works but not really exciting. That can be quite dependent on pdf file though. For example, in one pdf I tried (http://www.selab.isti.cnr.it/ws-mate/example.pdf), every word is wrapped by absolutely positioned div element. There's no document markup like headings and etc. I realize that should be a problem of this particular pdf but in accessibility world document structure is more important than position on the screen, so putting divs into paragraphs and making headings (font-size guess?) would do a better job. Btw, I'm not sure how to appear flowing panel containing table of contents by keyboard.

(In reply to Julian Viereck from comment #33) > How good are we in terms of accessibility? The text selection feature added > basic support for screen readers, but there might be more to do to get to > match the usual Firefox accessibility standards. I would posit that there are a few different levels: 1) How much A11Y support is needed to land in mozilla-central (but pref'd off for Aurora etc)? I would say zero. A11Y is something that projects _should_ bake in from the start, but if they haven't or there are unique constraints (like with PDF), then we need to balance broader testing vs. changes expected to eventually support A11Y. 2) How much A11Y is needed to ship in a Firefox release? The default bar projects should expect is that things should be in a good state wrt A11Y. If that's not a reasonable expectation for pdf.js I'd be willing to listen to the argument, of course. 3) Somewhere among all this -- what is the current experience wrt A11Y with Acrobat/Evince/whatever? That may influence decisions one way or the other.

(In reply to Justin Dolske [:Dolske] from comment #37) > (In reply to Julian Viereck from comment #33) > > > How good are we in terms of accessibility? The text selection feature added > > basic support for screen readers, but there might be more to do to get to > > match the usual Firefox accessibility standards. > > I would posit that there are a few different levels: > > 1) How much A11Y support is needed to land in mozilla-central (but pref'd > off for Aurora etc)? I would say zero. A11Y is something that projects > _should_ bake in from the start, but if they haven't or there are unique > constraints (like with PDF), then we need to balance broader testing vs. > changes expected to eventually support A11Y. a11y support may be missed or not finished for new features landed on trunk but accessibility team and feature author *should* work together to make the feature accessible. > 2) How much A11Y is needed to ship in a Firefox release? The default bar > projects should expect is that things should be in a good state wrt A11Y. If > that's not a reasonable expectation for pdf.js I'd be willing to listen to > the argument, of course. any feature in release should be accessible, if not then we should have a really good reason why it's not. > 3) Somewhere among all this -- what is the current experience wrt A11Y with > Acrobat/Evince/whatever? That may influence decisions one way or the other. Adobe products are accessible. And caret navigation I mentioned before works in Acrobat Reader. Also you should keep in mind that Mozilla is one of leaders in accessibility world so if some product is not well accessible then it doesn't excuse Firefox to have poor accessibility as well.

Accessibility is definitely a priority for us, and since we're tinkering with a new text selection backend it is timely to consider solutions and pitfalls before progressing much further. Since this is likely a long discussion in and of itself, I'm starting a new thread on the topic here: https://bugzilla.mozilla.org/show_bug.cgi?id=727819

assigning to paul to review the code

(In reply to alexander :surkov from comment #38) > (In reply to Justin Dolske [:Dolske] from comment #37) ... > > 1) How much A11Y support is needed to land in mozilla-central (but pref'd > > off for Aurora etc)? [...] > > a11y support may be missed or not finished for new features landed on trunk > but accessibility team and feature author *should* work together to make the > feature accessible. Yep! Definitely! > > 3) Somewhere among all this -- what is the current experience wrt A11Y with > > Acrobat/Evince/whatever? That may influence decisions one way or the other. > > Adobe products are accessible. And caret navigation I mentioned before works > in Acrobat Reader. Also you should keep in mind that Mozilla is one of > leaders in accessibility world so if some product is not well accessible > then it doesn't excuse Firefox to have poor accessibility as well. Also agreed, just wanted to start thinking about how pdf.js and existing tools compare.

(In reply to Justin Dolske [:Dolske] from comment #41) > > > 3) Somewhere among all this -- what is the current experience wrt A11Y with > > > Acrobat/Evince/whatever? That may influence decisions one way or the other. > > > > Adobe products are accessible. And caret navigation I mentioned before works > > in Acrobat Reader. Also you should keep in mind that Mozilla is one of > > leaders in accessibility world so if some product is not well accessible > > then it doesn't excuse Firefox to have poor accessibility as well. > > Also agreed, just wanted to start thinking about how pdf.js and existing > tools compare. Oh, I see, existing products analysis is what we need to do, always. It seems I treated "one way or the other" too wide.

Updating to current m-c. Try run: https://tbpl.mozilla.org/?tree=Try&rev=4afde1727f68

Our Mochitest seems to be causing other tests to fail on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=4afde1727f68 On Windows in particular it times out, but otherwise our own test seems to pass. Not sure what we're doing wrong. We're using a very simple test - everything passes when I run it locally on my Mac. Here's the source: https://bugzilla.mozilla.org/attachment.cgi?id=600296&action=diff#a/browser/app/profile/extensions/uriloader@pdf.js/test/browser_pdfjs_main.js_sec1 Any feedback is very welcome, thanks! PS: We're total Mochitest newbies :)

You want SimpleTest.finish() instead of finish().

Hi Josh, thanks for the prompt response. If I do SimpleTest.finish() the test never finishes. If I do both SimpleTest.waitForExplicitFinish() and SimpleTest.finish() the test finishes OK, but I get: Browser Chrome Test Summary Passed: 0 Failed: 0 Todo: 0 instead of the previous "Passed: 2". Is that how it's supposed to be?

Oh, I'm sorry, I didn't notice that this was a browser-chrome test instead of a regular mochitest one. You can ignore comment 46.

So I can repro your failure on windows: (run from <objdir>/_tests/testing/mochitest): $ python runtests.py --autorun --browser-chrome --log-file=foo.log --test-path=browser/app/profile/extensions/uriloader@pdf.js/test Running the same commandline on linux passes. The reason you're hanging is that your pagechange event is never fired. Looking at the code, I see that you always create the event with bubbling set to false. Which is probably fine because we should have the same DOM tree on both windows and linux/mac. Which event dispatch of the pagechange event is the one that you're trying to catch? Can you instrument that code and see if it is working properly on windows? I imagine it is the set Page code, but I don't see anything odd there. The other thing here is that there is a long delay on my linux VM's pageload of the pdf.js page and the same page load on windows. I mention this because your test does addTab(url) which will perform the page load, and then your test attaches the event listener for the pagechange event to that tab. Could that event listener be on the larger window? This way you would be certain not to race between event dispatch and setting your event listener. I tried putting the listener in the test on the window, but it didn't work, but maybe thinking about a possible race condition will spark an idea on your side. Those are my observations so far.

When do we expect this to land?

Clint- Thanks so much for your feedback. I managed to get moz-central for Windows built and running on my laptop's VMWare - I can definitely reproduce the problem using your command-line. I tried pursuing the race condition thread, but I don't think that's what the problem is (I tried for example simply listening for "load", to no avail). I dug a little deeper and found that the problem is in the HTTP server, during the request for the test PDF (http://example.com/browser/browser/app/profile/extensions/uriloader@pdf.js/test/file_pdfjs_test.pdf): * On Mac, the response header contains a "Content-Type:application/pdf" field, which makes the addon kick in * On Windows, the header contains instead "Content-Type:application/octet-stream", which makes Firefox open a "Save file" dialog Is there a way to patch our HTTP server to serve up the right content type for PDFs?

I expect you're hitting the failure case here, then: http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js#2890 If you can get access to the nsIHttpServer object, you might be able to call registerContentType: http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js#2439

Just add it to the list here: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/server.js#237 That will fix it for all Mochitest instances.

Thanks guys, modifying Mochitest's server did the trick on Windows. I'm attaching the patch for testing/mochitest/server.js. On a side note- I can't seem to get make/pymake to copy the test server file automatically to the Windows obj-.../ directory without doing a full build - on OS X, running: TEST_PATH=... make -C obj-...dir/ mochitest-browser-chrome does the trick, but not on Windows. Hopefully that won't cause any problems for the try server.

There's no copying involved on OS X, the relevant files are symlinks. That's not the case on Windows.

You can run make in $objdir/testing/mochitest to get the changes to take effect on Windows.

Latest try run https://tbpl.mozilla.org/?tree=Try&rev=2226027f826a

Security review is complete now. A couple of questions/comments: https://github.com/mozilla/pdf.js/blob/master/src/fonts.js#L534 The code from line 534 onwards seems a little dangerous, as there seems to be variables which take values from the PDF used without escaping. However I tested and couldn't find a way to control anything here (font names, in particular), but its not easy to test. I understand that this code is a hack anyways, and will be replaced eventually, but just drawing attention to it since it may lead to another XSS bug. https://github.com/mozilla/pdf.js/blob/master/extensions/firefox/components/PdfStreamConverter.js#L133 Should we whitelist GET here instead of blacklisting POST? Probably a minor thing, since I can't think of an attack scenario here with other HTTP methods, but maybe just to be on the safe side?

(In reply to Paul Theriault [:pauljt] from comment #61) > Security review is complete now. A couple of questions/comments: > > https://github.com/mozilla/pdf.js/blob/master/src/fonts.js#L534 > The code from line 534 onwards seems a little dangerous, as there seems to > be variables which take values from the PDF used without escaping. I wrote this code initially. I remember checking very carefully that the names are properly sanitized before they reach the string builder there. But it's definitely worth re-verifying.

Okay, looks like the mime type fix did not help. https://tbpl.mozilla.org/?tree=Try&rev=0ed81318d052 generate same errors as https://tbpl.mozilla.org/?tree=Try&rev=4afde1727f68: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_select_confirm.js | TypeError: row.keep is not a function Bug 679588 - Intermittent browser_select_update.js or browser_select_confirm.js or browser_select_selection.js | Test timed out followed by | Found a after previous test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_select_confirm.js | Test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_select_confirm.js | Found a after previous test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_select_update.js | Test timed out Bug 679588 - Intermittent browser_select_update.js or browser_select_confirm.js or browser_select_selection.js | Test timed out followed by | Found a after previous test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_select_update.js | Found a after previous test timed out

The mochitests fail due to browser_select_update.js and browser_select_confirm.js test logic. Sending for review to the tests author

To avoid future problems with ff version changes, the install.rdf has to be modified to <em:maxVersion>*</em:maxVersion> Fixing of browser_select_update.js and browser_select_confirm.js helped. Last try https://tbpl.mozilla.org/?tree=Try&rev=9da8037f2b97

Comment on attachment 605646 [details] [diff] [review] add built-in PDF support to Firefox with PDF.js (applied after test and pdf.js patches) Review of attachment 605646 [details] [diff] [review]: ----------------------------------------------------------------- Don't forget to ask a build system peer to review ::: browser/app/profile/extensions/Makefile.in @@ +37,5 @@ > > +DEPTH = ../../../.. > +topsrcdir = @top_srcdir@ > +srcdir = @srcdir@ > +VPATH = @srcdir@ If you're reindenting anyway, just one space before the '='. ::: browser/app/profile/extensions/uriloader@pdf.js/Makefile.in @@ +4,5 @@ > + > +DEPTH = ../../../../.. > +topsrcdir = @top_srcdir@ > +srcdir = @srcdir@ > +VPATH = @srcdir@ Same here @@ +6,5 @@ > +topsrcdir = @top_srcdir@ > +srcdir = @srcdir@ > +VPATH = @srcdir@ > + > +DIRS = test TEST_DIRS, surely?

> Okay, looks like the mime type fix did not help The mime patch fixed the only fail on Windows due to our own tests - we still need it. Compare the outputs under "Win opt > oth" between: https://tbpl.mozilla.org/?tree=Try&rev=4afde1727f68 https://tbpl.mozilla.org/?tree=Try&rev=0ed81318d052 > Don't forget to ask a build system peer to review That would be very helpful! Any suggestions?

(In reply to Yury (:yury) from comment #65) > To avoid future problems with ff version changes, the install.rdf has to be > modified to > > <em:maxVersion>*</em:maxVersion> Since it'll be shipped included, and only tested on that shipped version of the app, it might be better to make it compatible only with that specific version. To do that, run it through the preprocessor, and use @FIREFOX_VERSION@ for both minVersion and maxVersion. This is what the default theme does: https://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/install.rdf.in#17

Comment on attachment 606170 [details] [diff] [review] pdf.js v.0.2.373 Review of attachment 606170 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/extensions/uriloader@pdf.js/bootstrap.js @@ +75,5 @@ > + Factory.register(PdfStreamConverter); > + Services.prefs.setBoolPref('extensions.pdf.js.active', true); > +} > + > +function shutdown(aData, aReason) { Driveby: There's a bunch of stuff here (like unregistering components) you don't need to do if aReason == APP_SHUTDOWN.

(In reply to Blair McBride (:Unfocused) from comment #68) > Since it'll be shipped included, and only tested on that shipped version of > the app, it might be better to make it compatible only with that specific > version. To do that, run it through the preprocessor, and use > @FIREFOX_VERSION@ for both minVersion and maxVersion. Jesse asked if this would result in an incompatible extension warning when an application update is available - it will not. We special case extensions in the application directory, and assume they will be updated to be compatible when the application update is applied. If you do use @FIREFOX_VERSION@, you should also make the extension opt-in to strict compatibility, by adding the following to the install.rdf: <em:strictCompatibility>true</em:strictCompatibility>

Thanks for driveby-s. > Jesse asked if this would result in an incompatible extension warning when > an application update is available - it will not. We special case extensions > in the application directory, and assume they will be updated to be > compatible when the application update is applied. As I understand, the pdf.js extension will be a part of the Firefox as any other component (e.g. font sanitizer or video codec). The pdf.js might have it's own release/test cycles. Something similar to <em:maxValue>*<em:maxValue> (found it at https://mxr.mozilla.org/mozilla-central/source/testing/mochitest/roboextender/install.rdf) would be much better than <em:maxValue>@FIREFOX_VERSION@<em:maxValue> IMHO. We would probably modify the install.rdf to use @FIREFOX_VERSION@ to be on a par with {972ce4c6-7e08-4474-a285-3208198ce6fd}. However, the sole purpose to do that is to pass the unrelated to pdf.js mochitests (see https://tbpl.mozilla.org/?tree=Try&rev=2226027f826a) and it does not feel right.

(In reply to Yury (:yury) from comment #71) > We would probably modify the install.rdf to use @FIREFOX_VERSION@ to be on a > par with {972ce4c6-7e08-4474-a285-3208198ce6fd}. However, the sole purpose > to do that is to pass the unrelated to pdf.js mochitests (see > https://tbpl.mozilla.org/?tree=Try&rev=2226027f826a) and it does not feel > right. We should fix those tests to ignore pdf.js then, was that not the aim of the patch that Blair reviewed?

(In reply to Dave Townsend (:Mossop) from comment #72) > (In reply to Yury (:yury) from comment #71) > > We would probably modify the install.rdf to use @FIREFOX_VERSION@ to be on a > > par with {972ce4c6-7e08-4474-a285-3208198ce6fd}. However, the sole purpose > > to do that is to pass the unrelated to pdf.js mochitests (see > > https://tbpl.mozilla.org/?tree=Try&rev=2226027f826a) and it does not feel > > right. > > We should fix those tests to ignore pdf.js then, was that not the aim of the > patch that Blair reviewed? Yes and no. Initially, in ff13, only those two were failing, a those two were patches and reviewed by Blair. Once in was uplifted, more other test start failing, which indicated that pdf.js shall have a valid and up-to-date version. Is having "*" as a maxVersion for built-in add-on just a bad practice or an incorrect value?

(In reply to Yury (:yury) from comment #73) > (In reply to Dave Townsend (:Mossop) from comment #72) > > (In reply to Yury (:yury) from comment #71) > > > We would probably modify the install.rdf to use @FIREFOX_VERSION@ to be on a > > > par with {972ce4c6-7e08-4474-a285-3208198ce6fd}. However, the sole purpose > > > to do that is to pass the unrelated to pdf.js mochitests (see > > > https://tbpl.mozilla.org/?tree=Try&rev=2226027f826a) and it does not feel > > > right. > > > > We should fix those tests to ignore pdf.js then, was that not the aim of the > > patch that Blair reviewed? > > Yes and no. Initially, in ff13, only those two were failing, a those two > were patches and reviewed by Blair. Once in was uplifted, more other test > start failing, which indicated that pdf.js shall have a valid and up-to-date > version. > > Is having "*" as a maxVersion for built-in add-on just a bad practice or an > incorrect value? It is something we advocate against for add-on developers (and isn't supported in AMO). We've talked about making it an illegal value in the past but never really got around to it. I'd rather we not use it if we can avoid it unless there is a big issue I'm missing.

Latest try run https://tbpl.mozilla.org/?tree=Try&rev=1020ae7f7a76

Current we addressed: * comment #61 - The code from line 534 onwards seems a little dangerous, as there seems to be variables which take values from the PDF used without escaping * comment #61 - whitelist GET here instead of blacklisting POST * comment #66 - Makefile TEST_DIRS and space indent * comment #68 - use @FIREFOX_VERSION@ for both minVersion and maxVersion * comment #69 - don't need to do (stuff) if aReason == APP_SHUTDOWN * comment #70 - <em:strictCompatibility>true</em:strictCompatibility> Also, we have three patches I'm not sure who set the reviews to: - "pdf.js v.0.2.389" - add-on code - "pdf.js test" - basic testing for the integration - "add built-in PDF support to Firefox with PDF.js" - build scripts for integration of pdf.js and test into mozilla-central (In reply to Ms2ger from comment #66) > > Don't forget to ask a build system peer to review Ms2ger, could you suggest somebody?

Why ship default features as extensions? Doesn't the word "extension" mean a feature that is not shipped with the product by default?

Comment on attachment 606535 [details] [diff] [review] add built-in PDF support to Firefox with PDF.js (applied after tests and pdf.js patches) per conversation with :gavin

Comment on attachment 606785 [details] [diff] [review] pdf.js v.0.2.394 Review of attachment 606785 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/extensions/uriloader@pdf.js/LICENSE @@ +28,5 @@ > + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + DEALINGS IN THE SOFTWARE. I'm no licensing genius but a single license file and no license headers in the source code is not the norm for us. Has somebody that knows better than me (like gerv) confirmed that this is ok? ::: browser/app/profile/extensions/uriloader@pdf.js/bootstrap.js @@ +13,5 @@ > + > +Cu.import('resource://gre/modules/Services.jsm'); > + > +function log(str) { > + dump(str + '\n'); We like to avoid spamming the console by default in stuff released. Can you hide all logging output from pdf.js behind a pref that defaults to false? @@ +64,5 @@ > + .createInstance(Ci.nsILocalFile); > + var componentPath = aData.installPath.clone(); > + componentPath.append('content'); > + aliasFile.initWithPath(componentPath.path); > + var aliasURI = ioService.newFileURI(aliasFile); Why not just pass componentPath here? Even better, you can avoid using nsIFiles altogether with something like: var aliasURI = ioService.newURI("content/", "UTF-8", aData.resourceURI) @@ +72,5 @@ > + pdfStreamConverterUrl = aData.resourceURI.spec + > + 'components/PdfStreamConverter.js'; > + Cu.import(pdfStreamConverterUrl); > + Factory.register(PdfStreamConverter); > + Services.prefs.setBoolPref('extensions.pdf.js.active', true); It's unclear to me why this pref is necessary. Are there cases where you want to disable pdf rendering without disabling the add-on? @@ +88,5 @@ > + resProt.setSubstitution(RESOURCE_NAME, null); > + // Remove the contract/component. > + Factory.unregister(); > + // Unload the converter > + if (pdfStreamConverterUrl) { Not too bothered by this but are there cases where this is actually null? Seems like there shouldn't be. @@ +100,5 @@ > +} > + > +function uninstall(aData, aReason) { > + Services.prefs.clearUserPref('extensions.pdf.js.active'); > + application.prefs.setValue(EXT_PREFIX + '.database', '{}'); application doesn't seem to be defined anywhere ::: browser/app/profile/extensions/uriloader@pdf.js/install.rdf.in @@ +6,5 @@ > + xmlns:em="http://www.mozilla.org/2004/em-rdf#"> > + > + <Description about="urn:mozilla:install-manifest"> > + <em:id>uriloader@pdf.js</em:id> > + <em:name>pdf.js</em:name> I chatted briefly with Asa on IRC, he floated the idea of "PDF Viewer" as a name. He was going to talk to someone on cbeard's team about it, can you please sync up with them to get the name, description and creator fields correct here. For added fun apparently we need this localized. The default theme does that so that should help you out. @@ +8,5 @@ > + <Description about="urn:mozilla:install-manifest"> > + <em:id>uriloader@pdf.js</em:id> > + <em:name>pdf.js</em:name> > + <em:version>0.2.394</em:version> > + <em:iconURL>chrome://pdf.js/skin/logo.png</em:iconURL> You're actually better off just including the icon as icon.png alongside install.rdf (and icon64.png for a 64x64 version) as that then displays even when the extension is disabled. I don't actually see this file in the patch though, nor is there a chrome.manifest to register the chrome url for it. Is this just a mistake? @@ +14,5 @@ > + <Description> > + <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id> > + <em:minVersion>@FIREFOX_VERSION@</em:minVersion> > + <em:maxVersion>@FIREFOX_VERSION@</em:maxVersion> > + <em:strictCompatibility>true</em:strictCompatibility> This is in the wrong place, it needs to be a child of the main Description element. @@ +18,5 @@ > + <em:strictCompatibility>true</em:strictCompatibility> > + </Description> > + </em:targetApplication> > + <em:bootstrap>true</em:bootstrap> > + <em:unpack>true</em:unpack> Why do we need to install this unpacked? @@ +21,5 @@ > + <em:bootstrap>true</em:bootstrap> > + <em:unpack>true</em:unpack> > + <em:creator>Mozilla Labs</em:creator> > + <em:description>pdf.js uri loader</em:description> > + <em:homepageURL>https://github.com/mozilla/pdf.js/</em:homepageURL> I suspect we'll want a better homepage than this, again chat with Asa about it.

Comment on attachment 606535 [details] [diff] [review] add built-in PDF support to Firefox with PDF.js (applied after tests and pdf.js patches) Review of attachment 606535 [details] [diff] [review]: ----------------------------------------------------------------- r- mainly because I'd like to hear the reasons for needing to ship this unpacked before r+ing it. ::: browser/app/profile/extensions/Makefile.in @@ +42,2 @@ > > +DISTROEXT = $(call core_abspath,$(DIST))/bin/distribution/extensions None of the above changes seem necessary ::: browser/app/profile/extensions/installed-extensions.txt @@ +1,1 @@ > +theme,{972ce4c6-7e08-4474-a285-3208198ce6fd},uriloader@pdf.js This file is old and unused, please just delete it rather than patching it. ::: browser/app/profile/extensions/uriloader@pdf.js/Makefile.in @@ +11,5 @@ > + > +include $(DEPTH)/config/autoconf.mk > +include $(topsrcdir)/config/rules.mk > + > +FILES := $(shell cat $(srcdir)/extension-files) Normally we just include the files in the Makefile.in directly, is there a reason not to here, related to how pdf.js is built or something?

CCing gerv re:LICENSE issue above

Legal is on this. Please proceed up to the final point of landing and I will make sure we get the license issue resolved until then.

Product and Branding are on it too. Please don't let us get in the way of landing on m-c. We do need to get finalized strings before the migration to Aurora (for localization,) but not before landing on m-c. I've suggested as placeholder strings: Name: PDF Viewer [version number] Description: Uses HTML5 to display PDF files directly in Firefox. By: Mozilla Homepage: TBD, but something like http://support.mozilla.org/kb/using-mozilla-pdf-viewer

Clarification on string changes after landing on central: If we change strings, we'll need to rev the string names, too, also on mozilla-central.

The current Mozilla license policy: http://www.mozilla.org/MPL/license-policy.html states that code that originated with Mozilla that becomes part of the core products needs to be MPL 2.0. I plan to have a discussion about whether the Apache License 2.0, as a non-copyleft license, should be added to that list, but I have not yet managed to get sufficient bandwidth from Mitchell to start it. Several of the Mozilla legal team feel that, for a number of reasons, we should not be shipping code using licenses without patent protection clauses, such as MIT or BSD. (This particularly applies to B2G and Gaia, and there is a bug open on sorting that out too.) So, as I see it, we have the following options: 1) Switch to MPL 2.0 2) If a license with no copyleft at all is desirable for some reason, either: 2a) get the Apache discussion started and concluded and, if the result is to allow Apache, switch to Apache; or 2b) Try and convince the legal team that MIT is OK in this case. Gerv

(In reply to Axel Hecht [:Pike] from comment #86) > Clarification on string changes after landing on central: If we change > strings, we'll need to rev the string names, too, also on mozilla-central. If we know we're going to change the strings after landing does it make sense to just not localize them at first to save people wasting time translating them or does that involve peril I don't know about?

Yeah, keeping the strings internal 'til we got it figured out would help. You can put the strings in separate files and just package and reference them in the content package 'til they're finished, and then move them to locales/en-US files.

It might be off-topic for this bug, but if our current system doesn't have a tree where developers can check in strings without the l10n community jumping all over them and then complaining when they later change (or requiring an ugly "2" to be added to the string name), then that seems a bit wrong. I would expect mozilla-central to be that tree, and strings to be frozen once we uplift to aurora. And that seems to be what this says, too: http://blog.mozilla.com/axel/2011/04/11/being-a-localizer-in-the-rapid-release-cycle/ The risk of being an "Early Bird" in this model is that strings might change underneath you. If you don't like that, don't be an Early Bird. Gerv

(In reply to Axel Hecht [:Pike] from comment #89) > Yeah, keeping the strings internal 'til we got it figured out would help. > You can put the strings in separate files and just package and reference > them in the content package 'til they're finished, and then move them to > locales/en-US files. Ok then we should just put Asa's suggestions into install.rdf and file a spin-off bug to localize the name/description and come up with the final strings.

(In reply to Gervase Markham [:gerv] from comment #90) IMHO we should not have trees where localizers don't complain about strings being wrong, nor should we have trees without a messaging scheme on when localizers need to take action. And yes, that's off-topic for this bug.

(addressed by pull request https://github.com/mozilla/pdf.js/pull/1373) (In reply to Dave Townsend (:Mossop) from comment #81) > Comment on attachment 606785 [details] [diff] [review] > pdf.js v.0.2.394 > > Review of attachment 606785 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: browser/app/profile/extensions/uriloader@pdf.js/bootstrap.js > @@ +13,5 @@ > > + > > +Cu.import('resource://gre/modules/Services.jsm'); > > + > > +function log(str) { > > + dump(str + '\n'); > > We like to avoid spamming the console by default in stuff released. Can you > hide all logging output from pdf.js behind a pref that defaults to false? Existing extensions.uriloader@pdf.js.pdfBugEnabled is used to suppress the log output from the pdf.js. > > @@ +64,5 @@ > > + .createInstance(Ci.nsILocalFile); > > + var componentPath = aData.installPath.clone(); > > + componentPath.append('content'); > > + aliasFile.initWithPath(componentPath.path); > > + var aliasURI = ioService.newFileURI(aliasFile); > > Why not just pass componentPath here? Even better, you can avoid using > nsIFiles altogether with something like: > > var aliasURI = ioService.newURI("content/", "UTF-8", aData.resourceURI) Fixed by using aData.resourceURI > > @@ +72,5 @@ > > + pdfStreamConverterUrl = aData.resourceURI.spec + > > + 'components/PdfStreamConverter.js'; > > + Cu.import(pdfStreamConverterUrl); > > + Factory.register(PdfStreamConverter); > > + Services.prefs.setBoolPref('extensions.pdf.js.active', true); > > It's unclear to me why this pref is necessary. Are there cases where you > want to disable pdf rendering without disabling the add-on? > extensions.pdf.js.active sets and gets are removed. > @@ +88,5 @@ > > + resProt.setSubstitution(RESOURCE_NAME, null); > > + // Remove the contract/component. > > + Factory.unregister(); > > + // Unload the converter > > + if (pdfStreamConverterUrl) { > > Not too bothered by this but are there cases where this is actually null? > Seems like there shouldn't be. > The if-check is removed. > @@ +100,5 @@ > > +} > > + > > +function uninstall(aData, aReason) { > > + Services.prefs.clearUserPref('extensions.pdf.js.active'); > > + application.prefs.setValue(EXT_PREFIX + '.database', '{}'); > > application doesn't seem to be defined anywhere > application is defined now. > ::: browser/app/profile/extensions/uriloader@pdf.js/install.rdf.in > @@ +6,5 @@ > > + xmlns:em="http://www.mozilla.org/2004/em-rdf#"> > > + > > + <Description about="urn:mozilla:install-manifest"> > > + <em:id>uriloader@pdf.js</em:id> > > + <em:name>pdf.js</em:name> > > I chatted briefly with Asa on IRC, he floated the idea of "PDF Viewer" as a > name. Name is set to "PDF Viewer" now. > @@ +8,5 @@ > > + <Description about="urn:mozilla:install-manifest"> > > + <em:id>uriloader@pdf.js</em:id> > > + <em:name>pdf.js</em:name> > > + <em:version>0.2.394</em:version> > > + <em:iconURL>chrome://pdf.js/skin/logo.png</em:iconURL> > > You're actually better off just including the icon as icon.png alongside > install.rdf (and icon64.png for a 64x64 version) as that then displays even > when the extension is disabled. icon.png and icon64.png is included in the package > I don't actually see this file in the patch > though, nor is there a chrome.manifest to register the chrome url for it. Is > this just a mistake? > The reference to the logo.png is remove. > @@ +14,5 @@ > > + <Description> > > + <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id> > > + <em:minVersion>@FIREFOX_VERSION@</em:minVersion> > > + <em:maxVersion>@FIREFOX_VERSION@</em:maxVersion> > > + <em:strictCompatibility>true</em:strictCompatibility> > > This is in the wrong place, it needs to be a child of the main Description > element. Fixed. > > @@ +18,5 @@ > > + <em:strictCompatibility>true</em:strictCompatibility> > > + </Description> > > + </em:targetApplication> > > + <em:bootstrap>true</em:bootstrap> > > + <em:unpack>true</em:unpack> > > Why do we need to install this unpacked? > You are right; there is no reason to have it unpacked. (There was a concern about accessing the files over the "resource" protocol, but was not a valid one). Fixed. The integration patch will package the files into the xpi file. > @@ +21,5 @@ > > + <em:bootstrap>true</em:bootstrap> > > + <em:unpack>true</em:unpack> > > + <em:creator>Mozilla Labs</em:creator> > > + <em:description>pdf.js uri loader</em:description> > > + <em:homepageURL>https://github.com/mozilla/pdf.js/</em:homepageURL> > > I suspect we'll want a better homepage than this, again chat with Asa about > it. Homepage is set to "http://support.mozilla.org/kb/using-mozilla-pdf-viewer"

(In reply to Dave Townsend (:Mossop) from comment #82) > Comment on attachment 606535 [details] [diff] [review] > add built-in PDF support to Firefox with PDF.js (applied after tests and > pdf.js patches) > > Review of attachment 606535 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- mainly because I'd like to hear the reasons for needing to ship this > unpacked before r+ing it. > See comment #93. This patch packages the files into the xpi. We are not sure if the "install::" section is correct though -- hard to find a sample. > ::: browser/app/profile/extensions/Makefile.in > @@ +42,2 @@ > > > > +DISTROEXT = $(call core_abspath,$(DIST))/bin/distribution/extensions > > None of the above changes seem necessary > The change is reverted. > ::: browser/app/profile/extensions/installed-extensions.txt > @@ +1,1 @@ > > +theme,{972ce4c6-7e08-4474-a285-3208198ce6fd},uriloader@pdf.js > > This file is old and unused, please just delete it rather than patching it. > The patch will not be touching this file. Do you want to include a separate patch that removes the installed-extensions.txt file? > ::: browser/app/profile/extensions/uriloader@pdf.js/Makefile.in > @@ +11,5 @@ > > + > > +include $(DEPTH)/config/autoconf.mk > > +include $(topsrcdir)/config/rules.mk > > + > > +FILES := $(shell cat $(srcdir)/extension-files) > > Normally we just include the files in the Makefile.in directly, is there a > reason not to here, related to how pdf.js is built or something? Using the extension-files as an extension files whitelist to avoid noise in the xpi file such as MOZILLA.readme, install.pdf.in and this make file. Also, that will be better from the support side. The pdf.js code will be test and pushed from github repositort. In case if Makefile has to be modified by the m-c developers, the pdf.js team will not be a blocker. Shall we combine Makefile and extension-files files, or keep them separate? (last try run https://tbpl.mozilla.org/?tree=Try&rev=21df4d4e89a5)

Comment on attachment 607848 [details] [diff] [review] add built-in PDF support to Firefox with PDF.js (applied after tests and pdf.js patches) Review of attachment 607848 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/extensions/uriloader@pdf.js/Makefile.in @@ +6,5 @@ > +topsrcdir = @top_srcdir@ > +srcdir = @srcdir@ > +VPATH = @srcdir@ > + > +DISTROEXT = $(call core_abspath,$(DIST))/bin/extensions To save confusion can you call this APPEXT instead. @@ +24,5 @@ > + cd $(call core_abspath,$(srcdir)) && \ > + $(ZIP) -9X $(DISTROEXT)/uriloader@pdf.js.xpi $(FILES) > + > +install:: > + $(SYSINSTALL) $(DISTROEXT)/uriloader@pdf.js.xpi $(DESTDIR)$(mozappdir)/extensions I'm not even sure we need install sections these days but based on other examples I think you should put $(IFLAGS1) before the file.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1c93e7f34f https://hg.mozilla.org/integration/mozilla-inbound/rev/4208fe045a06 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c784e5f33b9 https://hg.mozilla.org/integration/mozilla-inbound/rev/e95e2d36dbe1

I think this is going to get caught by our add-on blocking code causing everyone to get a tab asking them if they want to enable this when they start the first nightly that includes it.

https://hg.mozilla.org/mozilla-central/rev/fa1c93e7f34f https://hg.mozilla.org/mozilla-central/rev/4208fe045a06 https://hg.mozilla.org/mozilla-central/rev/0c784e5f33b9 https://hg.mozilla.org/mozilla-central/rev/e95e2d36dbe1

(In reply to Dave Townsend (:Mossop) from comment #101) > I think this is going to get caught by our add-on blocking code causing > everyone to get a tab asking them if they want to enable this when they > start the first nightly that includes it. Bah, yes. Just double checked to confirm. (And it's past 3am, so I'm not thinking of a solution right now.)

FYI, the URL http://support.mozilla.org/en-US/kb/using-mozilla-pdf-viewer is 404 when clicked on from in the Addon Manager. Going direct to link also is 404.

Can the extension live in /extensions instead of browser/ ? I suspect that Thunderbird and SeaMonkey would want that. In fact I remember TB planning to ship pdfjs so not having multiple copies in the tree would be better.

I'd like to suggest that it live in <toplevel>/pdf. We want less hierarchy in general.

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #104) > FYI, the URL http://support.mozilla.org/en-US/kb/using-mozilla-pdf-viewer is > 404 when clicked on from in the Addon Manager. Going direct to link also is > 404. I'm following up with David Tenser et al to figure out how to fix that.

(In reply to Blair McBride (:Unfocused) from comment #103) > (In reply to Dave Townsend (:Mossop) from comment #101) > > I think this is going to get caught by our add-on blocking code causing > > everyone to get a tab asking them if they want to enable this when they > > start the first nightly that includes it. > > Bah, yes. Just double checked to confirm. > (And it's past 3am, so I'm not thinking of a solution right now.) We could disable third-party blocking for the app dir (but I know we explicitly wanted that for some add-on that was getting injected there). We could ship this as a distribution add-on, but then nightly testers wouldn't get it and it changes the behaviour somewhat. Or we don't ship this as an extension.

(In reply to Dave Townsend (:Mossop) from comment #108) > (In reply to Blair McBride (:Unfocused) from comment #103) > > (In reply to Dave Townsend (:Mossop) from comment #101) > > > I think this is going to get caught by our add-on blocking code causing > > > everyone to get a tab asking them if they want to enable this when they > > > start the first nightly that includes it. > > > > Bah, yes. Just double checked to confirm. > > (And it's past 3am, so I'm not thinking of a solution right now.) > > We could disable third-party blocking for the app dir (but I know we > explicitly wanted that for some add-on that was getting injected there). > We could ship this as a distribution add-on, but then nightly testers > wouldn't get it and it changes the behaviour somewhat. > Or we don't ship this as an extension. If we didn't ship this as an add-on what would be the preferred way to include it? Something like the RSS reader? I'm hoping we don't have to do this as it would be require quite a bit of change at this point.

(In reply to Brendan Dahl from comment #109) > (In reply to Dave Townsend (:Mossop) from comment #108) > > (In reply to Blair McBride (:Unfocused) from comment #103) > > > (In reply to Dave Townsend (:Mossop) from comment #101) > > > > I think this is going to get caught by our add-on blocking code causing > > > > everyone to get a tab asking them if they want to enable this when they > > > > start the first nightly that includes it. > > > > > > Bah, yes. Just double checked to confirm. > > > (And it's past 3am, so I'm not thinking of a solution right now.) > > > > We could disable third-party blocking for the app dir (but I know we > > explicitly wanted that for some add-on that was getting injected there). > > We could ship this as a distribution add-on, but then nightly testers > > wouldn't get it and it changes the behaviour somewhat. > > Or we don't ship this as an extension. > > If we didn't ship this as an add-on what would be the preferred way to > include it? Something like the RSS reader? I'm hoping we don't have to do > this as it would be require quite a bit of change at this point. Two options, both pretty straightforward I think: Either add a chrome.manifest file to do the component registration exactly as if it were a non-restartless add-on, then we ship it in <appdir>/distribution/bundles and it gets loaded exactly as an extension, just not shown in the extension manager. For restartless add-ons right now that method doesn't work (though maybe it should!) so the alternative is to just have to package the files somewhere sensible in the appdir and then write some hook code in Firefox to load bootstrap.js and call its startup method during startup. Of course the downside to both of these is that pdf.js no longer appears as an add-on so there is no default UI for disabling it.

(In reply to Dave Townsend (:Mossop) from comment #110) > (In reply to Brendan Dahl from comment #109) > > (In reply to Dave Townsend (:Mossop) from comment #108) > > > (In reply to Blair McBride (:Unfocused) from comment #103) > > > > (In reply to Dave Townsend (:Mossop) from comment #101) > > > > > I think this is going to get caught by our add-on blocking code causing > > > > > everyone to get a tab asking them if they want to enable this when they > > > > > start the first nightly that includes it. > > > > > > > > Bah, yes. Just double checked to confirm. > > > > (And it's past 3am, so I'm not thinking of a solution right now.) > > > > > > We could disable third-party blocking for the app dir (but I know we > > > explicitly wanted that for some add-on that was getting injected there). > > > We could ship this as a distribution add-on, but then nightly testers > > > wouldn't get it and it changes the behaviour somewhat. > > > Or we don't ship this as an extension. > > > > If we didn't ship this as an add-on what would be the preferred way to > > include it? Something like the RSS reader? I'm hoping we don't have to do > > this as it would be require quite a bit of change at this point. > > Two options, both pretty straightforward I think: > > Either add a chrome.manifest file to do the component registration exactly > as if it were a non-restartless add-on, then we ship it in > <appdir>/distribution/bundles and it gets loaded exactly as an extension, > just not shown in the extension manager. > > For restartless add-ons right now that method doesn't work (though maybe it > should!) so the alternative is to just have to package the files somewhere > sensible in the appdir and then write some hook code in Firefox to load > bootstrap.js and call its startup method during startup. > > Of course the downside to both of these is that pdf.js no longer appears as > an add-on so there is no default UI for disabling it. Oh another downside here is that this might conflict if the user already has the pdf.js extension installed

(In reply to Dave Townsend (:Mossop) from comment #111) > > Of course the downside to both of these is that pdf.js no longer appears as > > an add-on so there is no default UI for disabling it. > > Oh another downside here is that this might conflict if the user already has > the pdf.js extension installed For the record, AMO says we have around 12,000 average daily users of the extension as of today: https://addons.mozilla.org/en-US/firefox/addon/pdfjs/statistics/?last=30

(In reply to Bill Walker [:bwalker] [@wfwalker] from comment #112) > (In reply to Dave Townsend (:Mossop) from comment #111) > > > > Of course the downside to both of these is that pdf.js no longer appears as > > > an add-on so there is no default UI for disabling it. > > > > Oh another downside here is that this might conflict if the user already has > > the pdf.js extension installed > > For the record, AMO says we have around 12,000 average daily users of the > extension as of today: > https://addons.mozilla.org/en-US/firefox/addon/pdfjs/statistics/?last=30 When I say "conflict" I mean I don't really know what the result will be. If I had to guess I'd say that it'd work but not sure which of them (the shipped code and the extension code) would actually be working. We could probably make the extension not register itself (even uninstall itself) if Firefox includes pdf.js, or vice versa.

(In reply to Philip Chee from comment #105) > Can the extension live in /extensions instead of browser/ ? I suspect that > Thunderbird and SeaMonkey would want that. In fact I remember TB planning to > ship pdfjs so not having multiple copies in the tree would be better. Making this an opt-in part of Gecko involves a bunch of build and coordination work that I don't think we want to spend time doing at this point. If someone wants to do that work at some later point we can do it, but for now the focus is on getting this into Firefox. We can easily shuffle things around later, once the primary goal of getting this into Firefox has been achieved.

Is the code in pdf.js (not the bootstrap code) running with chrome or content privileges?

> Is the code in pdf.js (not the bootstrap code) running with chrome or content privileges? All the core pdf.js code is unprivileged. Few utility functions, such as download, get/set settings is in the chrome code: https://github.com/mozilla/pdf.js/blob/master/extensions/firefox/components/PdfStreamConverter.js#L52 the communication with chrome code done here: https://github.com/mozilla/pdf.js/blob/master/web/viewer.js#L60

(In reply to Dave Townsend (:Mossop) from comment #113) > We could > probably make the extension not register itself (even uninstall itself) if > Firefox includes pdf.js, or vice versa. Does the change make it impossible to test the latest development version of pdf.js?

(In reply to Masatoshi Kimura [:emk] from comment #117) > (In reply to Dave Townsend (:Mossop) from comment #113) > > We could > > probably make the extension not register itself (even uninstall itself) if > > Firefox includes pdf.js, or vice versa. > Does the change make it impossible to test the latest development version of > pdf.js? One of those options would yes

Is there any chance that Stephen Horlander takes a look at this. Because the user interface looks very old.

(In reply to Dave Townsend from comment #108) > We could ship this as a distribution add-on, but then nightly testers > wouldn't get it and it changes the behaviour somewhat. This is what was recommended for SeaMonkey's bundled extensions; if it's not good enough then should we be switching to some other scheme?

(In reply to neil@parkwaycc.co.uk from comment #120) > (In reply to Dave Townsend from comment #108) > > We could ship this as a distribution add-on, but then nightly testers > > wouldn't get it and it changes the behaviour somewhat. > This is what was recommended for SeaMonkey's bundled extensions; if it's not > good enough then should we be switching to some other scheme? You should use it if it works for you, but it has a different behaviour, notably users can uninstall the add-on and wouldn't have a way to get it back, and Nightly testers wouldn't even see the add-on at first, nor would it be easy to update it in nightly builds since we only check distribution add-ons when the Firefox version changes.

It was very odd to see the tab show up talking about PDF.js as I had not been opening a PDF. What triggers the tab to show up? Could we install without user interaction?

(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #122) > It was very odd to see the tab show up talking about PDF.js as I had not > been opening a PDF. What triggers the tab to show up? > Could we install without user interaction? See bug 738674

Using Aurora on Linux, i have PDF Viewer as add-on from today. Looking in the add-on pane, and clicking "More", there's a dead link (https://support.mozilla.org/fr/kb/using-mozilla-pdf-viewer)

Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120324 Firefox/14.0a1 How does one test this? The extension is enabled but PDFs bring up the "What should Nightly do with this file?" dialogue as before (confirmed in usual profile and a clean profile). I couldn't see a pref to enable it in about:config.

(In reply to Daniel Cater from comment #125) > Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120324 Firefox/14.0a1 > > How does one test this? The extension is enabled but PDFs bring up the "What > should Nightly do with this file?" dialogue as before (confirmed in usual > profile and a clean profile). I couldn't see a pref to enable it in > about:config. It sometimes happens after firefox crashes. Disabling/re-enabling the addon helps, at least in my case.

Very nice! :) Bug: When zooming with CTRL + mouse-wheel (or CTRL + PLUS), the page zooms, but the text get blurry (probably just the page image is resized, but not re-rendered). If enabling "Zoom text only", then only the toolbar text zooms (and breaks), and the page doesn't. The +/- buttons at the top work as expected and re-render the page at a different zoom, with sharp text. (tested with http://embedded.arch.ethz.ch/seeduino.pdf)

A small bug: Clicking "Download" button if a pdf file is loaded results in downloading it again. To my mind such behavior is waste of time for a user who wants one way or another to get the file as quickly as possible. I propose replacing "Download" with "Save" when pdf is fully loaded and saving it by extracting from cache.

Verified fixed using Windows 7 and the latest Nightly.