1338476 - [jsplugins] Add a pref for turning on/off mortar pdf

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect)

Description

[Ya-Chieh Wu (inactive)]

This idea came up when we wanted to turn mortar pdf on/off in nightly.

Comment 1

[Ya-Chieh Wu (inactive)]

Attachment: Bug-1338476-Add-a-pref-for-turning-on-off-mortar-pdf.patch

I add |pref("pdfium.disabled", false)| to firefox.js for us to disable/enable the registration of our pdfium. This patch is based on Bug 1340472's modification.

Comment 2

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1338476-Add-a-pref-for-turning-on-off-mortar-pdf.patch

The preference is setting to |pref("pdfium.enabled", false)|.

Comment 3

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Add-a-pref-for-turning-on-off-mortar-pdf.patch

Comment 4

[Ya-Chieh Wu (inactive)]

Comment on attachment 8844817 [details] [diff] [review]
0001-Add-a-pref-for-turning-on-off-mortar-pdf.patch

Note:

(1) I move "ppapi.js:generateRandomBytes"'s listener outside of init/uninit because turn pdfium on/off should only effect future tabs.  
(2) Turning off the registration become effective after no more opened pdf tab that uses pdfium.

Comment 5

[Ya-Chieh Wu (inactive)]

Hey Evelyn,

I need our people to at least review the add-on part. Could you help?
As for the modification of firefox.js, I will find out it we need certain people to review it. 

Also, the issue 2 I mentioned in Comment 4 will be handled in Bug 1345050.

thx.

Comment 6

[Evelyn Hung]

Comment on attachment 8844817 [details] [diff] [review]
0001-Add-a-pref-for-turning-on-off-mortar-pdf.patch

Today is *review commit message first* day, so please add bug number in the commit, and write more informative messages e.g. the pref name you added. Thanks.

Comment 7

[Evelyn Hung]

Comment on attachment 8844817 [details] [diff] [review]
0001-Add-a-pref-for-turning-on-off-mortar-pdf.patch

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

The code looks okay to me. Should we prevent the case that pdfium.init() being called twice? I think it might happen if pref observer doesn't guarantee to inform you only when the pref changes its value. Guess it triggers your preferObserver when every time the pref being touched. 

BTW, since the bootstrap.js is getting complicated, I feel we could refactor it a bit to make the code more clean and organized. Feel free to file another bug for that.

::: browser/extensions/mortar/host/pdf/bootstrap.js
@@ +12,5 @@
>    Components.classes["@mozilla.org/moz/jssubscript-loader;1"].getService(Components.interfaces.mozIJSSubScriptLoader).loadSubScript("resource://ppapipdf.js/ppapi-content-sandbox.js", sandbox);
>  }
>  
> +function prefObserver(aSubject, aTopic, aData) {
> +  if (Services.prefs.getBoolPref("pdfium.enabled")) {

nit: move "pdfium.enabled" to a const string.

@@ +80,5 @@
> +  if (Services.prefs.getBoolPref("pdfium.enabled")) {
> +    pdfium.init();
> +  }
> +  Services.prefs.addObserver("pdfium.enabled", prefObserver, false);
> +  Services.ppmm.addMessageListener("ppapi.js:generateRandomBytes", pdfium.generateRandomBytesListener);

nit: consider to move "ppapi.js:generateRandomBytes" to a const string, too.

Comment 8

[Ya-Chieh Wu (inactive)]

(In reply to Evelyn Hung [:evelyn] from comment #7)
> The code looks okay to me. Should we prevent the case that pdfium.init()
> being called twice? I think it might happen if pref observer doesn't
> guarantee to inform you only when the pref changes its value. Guess it
> triggers your preferObserver when every time the pref being touched.  

Since the pref is saving as a boolean, pdfium.init() won't be called twice in a row.
no?

Comment 9

[Ya-Chieh Wu (inactive)]

Attachment: Add "pdfium.enabled" as a pref for turning on/off mortar pdf

Hey Gijs,

I don't know if you can help with reviewing a pref that I add into browser/app/profile/firefox.js since I saw you reviewing the front-end code. We add this pref into Firefox for turning pdfium on/off. How do you think about this?



Hey Evelyn,

I updated the patch with addressing the issues in Comment 7. Could you take a look?

Comment 10

[Evelyn Hung]

(In reply to Ya-Chieh Wu from comment #8)
> (In reply to Evelyn Hung [:evelyn] from comment #7)
> > The code looks okay to me. Should we prevent the case that pdfium.init()
> > being called twice? I think it might happen if pref observer doesn't
> > guarantee to inform you only when the pref changes its value. Guess it
> > triggers your preferObserver when every time the pref being touched.  
> 
> Since the pref is saving as a boolean, pdfium.init() won't be called twice
> in a row.
> no?

Not sure if we can safely assume that. It depends on the implementation of observer that won't accidentally trigger preferObserver() twice when the value is unchanged. It may be possible to happen when there is somewhere in the code that will modify the pref and a weird racing issue happens... Anyway, I'm not the expert of preference, but if I were you, I will cache the last value to avoid consecutively calling pdfium.init() twice.

Comment 11

[:Gijs (out until Jan 5th; he/him)]

Comment on attachment 8846489 [details] [diff] [review]
Add "pdfium.enabled" as a pref for turning on/off mortar pdf

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

::: browser/app/profile/firefox.js
@@ +1205,5 @@
>  pref("toolkit.startup.max_resumed_crashes", 3);
>  
> +// Disable pdfium to register to pdf mime type.
> +// Note: turning off the registration become effective
> +// after no more opened pdf tab that uses pdfium.

English nits:

// Whether we use pdfium to view content with the pdf mime type.
// Note: if the pref is set to false while Firefox is open, it won't
// take effect until there are no open pdfium tabs.

Comment 12

[Ya-Chieh Wu (inactive)]

Attachment: Add "pdfium.enabled" as a pref for turning on/off mortar pdf

result of try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fd1737fb767632056a54eb3e83900a3733276d4

Comment 13

[Ya-Chieh Wu (inactive)]

the result of try looks fine.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32e9e0aa9a5b
Add "pdfium.enabled" as a pref for turning on/off mortar pdf. r=evelyn, r=Gijs

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32e9e0aa9a5b

Comment 16

[Tanvir Rahman]

I can see the feature implemented in latest Nightly 55.0a1 on windows 10, 64-bit

Build ID 	20170604030205
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Comment 17

[Md.Tarikul Islam Oashi]

I can see the feature implemented in latest Nightly 55.0a1 on Linux Mint 18.1 Serena"(64 Bit)

Build ID 	20170606100219
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
 
[Bugday-20170607]

Comment 18

[Tanvir Rahman]

As per Comment 16 and Comment 17, I am marking this bug as verified fixed.

Comment 19

[u603259]

Hi,

Is this flag intentionally broken? Testing on 60.0b1, Dev Edition.

Thanks,

DM

Comment 20

[:Gijs (out until Jan 5th; he/him)]

(In reply to Dylan Myers from comment #19)
> Is this flag intentionally broken? Testing on 60.0b1, Dev Edition.

:bdahl, do you know? :-)

Comment 21

[Brendan Dahl [:bdahl]]

I haven't heard anything about mortar in awhile, maybe Peter knows?

Comment 22

[u603259]

Not sure what the etiquette here is regarding post bumping but I am very interested in knowing. Thanks.

Comment 23

[Peter Van der Beken [:peterv]]

mortar was canceled, see https://wiki.mozilla.org/Mortar_Project.