1132925 - Don't set preferences in AboutReader.jsm (so that it can run in the child process)

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[:Margaret Leibovic]

This is needed for bug 1129106. To do this, we should pass messages to the parent process to set reader mode prefs, rather than do it directly in AboutReader.jsm.

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - set reader mode prefs through parent process

Comment 2

[:Margaret Leibovic]

Comment on attachment 8564634 [details] [diff] [review]
rev 1 - set reader mode prefs through parent process

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

Sorry for the slow review, this looks good! You'll just need to update your tree and rebase, since some changes have landed that make your patch fail to apply.

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - set reader mode prefs through parent process

Comment 4

[:Margaret Leibovic]

Comment on attachment 8567221 [details] [diff] [review]
rev 2 - set reader mode prefs through parent process

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

Looks good! I just have one small nit.

::: browser/modules/ReaderParent.jsm
@@ +28,5 @@
>      "Reader:Share",
>      "Reader:SystemUIVisibility",
>      "Reader:UpdateReaderButton",
> +    "Reader:setIntPref",
> +    "Reader:setCharPref",

Nit: I think we should capitialize Set here, to be consistent with the other messages, e.g. "Reader:SetIntPref".

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - set reader mode prefs through parent process

Comment 6

[:Margaret Leibovic]

Comment on attachment 8567298 [details] [diff] [review]
rev 3 - set reader mode prefs through parent process

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

Looks good, thanks!

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/a9139003abaf

Comment 8

[:Margaret Leibovic]

Gah, I failed at reviewing this patch and had to back it out:
https://hg.mozilla.org/integration/fx-team/rev/d5cf77595feb

We also need to update Firefox for Android's Reader.js file to handle these messages on mobile:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Reader.js

And we register the listeners in browser.js here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#173

Can you update the patch to do this? I can test for you on Android if you don't have a mobile build environment set up.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8567298 [details] [diff] [review]
rev 3 - set reader mode prefs through parent process

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

We need to fix this for mobile, as mentioned in my last comment.

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - set reader mode prefs through parent process

(In reply to :Margaret Leibovic from comment #8)
> Can you update the patch to do this? I can test for you on Android if you
> don't have a mobile build environment set up.

Currently I don't have a mobile build environment so please test it on Android.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8567377 [details] [diff] [review]
rev 4 - set reader mode prefs through parent process

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

Confirmed this works on mobile. Thanks for the quick update, and I'm sorry about missing this the first time!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f2fd19c776cf

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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