740795 - Don't ship PDF Viewer as an add-on

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect)

Description

[JK]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120321033733

Steps to reproduce:

It is shipped by default, therefore it's not an add-on (a feature that is not a part of the default installation) and should not be shipped as an add-on.

Using the add-on manager as a way to disable the PDF viewer is not needed, as a preference can be added into the Options box.

Comment 1

[Jesper Hansen]

Not an add-on? (http://i.imgur.com/Bp2WS.png)
The user can choose to enable the add-on on their first run with this new bundled add-on or keep it disabled thus providing a easy and nice way to enabling and disabling pdf.js without embedding it into firefox.

Can someone comment on if this allows the developers to update pdf.js separate of firefox?

Comment 2

[Masatoshi Kimura [:emk]]

> Using the add-on manager as a way to disable the PDF viewer is not needed, as a preference can be added into the Options box.
If it is not shipped as an add-on, PDF Viewer should be selectable from Options > Applications like feeds.

Comment 3

[Dave Townsend [:mossop]]

(In reply to Jesper Hansen from comment #1)
> Not an add-on? (http://i.imgur.com/Bp2WS.png)
> The user can choose to enable the add-on on their first run with this new
> bundled add-on or keep it disabled thus providing a easy and nice way to
> enabling and disabling pdf.js without embedding it into firefox.
> 
> Can someone comment on if this allows the developers to update pdf.js
> separate of firefox?

The way it is currently packaged, no this doesn't allow updating pdf.js automatically.

Comment 4

[Daniel C]

I don't think having an invisible extension is a good idea. What are the difficulties in integrating the code with Firefox?

Comment 5

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

What do you see as the difference between an "invisible extension" and any other feature "integrated with Firefox"?

Having useful conversation about packaging mechanics is difficult because everyone seems to have slightly different hidden assumptions about exactly what implications each strategy has :)

Comment 6

[Daniel C]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #5)
> What do you see as the difference between an "invisible extension" and any
> other feature "integrated with Firefox"?

Maintenance, mainly, but obviously if the module owner has no problem with it then presumably it will be fine.

Let's say the preference to disable pdf.js works by disabling the extension. If I have add-on sync enabled will that then propogate across browsers even if I have preferences sync disabled? Will about:support list the extension? Crash reports? Will the compatibility check on upgrade check for updates to pdf.js?

That's without the problems that already exist - new installs don't have it enabled and upgrades run into the 3rd-party blocking code.

Comment 7

[Dave Townsend [:mossop]]

Attachment: WIP

I've been considering different approaches for embedding this code into Firefox. In particular I was looking for an approach that would work for other features that might get initially developed as add-ons and made it extremely easy to transition those into the build as well as easy to keep in sync with an externally maintained add-on.

This is what I think is a good way to go. It is some Makefile shenanigans that as a first pass allows any non-restartless add-on (no binary libraries though) to be dropped into the source and with no changes get embedded into omnni.ja and run essentially as if it were still an add-on.

Writing a simple JS component should make it possible to work for restartless add-ons in the future too. As it happens though it is trivial to convert pdf.js to a non-restartless add-on by adding a chrome.manifest that does everything its bootstrap.js does, probably slightly more performant that way too.

The Makefile goop that mangles chrome.manifests is a little ugly, might want to change that to a python script or something, but otherwise what are your thoughts on this?

Comment 8

[[e-mail]]

Comment on attachment 615003 [details] [diff] [review]
WIP

Any chance we can move this somewhere more useful, such as extensions/pdfjs ?

Nit: you added a manifest but you didn't alter the bootstrap code.

Comment 9

[[e-mail]]

Attachment: Proof of concept

Also removes all Firefox-specific code from pdf.js so that it works in all applications.

Comment 10

[Dave Townsend [:mossop]]

(In reply to [e-mail] from comment #8)
> Comment on attachment 615003 [details] [diff] [review]
> WIP
> 
> Any chance we can move this somewhere more useful, such as extensions/pdfjs ?
> 
> Nit: you added a manifest but you didn't alter the bootstrap code.

I don't have a problem with that, just wanting to get feedback on the approach before I come up with a final patch though.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 615003 [details] [diff] [review]
WIP

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

+1 on the more explicit directory name, and shallower filesystem depth for these to live in. would be great if test dirs were descended into and discovered automagically for each add-on, but that's gravy.

and since we have both module owners here: if this does end up in toolkit, would be nice to use this opportunity to sync the /browser and /toolkit strategy wrt this, for future features developed this way.

Comment 12

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

Comment on attachment 615003 [details] [diff] [review]
WIP

I might want to nitpick the top-level directory name ("features" may be confusing?), but this seems pretty reasonable. I agree that we'll probably want a python script instead of that makefile magic :)

Comment 13

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

Comment on attachment 615190 [details] [diff] [review]
Proof of concept

You should probably submit the FUEL-ectomy and other cleanup as a github pull-request (https://github.com/mozilla/pdf.js/tree/master/extensions/firefox), I think we want those either way.

I'm not sure I like using the old EXTENSIONS mechanism, mostly because I thought we were getting rid of that as much as possible.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 615003 [details] [diff] [review]
WIP

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

::: browser/features/Makefile.in
@@ +28,5 @@
> +                       --exclude=test \
> +                       --exclude=install.rdf \
> +                       --exclude=icon.png \
> +                       --exclude=icon64.png \
> +                       --exclude=chrome.manifest) | (cd $(CHROMEDIR) && tar -xf -)

This will only work for --enable-chrome-format=flat (which is what we actually use for omnijar). It might be better to generate a jar.mn, which will also work for --enable-chrome-format=jar and symlink.

Comment 15

[Dave Townsend [:mossop]]

(In reply to Mike Hommey [:glandium] from comment #14)
> Comment on attachment 615003 [details] [diff] [review]
> WIP
> 
> Review of attachment 615003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/features/Makefile.in
> @@ +28,5 @@
> > +                       --exclude=test \
> > +                       --exclude=install.rdf \
> > +                       --exclude=icon.png \
> > +                       --exclude=icon64.png \
> > +                       --exclude=chrome.manifest) | (cd $(CHROMEDIR) && tar -xf -)
> 
> This will only work for --enable-chrome-format=flat (which is what we
> actually use for omnijar). It might be better to generate a jar.mn, which
> will also work for --enable-chrome-format=jar and symlink.

Under what circumstances do we use those?

Comment 16

[Dave Townsend [:mossop]]

Also I'm not sure what "works" should mean in that case, when I build with jar chrome format it certainly "works" in the sense that it builds Firefox and when you run it pdf.js works, it just isn't inside a jar file. That seems relatively easy to fix without having to go through the trouble of jar.mn. Haven't looked into symlinks yet but that seems likely as easy too.

Comment 17

[Mike Hommey [:glandium]]

(In reply to Dave Townsend (:Mossop) from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #14)
> > Comment on attachment 615003 [details] [diff] [review]
> > WIP
> > 
> > Review of attachment 615003 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/features/Makefile.in
> > @@ +28,5 @@
> > > +                       --exclude=test \
> > > +                       --exclude=install.rdf \
> > > +                       --exclude=icon.png \
> > > +                       --exclude=icon64.png \
> > > +                       --exclude=chrome.manifest) | (cd $(CHROMEDIR) && tar -xf -)
> > 
> > This will only work for --enable-chrome-format=flat (which is what we
> > actually use for omnijar). It might be better to generate a jar.mn, which
> > will also work for --enable-chrome-format=jar and symlink.
> 
> Under what circumstances do we use those?

So, actually, we do use symlink on Linux for omnijar. This has the advantage to allow modifying front-end code without even rebuilding anything (provided you touch non-preprocessed files). And I'm using jar for Iceweasel (the Debian rebranded Firefox) for the browser part because I haven't solved some of the configurability problems I'd get with omnijar (the XRE part is omnijared, though)

Comment 18

[Dave Townsend [:mossop]]

Attachment: WIP patch

This is a little further along now but I'd like to get some feedback on what it generates and some issues I hit.

On builds where the chrome packaging is flat/symlink/omnijar it just copies (or links) the feature's files into a directory under chrome. On builds where the chrome is packaged in jars it creates a jar under chrome for the feature. It then writes a fixed chrome.manifest under chrome pointing at the chrome.manifest for the feature either in the dir or in the jar. This required some work as the manifest line only supports a relative path, not pointing inside a jar so I put together a potential patch to support that. There might be an easier way to do that but for now it just figures out the full url to the manifest and then builds a FileLocation from that.

Main questions I'd like to hear answered:

Does this packaging seem right for each style of chrome packaging?

Does making manifest lines in chrome.manifest accept relative urls seem right?

Is there an easier way of generating a new FileLocation with a relative url to the previous? I guess it'd require a way of parsing relative URLs but I'm not sure we have that.

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 619084 [details] [diff] [review]
WIP patch

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

I haven't looked in detail, but wouldn't it be simpler to just create a $feature.manifest in the chrome directory that directly references things in the jar, instead of making it include the manifest that is in the jar?

Also, I'd suggest making this more than something for browser/ (there might be some interest in "bundling" addons in toolkit, too (things like about:telemetry), or in other apps), and I'd suggest avoiding our usual "let's do everything under libs::" pattern, because that's what makes our nop builds not be nop builds and take so long.

Comment 20

[Dave Townsend [:mossop]]

(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 619084 [details] [diff] [review]
> WIP patch
> 
> Review of attachment 619084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't looked in detail, but wouldn't it be simpler to just create a
> $feature.manifest in the chrome directory that directly references things in
> the jar, instead of making it include the manifest that is in the jar?

I can do that, but only if I also make component and interface lines support relative urls where again they only support paths right now.

> Also, I'd suggest making this more than something for browser/ (there might
> be some interest in "bundling" addons in toolkit, too (things like
> about:telemetry), or in other apps), and I'd suggest avoiding our usual
> "let's do everything under libs::" pattern, because that's what makes our
> nop builds not be nop builds and take so long.

What is the alternative to libs::?

Comment 21

[Benjamin Smedberg]

Comment on attachment 619084 [details] [diff] [review]
WIP patch

In general I strongly prefer to build these things using a jar.mn. In the case we have a "prepackaged" extension structure, can we just stick it into dist/bin/chrome/somedir and have a simple manifest line which references that chrome.manifest? Then it should be within the omnijar in all cases.

I definitely don't think that this amount of complexity is warranted.

Comment 22

[Mike Hommey [:glandium]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #21)
> Comment on attachment 619084 [details] [diff] [review]
> WIP patch
> 
> In general I strongly prefer to build these things using a jar.mn. In the
> case we have a "prepackaged" extension structure, can we just stick it into
> dist/bin/chrome/somedir and have a simple manifest line which references
> that chrome.manifest? Then it should be within the omnijar in all cases.
> 
> I definitely don't think that this amount of complexity is warranted.

The problem is with the jar chrome format. (you know, the one with separate jar files instead of directories under chrome/)

Comment 23

[Dave Townsend [:mossop]]

(In reply to Mike Hommey [:glandium] from comment #22)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #21)
> > Comment on attachment 619084 [details] [diff] [review]
> > WIP patch
> > 
> > In general I strongly prefer to build these things using a jar.mn. In the
> > case we have a "prepackaged" extension structure, can we just stick it into
> > dist/bin/chrome/somedir and have a simple manifest line which references
> > that chrome.manifest? Then it should be within the omnijar in all cases.
> > 
> > I definitely don't think that this amount of complexity is warranted.
> 
> The problem is with the jar chrome format. (you know, the one with separate
> jar files instead of directories under chrome/)

Yeah my previous patch just ignored the packaging format and made flat file structures. It works in all cases, you just don't end up with the code in a jar in the case of jar chrome format.

Comment 24

[Mike Hommey [:glandium]]

(In reply to Dave Townsend (:Mossop) from comment #20)
> (In reply to Mike Hommey [:glandium] from comment #19)
> > Comment on attachment 619084 [details] [diff] [review]
> > WIP patch
> > 
> > Review of attachment 619084 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I haven't looked in detail, but wouldn't it be simpler to just create a
> > $feature.manifest in the chrome directory that directly references things in
> > the jar, instead of making it include the manifest that is in the jar?
> 
> I can do that, but only if I also make component and interface lines support
> relative urls where again they only support paths right now.

Why don't they simply go in $APPDIR/components?

Comment 25

[Mike Hommey [:glandium]]

(In reply to Dave Townsend (:Mossop) from comment #20)
> What is the alternative to libs::?

There are different solutions to the problem. Best might be to discuss with people on #pymake.

Comment 26

[Dave Townsend [:mossop]]

(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Dave Townsend (:Mossop) from comment #20)
> > (In reply to Mike Hommey [:glandium] from comment #19)
> > > Comment on attachment 619084 [details] [diff] [review]
> > > WIP patch
> > > 
> > > Review of attachment 619084 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I haven't looked in detail, but wouldn't it be simpler to just create a
> > > $feature.manifest in the chrome directory that directly references things in
> > > the jar, instead of making it include the manifest that is in the jar?
> > 
> > I can do that, but only if I also make component and interface lines support
> > relative urls where again they only support paths right now.
> 
> Why don't they simply go in $APPDIR/components?

We could do that, maybe a python script that parses the manifest to find the components and copy those there and splits the manifest into two parts, one with the component/interface lines and one with the rest of the stuff for the chrome directory.

Comment 27

[[e-mail]]

(In reply to Gavin Sharp from comment #13)
> You should probably submit the FUEL-ectomy and other cleanup as a github
> pull-request
> (https://github.com/mozilla/pdf.js/tree/master/extensions/firefox), I think
> we want those either way.
I have no idea what github is, nor have I ever installed git...

> I'm not sure I like using the old EXTENSIONS mechanism, mostly because I
> thought we were getting rid of that as much as possible.
Do we actually have a useful replacement for that yet?

(In reply to Mike Hommey from comment #14)
> This will only work for --enable-chrome-format=flat (which is what we
> actually use for omnijar). It might be better to generate a jar.mn, which
> will also work for --enable-chrome-format=jar and symlink.
As far as I can tell, the extension has no chrome (only components and resources) so the chrome format doesn't appear to be relevant.

Comment 28

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

(In reply to [e-mail] from comment #27)
> (In reply to Gavin Sharp from comment #13)
> > You should probably submit the FUEL-ectomy and other cleanup as a github
> > pull-request
> > (https://github.com/mozilla/pdf.js/tree/master/extensions/firefox), I think
> > we want those either way.
> I have no idea what github is, nor have I ever installed git...

What a wonderful learning opportunity! :) FWIW, you don't need to install git to contribute via github (you can do it entirely on the web).

Comment 29

[Dave Townsend [:mossop]]

Attachment: patch

Ok, would like to just get on with this now. This is pretty much the simple case (I left the file moves for the pdf.js bits out of the patch to make it simpler).

The Makefile copies anything in the extension's directory (ignoring a few unnecessary files) into a directory under chrome, then if there is a manifest creates a manifest file inside the chrome dir for it. This is necessary since when converting to omnijar, only manifests directly inside the chrome directory are properly handled. We could avoid the additional level of indirection there by manually rewriting the paths in chrome.manifests, I have a python script that does it but this way seems simpler to start with.

In the case of our normal builds, everything makes it into omnijar and works fine. For flat or jar chrome formats, the files are just in a flat directory structure and also work fine. Since the jar format isn't used in our regular builds I don't want to spend the time getting this right and holding up this.

I talked with gavin on IRC and decided on going with browser/extensions. It is a bit of an overloaded term but they are extensions we are putting in there and if we find it is too confusing we can always move it later.

Comment 30

[Dave Townsend [:mossop]]

Attachment: pdf.js file moves

These are the file moves and the addition of a chrome.manifest for pdf.js

Comment 31

[Dave Townsend [:mossop]]

Comment on attachment 615190 [details] [diff] [review]
Proof of concept

As gavin said, take the pdf.js changes upstream for review. As for making pdf.js shared with other apps I don't think we should impose the additional work there when we haven't even quite figured out how this is working with Firefox yet.

Comment 32

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 622210 [details] [diff] [review]
patch

IIUC, a Makefile in browser/extensions is designed to work with all such extensions.  However, the "STRIP" list is quite specific to pdf.js and not extensible for future extensions.  Also, I'm working on another "extension" that has a .idl file that needs to be built - it's not clear how that would be done.

Is there a reason we don't take the usual approach of allowing each subdir to have its own Makefile while the Makefile in browser/extensions is just the glue that binds it all together?

Comment 33

[Dave Townsend [:mossop]]

(In reply to Mark Hammond (:markh) from comment #32)
> Comment on attachment 622210 [details] [diff] [review]
> patch
> 
> IIUC, a Makefile in browser/extensions is designed to work with all such
> extensions.  However, the "STRIP" list is quite specific to pdf.js and not
> extensible for future extensions.  Also, I'm working on another "extension"
> that has a .idl file that needs to be built - it's not clear how that would
> be done.
> 
> Is there a reason we don't take the usual approach of allowing each subdir
> to have its own Makefile while the Makefile in browser/extensions is just
> the glue that binds it all together?

This is designed to just take existing packaged addons where there isn't any special work required (true the strip list might be a little better though). I expect it to get tweaked as we include more things like this, I don't want to attempt to design too much for the future right now though.

Comment 34

[Mike Hommey [:glandium]]

Please rename the STRIP variable. STRIP is an existing variable in config/autoconf.mk.in

Comment 35

[Benjamin Smedberg]

Attachment: Part "B": Add an -X flag to nsinstall.py, rev. 1

Comment 36

[Benjamin Smedberg]

Attachment: Part "C" - use nsinstall.py with -X to install the bits we care about, rev. 2

Comment 37

[(not currently active) Ted Mielczarek]

Comment on attachment 623208 [details] [diff] [review]
Part "B": Add an -X flag to nsinstall.py, rev. 1

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

Please add a unit test for this:
http://mxr.mozilla.org/mozilla-central/source/config/tests/unit-nsinstall.py

::: config/nsinstall.py
@@ +115,5 @@
>      p.error('not enough arguments')
>  
>    def copy_all_entries(entries, target):
>      for e in entries:
> +      if e in options.X:

Having to specify a full pathname to skip seems difficult to use. I think you should at least use os.path.normpath on both sides here to make sure fiddly path differences don't break things.

Comment 38

[(not currently active) Ted Mielczarek]

Comment on attachment 623209 [details] [diff] [review]
Part "C" - use nsinstall.py with -X to install the bits we care about, rev. 2

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

::: browser/extensions/Makefile.in
@@ +10,5 @@
> +CHROMEDIR = $(call core_abspath,$(DIST))/bin/chrome
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +TEST_DIRS += $(addsuffix /test,$(EXTENSIONS))

I don't see where $(EXTENSIONS) comes from?

@@ +14,5 @@
> +TEST_DIRS += $(addsuffix /test,$(EXTENSIONS))
> +
> +include $(topsrcdir)/config/rules.mk
> +
> +STRIP = \

As glandium pointed out, $(STRIP) is already used in autoconf.mk for the 'strip' binary, so you should rename this variable. Perhaps SKIP_FILES?

Related, we've been discussing using lowercase variable names as a convention for Makefile-local variables (compared to ALL_CAPS for special vars handled by the build system).

@@ +22,5 @@
> +  icon.png \
> +  icon64.png \
> +  $(NULL)
> +
> +FILES = $(filter-out $(STRIP),$(patsubst $(srcdir)/$(extension)/%,%,$(wildcard $(srcdir)/$(extension)/*)))

This looks leftover from something else?

@@ +26,5 @@
> +FILES = $(filter-out $(STRIP),$(patsubst $(srcdir)/$(extension)/%,%,$(wildcard $(srcdir)/$(extension)/*)))
> +
> +libs::
> +	$(PYTHON) $(topsrcdir)/config/nsinstall.py $(srcdir)/pdfjs \
> +          $(foreach stripped,$(STRIP), -X $(srcdir)/pdfjs/$(stripped)) \

Can you move the first argument to nsinstall down to its own line? That'd make this a bit easier to read.

Comment 39

[Benjamin Smedberg]

Attachment: Part "C" - use nsinstall.py with -X to install the bits we care about, rev. 2

Comment 40

[Bill Walker [:bwalker] [@wfwalker]]

Can this patch land now?

Comment 41

[Benjamin Smedberg]

This landed and bounced on inbound because of check-sync-dirs bustage. I've started a try run and will reland if we're good there.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Original landing and backout:
https://hg.mozilla.org/mozilla-central/rev/6eb40a0f8e4c
https://hg.mozilla.org/mozilla-central/rev/7bbd1cc126f3
https://hg.mozilla.org/mozilla-central/rev/a65ea296a6ea
https://hg.mozilla.org/mozilla-central/rev/db15d44550a6
https://hg.mozilla.org/mozilla-central/rev/598158a33067

Comment 43

[Willy_ Foo_Foo]

I don't think having an invisible extension is a good idea. What are the difficulties in integrating the code with Firefox?
Firefox has a problem updating itself if it is removed(in standard install remove PDF.js from install directory, result incremental update does not apply)

Comment 44

[Dave Townsend [:mossop]]

(In reply to magnumarchonbasileus from comment #43)
> I don't think having an invisible extension is a good idea. What are the
> difficulties in integrating the code with Firefox?
> Firefox has a problem updating itself if it is removed(in standard install
> remove PDF.js from install directory, result incremental update does not
> apply)

The patches here does integrate the code with Firefox.

Comment 45

[Willy_ Foo_Foo]

(In reply to Dave Townsend (:Mossop) from comment #44)
> The patches here does integrate the code with Firefox.

So is it integrated yet(or will in time for FF 15?)

Comment 46

[Dave Townsend [:mossop]]

(In reply to magnumarchonbasileus from comment #45)
> (In reply to Dave Townsend (:Mossop) from comment #44)
> > The patches here does integrate the code with Firefox.
> 
> So is it integrated yet(or will in time for FF 15?)

It will be when these patches land, there are other things that need fixing for this feature to ship with a release version of Firefox though, those are tracked in other bugs.

Comment 47

[Benjamin Smedberg]

https://hg.mozilla.org/mozilla-central/rev/d50e4a17308b

Comment 48

[JK]

On build http://hg.mozilla.org/mozilla-central/rev/f43e8d300f21 (2012-05-24) it's still listed as an add-on.

Comment 49

[Serge Gautherie (:sgautherie)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #47)

https://hg.mozilla.org/mozilla-central/rev/edb5a8927dae
https://hg.mozilla.org/mozilla-central/rev/ffaf38d1e36d
+

> https://hg.mozilla.org/mozilla-central/rev/d50e4a17308b