Last Comment Bug 714712 - add built-in PDF support to Firefox with PDF.js
: add built-in PDF support to Firefox with PDF.js
Status: RESOLVED FIXED
[secr:ptheriault][sg:want] [testday-2...
:
Product: Firefox
Classification: Client Software
Component: PDF Viewer (show other bugs)
: 12 Branch
: All All
: -- enhancement with 12 votes (vote)
: Firefox 14
Assigned To: Yury Delendik (:yury)
:
:
Mentors:
https://github.com/mozilla/pdf.js
: 344620 455917 456148 (view as bug list)
Depends on: 738947 739043 755957 770439 787674 815558 734794 738287 738344 738674 738858 738952 738953 738967 740795 741239 742000 742101 743252 752676 760746 760997 763086 766131 773387 800542
Blocks: FF2SM 737347 743264
  Show dependency treegraph
 
Reported: 2012-01-02 21:06 PST by Josh Aas
Modified: 2016-06-16 06:09 PDT (History)
83 users (show)
dveditz: sec‑review+
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adding pdf.js to the mozilla-central (2.63 MB, patch)
2012-02-06 15:25 PST, Yury Delendik (:yury)
myk: feedback+
Details | Diff | Splinter Review
Adding pdf.js to the mozilla-central (without pdf.js) (5.35 KB, patch)
2012-02-07 16:24 PST, Yury Delendik (:yury)
21: feedback+
Details | Diff | Splinter Review
pdf.js v0.2.193 (2.63 MB, patch)
2012-02-07 16:40 PST, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Adding pdf.js to the mozilla-central (without pdf.js) (4.34 KB, patch)
2012-02-08 09:55 PST, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Adding pdf.js to the mozilla-central (without pdf.js) (1.01 MB, patch)
2012-02-09 20:51 PST, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
595250: pdf.js v0.2.213 (2.64 MB, patch)
2012-02-09 20:52 PST, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
pdf.js v0.2.324 (2.72 MB, patch)
2012-02-23 20:21 PST, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Adding pdf.js to the mozilla-central (without pdf.js) (14.84 KB, patch)
2012-02-23 20:23 PST, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Adds PDF content type to Mochitest's server (1.17 KB, patch)
2012-03-13 12:52 PDT, Artur Adib
no flags Details | Diff | Splinter Review
pdf.js version 0.2.364 (2.73 MB, patch)
2012-03-13 20:34 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
pdf.js test (12.39 KB, patch)
2012-03-13 20:36 PDT, Yury Delendik (:yury)
gavin.sharp: review+
Details | Diff | Splinter Review
add built-in PDF support to Firefox with PDF.js (applied after test and pdf.js patches) (3.85 KB, patch)
2012-03-13 20:38 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Fixes browser_select_update.js and browser_select_confirm.js (2.44 KB, patch)
2012-03-14 18:30 PDT, Yury Delendik (:yury)
blair: review+
Details | Diff | Splinter Review
pdf.js v.0.2.373 (2.73 MB, patch)
2012-03-15 05:09 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
pdf.js v.0.2.389 (2.73 MB, patch)
2012-03-16 04:57 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
add built-in PDF support to Firefox with PDF.js (applied after tests and pdf.js patches) (4.18 KB, patch)
2012-03-16 05:02 PDT, Yury Delendik (:yury)
dtownsend: review-
Details | Diff | Splinter Review
pdf.js v.0.2.394 (2.73 MB, patch)
2012-03-16 16:38 PDT, Yury Delendik (:yury)
dtownsend: review-
Details | Diff | Splinter Review
pdf.js v.0.2.414 (2.74 MB, patch)
2012-03-20 21:05 PDT, Yury Delendik (:yury)
dtownsend: review+
Details | Diff | Splinter Review
add built-in PDF support to Firefox with PDF.js (applied after tests and pdf.js patches) (3.53 KB, patch)
2012-03-20 21:20 PDT, Yury Delendik (:yury)
dtownsend: review+
Details | Diff | Splinter Review
patch for checkin (1/4) (2.74 MB, patch)
2012-03-21 18:57 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
patch for checkin (2/4) (12.40 KB, patch)
2012-03-21 19:07 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
patch for checkin (3/4) (2.46 KB, patch)
2012-03-21 19:08 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
patch for checkin (4/4) (3.48 KB, patch)
2012-03-21 19:15 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review

Description Josh Aas 2012-01-02 21:06:53 PST
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.
Comment 1 Brendan Dahl [:bdahl] 2012-01-03 09:32:16 PST
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
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-03 13:51:24 PST
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.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-03 13:53:33 PST
(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.
Comment 4 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-04 03:34:14 PST
(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?
Comment 5 Artur Adib 2012-01-04 08:29:10 PST
> 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).
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-04 13:53:02 PST
(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.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-04 14:45:41 PST
<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.
Comment 8 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-06 03:50:57 PST
(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?
Comment 9 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-06 03:53:38 PST
(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.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-06 04:27:23 PST
(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.
Comment 11 Alexandre Poirot [:ochameau] 2012-01-06 04:34:15 PST
(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?
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-06 04:36:15 PST
(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.
Comment 13 Artur Adib 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.

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?
Comment 14 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-06 06:40:04 PST
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)?
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 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 :).

(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.
Comment 16 Justin Dolske [:Dolske] 2012-01-09 02:55:16 PST
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)?
Comment 17 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-09 06:12:10 PST
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!)
Comment 18 Yury Delendik (:yury) 2012-02-06 15:25:33 PST
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
Comment 19 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-02-07 04:58:47 PST
(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 20 Myk Melez [:myk] [@mykmelez] 2012-02-07 14:43:08 PST
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.
Comment 21 Yury Delendik (:yury) 2012-02-07 16:24:19 PST
Created attachment 595244 [details] [diff] [review]
Adding pdf.js to the mozilla-central (without pdf.js)
Comment 22 Yury Delendik (:yury) 2012-02-07 16:40:28 PST
Created attachment 595250 [details] [diff] [review]
pdf.js v0.2.193
Comment 23 Yury Delendik (:yury) 2012-02-07 16:49:05 PST
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 24 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-02-08 03:39:25 PST
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 25 Ted Mielczarek [:ted.mielczarek] 2012-02-08 05:07:55 PST
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 $^.)
Comment 26 Yury Delendik (:yury) 2012-02-08 09:55:36 PST
Created attachment 595440 [details] [diff] [review]
Adding pdf.js to the mozilla-central (without pdf.js)

(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?
Comment 27 Ted Mielczarek [:ted.mielczarek] 2012-02-08 10:05:57 PST
Oh, in that case you might just want to use $(DIR_INSTALL).
Comment 28 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-02-09 01:53:25 PST
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.
Comment 29 Yury Delendik (:yury) 2012-02-09 20:51:40 PST
Created attachment 595954 [details] [diff] [review]
Adding pdf.js to the mozilla-central (without pdf.js)
Comment 30 Yury Delendik (:yury) 2012-02-09 20:52:46 PST
Created attachment 595955 [details] [diff] [review]
595250: pdf.js v0.2.213
Comment 31 Yury Delendik (:yury) 2012-02-09 20:57:41 PST
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
Comment 32 Guillaume C. [:ge3k0s] 2012-02-10 13:00:54 PST
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).
Comment 33 Julian Viereck 2012-02-12 23:30:01 PST
(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.
Comment 34 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-13 11:10:09 PST
I don't know.
Comment 35 Axel Hecht [:Pike] 2012-02-13 11:55:40 PST
(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.
Comment 36 alexander :surkov 2012-02-13 15:49:27 PST
(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.
Comment 37 Justin Dolske [:Dolske] 2012-02-15 23:04:57 PST
(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.
Comment 38 alexander :surkov 2012-02-16 00:54:36 PST
(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.
Comment 39 Artur Adib 2012-02-16 06:56:11 PST
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
Comment 40 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-02-22 14:28:01 PST
assigning to paul to review the code
Comment 41 Justin Dolske [:Dolske] 2012-02-23 00:55:23 PST
(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.
Comment 42 alexander :surkov 2012-02-23 19:00:38 PST
(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.
Comment 43 Yury Delendik (:yury) 2012-02-23 20:21:36 PST
Created attachment 600295 [details] [diff] [review]
pdf.js v0.2.324
Comment 44 Yury Delendik (:yury) 2012-02-23 20:23:25 PST
Created attachment 600296 [details] [diff] [review]
Adding pdf.js to the mozilla-central (without pdf.js)

Updating to current m-c. Try run:
https://tbpl.mozilla.org/?tree=Try&rev=4afde1727f68
Comment 45 Artur Adib 2012-02-29 10:25:39 PST
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 :)
Comment 46 Josh Matthews [:jdm] 2012-02-29 11:08:26 PST
You want SimpleTest.finish() instead of finish().
Comment 47 Artur Adib 2012-02-29 11:27:51 PST
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?
Comment 48 Josh Matthews [:jdm] 2012-02-29 12:19:15 PST
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.
Comment 49 cmtalbert 2012-03-07 19:36:34 PST
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.
Comment 50 Andreas Gal :gal 2012-03-13 07:02:47 PDT
When do we expect this to land?
Comment 51 Artur Adib 2012-03-13 08:46:38 PDT
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?
Comment 52 Josh Matthews [:jdm] 2012-03-13 09:26:05 PDT
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
Comment 53 Ted Mielczarek [:ted.mielczarek] 2012-03-13 09:36:06 PDT
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.
Comment 54 Artur Adib 2012-03-13 12:50:50 PDT
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.
Comment 55 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-13 12:52:07 PDT
There's no copying involved on OS X, the relevant files are symlinks.  That's not the case on Windows.
Comment 56 Artur Adib 2012-03-13 12:52:20 PDT
Created attachment 605498 [details] [diff] [review]
Adds PDF content type to Mochitest's server
Comment 57 Ted Mielczarek [:ted.mielczarek] 2012-03-13 12:53:30 PDT
You can run make in $objdir/testing/mochitest to get the changes to take effect on Windows.
Comment 58 Yury Delendik (:yury) 2012-03-13 20:34:53 PDT
Created attachment 605644 [details] [diff] [review]
pdf.js version 0.2.364
Comment 59 Yury Delendik (:yury) 2012-03-13 20:36:37 PDT
Created attachment 605645 [details] [diff] [review]
pdf.js test
Comment 60 Yury Delendik (:yury) 2012-03-13 20:38:42 PDT
Created attachment 605646 [details] [diff] [review]
add built-in PDF support to Firefox with PDF.js (applied after test and pdf.js patches)

Latest try run https://tbpl.mozilla.org/?tree=Try&rev=2226027f826a
Comment 61 Paul Theriault [:pauljt] 2012-03-14 05:38:36 PDT
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?
Comment 62 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-14 11:30:41 PDT
(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.
Comment 63 Yury Delendik (:yury) 2012-03-14 17:00:44 PDT
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
Comment 64 Yury Delendik (:yury) 2012-03-14 18:30:57 PDT
Created attachment 606050 [details] [diff] [review]
Fixes browser_select_update.js and browser_select_confirm.js

The mochitests fail due to browser_select_update.js and browser_select_confirm.js test logic. Sending for review to the tests author
Comment 65 Yury Delendik (:yury) 2012-03-15 05:09:35 PDT
Created attachment 606170 [details] [diff] [review]
pdf.js v.0.2.373

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 66 :Ms2ger (⌚ UTC+1/+2) 2012-03-15 05:38:21 PDT
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?
Comment 67 Artur Adib 2012-03-15 06:26:11 PDT
> 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?
Comment 68 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-15 17:00:37 PDT
(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 69 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-15 17:06:23 PDT
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.
Comment 70 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-15 17:50:06 PDT
(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>
Comment 71 Yury Delendik (:yury) 2012-03-15 18:52:47 PDT
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.
Comment 72 Dave Townsend [:mossop] 2012-03-15 19:04:21 PDT
(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?
Comment 73 Yury Delendik (:yury) 2012-03-15 19:18:01 PDT
(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?
Comment 74 Dave Townsend [:mossop] 2012-03-15 19:33:40 PDT
(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.
Comment 75 Yury Delendik (:yury) 2012-03-16 04:57:59 PDT
Created attachment 606534 [details] [diff] [review]
pdf.js v.0.2.389
Comment 76 Yury Delendik (:yury) 2012-03-16 05:02:27 PDT
Created attachment 606535 [details] [diff] [review]
add built-in PDF support to Firefox with PDF.js (applied after tests and pdf.js patches)

Latest try run https://tbpl.mozilla.org/?tree=Try&rev=1020ae7f7a76
Comment 77 Yury Delendik (:yury) 2012-03-16 05:21:15 PDT
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?
Comment 78 JK 2012-03-16 05:56:55 PDT
Why ship default features as extensions? Doesn't the word "extension" mean a feature that is not shipped with the product by default?
Comment 79 Yury Delendik (:yury) 2012-03-16 16:38:24 PDT
Created attachment 606785 [details] [diff] [review]
pdf.js v.0.2.394
Comment 80 Yury Delendik (:yury) 2012-03-16 16:42:38 PDT
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 81 Dave Townsend [:mossop] 2012-03-19 16:31:03 PDT
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 82 Dave Townsend [:mossop] 2012-03-19 16:31:47 PDT
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?
Comment 83 Artur Adib 2012-03-19 17:18:36 PDT
CCing gerv re:LICENSE issue above
Comment 84 Andreas Gal :gal 2012-03-19 17:51:24 PDT
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.
Comment 85 Asa Dotzler [:asa] 2012-03-19 18:04:01 PDT
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
Comment 86 Axel Hecht [:Pike] 2012-03-20 01:19:05 PDT
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.
Comment 87 Gervase Markham [:gerv] 2012-03-20 04:05:30 PDT
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
Comment 88 Dave Townsend [:mossop] 2012-03-20 09:58:06 PDT
(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?
Comment 89 Axel Hecht [:Pike] 2012-03-20 10:03:38 PDT
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.
Comment 90 Gervase Markham [:gerv] 2012-03-20 10:16:11 PDT
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
Comment 91 Dave Townsend [:mossop] 2012-03-20 10:29:30 PDT
(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.
Comment 92 Axel Hecht [:Pike] 2012-03-20 10:43:10 PDT
(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.
Comment 93 Yury Delendik (:yury) 2012-03-20 21:05:08 PDT
Created attachment 607846 [details] [diff] [review]
pdf.js v.0.2.414

(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"
Comment 94 Yury Delendik (:yury) 2012-03-20 21:20:35 PDT
Created attachment 607848 [details] [diff] [review]
add built-in PDF support to Firefox with PDF.js (applied after tests and pdf.js patches)

(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 95 Dave Townsend [:mossop] 2012-03-21 10:45:57 PDT
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.
Comment 96 Yury Delendik (:yury) 2012-03-21 18:57:59 PDT
Created attachment 608181 [details] [diff] [review]
patch for checkin (1/4)
Comment 97 Yury Delendik (:yury) 2012-03-21 19:07:13 PDT
Created attachment 608184 [details] [diff] [review]
patch for checkin (2/4)
Comment 98 Yury Delendik (:yury) 2012-03-21 19:08:53 PDT
Created attachment 608185 [details] [diff] [review]
patch for checkin (3/4)
Comment 99 Yury Delendik (:yury) 2012-03-21 19:15:06 PDT
Created attachment 608189 [details] [diff] [review]
patch for checkin (4/4)
Comment 101 Dave Townsend [:mossop] 2012-03-21 22:56:28 PDT
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.
Comment 103 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-22 07:22:24 PDT
(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.)
Comment 104 Jim Jeffery not reading bug-mail 1/2/11 2012-03-22 09:15:46 PDT
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.
Comment 105 Philip Chee 2012-03-22 09:25:30 PDT
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.
Comment 106 Benjamin Smedberg [:bsmedberg] 2012-03-22 09:31:40 PDT
I'd like to suggest that it live in <toplevel>/pdf. We want less hierarchy in general.
Comment 107 Bill Walker [:bwalker] [@wfwalker] 2012-03-22 09:36:53 PDT
(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.
Comment 108 Dave Townsend [:mossop] 2012-03-22 09:43:56 PDT
(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.
Comment 109 Brendan Dahl [:bdahl] 2012-03-22 09:55:41 PDT
(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.
Comment 110 Dave Townsend [:mossop] 2012-03-22 10:09:19 PDT
(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.
Comment 111 Dave Townsend [:mossop] 2012-03-22 11:02:12 PDT
(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
Comment 112 Bill Walker [:bwalker] [@wfwalker] 2012-03-22 11:05:20 PDT
(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
Comment 113 Dave Townsend [:mossop] 2012-03-22 11:22:29 PDT
(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.
Comment 114 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-22 14:02:47 PDT
(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.
Comment 115 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-22 14:47:37 PDT
Is the code in pdf.js (not the bootstrap code) running with chrome or content privileges?
Comment 116 Yury Delendik (:yury) 2012-03-22 15:29:36 PDT
> 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
Comment 117 Masatoshi Kimura [:emk] 2012-03-22 16:12:48 PDT
(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?
Comment 118 Dave Townsend [:mossop] 2012-03-23 09:25:55 PDT
(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
Comment 119 Guillaume C. [:ge3k0s] 2012-03-23 09:35:31 PDT
Is there any chance that Stephen Horlander takes a look at this. Because the user interface looks very old.
Comment 120 neil@parkwaycc.co.uk 2012-03-23 12:27:44 PDT
(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?
Comment 121 Dave Townsend [:mossop] 2012-03-23 12:48:07 PDT
(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.
Comment 122 Armen Zambrano [:armenzg] (EDT/UTC-4) 2012-03-23 13:13:11 PDT
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?
Comment 123 Dave Townsend [:mossop] 2012-03-23 13:14:57 PDT
(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
Comment 124 antistress 2012-03-23 21:07:28 PDT
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)
Comment 125 Daniel Cater 2012-03-24 06:11:13 PDT
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.
Comment 126 Constantine 2012-03-24 08:59:36 PDT
(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.
Comment 127 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-03-24 11:57:57 PDT
*** Bug 344620 has been marked as a duplicate of this bug. ***
Comment 128 Avi Halachmi (:avih) 2012-04-01 05:34:35 PDT
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)
Comment 129 Constantine 2012-04-06 13:46:04 PDT
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.
Comment 130 Mardeg 2012-06-04 14:44:20 PDT
*** Bug 455917 has been marked as a duplicate of this bug. ***
Comment 131 Mardeg 2012-06-04 14:44:23 PDT
*** Bug 456148 has been marked as a duplicate of this bug. ***
Comment 132 Gabriela [:gaby2300] 2012-11-01 14:38:47 PDT
Verified fixed using Windows 7 and the latest Nightly.

Note You need to log in before you can comment on or make changes to this bug.