867875 - Add the pref to switch reader mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

After Bug 852417, we can enable reader mode on low memory platform.
Now, I propose that we introduce a new pref to switch reader mode on general.

I think to add these change:
* Add the pref to toggle reader mode.
 * Toggle parse-on-load reader mode.
 * Toggle "Add reading list" context menu.
* Change the name "reader.force_allow" for compatibilities about above changes.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part1: Add the pref to toggle reader mode parsing on load

This patch includes:
* Toggle parse-on-load reader mode.
* Change the name "reader.force_allow" for compatibilities about above changes.

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

And I'm making patches which adds the pref to toggle "Add reading list" context menu.

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part2: Support function as the 2nd paramater of NativeWindow.contextmenus.add()

I think we need to clean up & make these part smart in the future...

Comment 4

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part3: Add the pref to toggle "Add Reading List" context menu

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 745078 [details] [diff] [review]
part1: Add the pref to toggle reader mode parsing on load

Looks good to me. The pref observer might not be strictly needed, but it's a nice convenience.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 745078 [details] [diff] [review]
part1: Add the pref to toggle reader mode parsing on load

>   init: function Reader_init() {

>+    Services.prefs.addObserver("reader.", this, false);

Maybe this should be "reader.parse-on-load." since that's all we look for in observe

>   uninit: function Reader_uninit() {
>+    Services.prefs.removeObserver("reader.", this);

Same

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> Comment on attachment 745078 [details] [diff] [review]
> part1: Add the pref to toggle reader mode parsing on load
> 
> Looks good to me. The pref observer might not be strictly needed, but it's a
> nice convenience.

Thank you review! I intend to reduce the number of times checking the pref by the pref observer. This pref will not change many times. So it's better that caching the value.

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Comment on attachment 745078 [details] [diff] [review]
> part1: Add the pref to toggle reader mode parsing on load
> 
> >   init: function Reader_init() {
> 
> >+    Services.prefs.addObserver("reader.", this, false);
> 
> Maybe this should be "reader.parse-on-load." since that's all we look for in
> observe
> 
> >   uninit: function Reader_uninit() {
> >+    Services.prefs.removeObserver("reader.", this);
> 
> Same

These points focuses part.3 ( attachment 745635 [details] [diff] [review] ).

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

I need to think about part 2 and part 3 a bit more. I see disabling the menus as a different issue, and I'm not sure we really need to support it. We don't have prefs for disabling any other menus. Being able to enable/disable the reader-on-pageload is separate (and different) from controlling the context menu as well.

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> I need to think about part 2 and part 3 a bit more. I see disabling the
> menus as a different issue, and I'm not sure we really need to support it.
> We don't have prefs for disabling any other menus. Being able to
> enable/disable the reader-on-pageload is separate (and different) from
> controlling the context menu as well.

At first, I had seemed that reader mode are optional features for a current browser. Thus I proposed the patch part 2&3. But I agree your point that is we need to think about it more.

I'll rebase part 1, and file a new bug for thinking about part2&3.

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Add the pref to toggle reader mode parsing on load

Rebase the patch.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff22928369b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ff22928369b