Closed Bug 867875 Opened 7 years ago Closed 7 years ago

Add the pref to switch reader mode

Categories

(Firefox for Android :: Reader View, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: tetsuharu, Assigned: tetsuharu)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
This patch includes:
* Toggle parse-on-load reader mode.
* Change the name "reader.force_allow" for compatibilities about above changes.
Attachment #745078 - Flags: review?(mark.finkle)
And I'm making patches which adds the pref to toggle "Add reading list" context menu.
I think we need to clean up & make these part smart in the future...
Attachment #745634 - Flags: review?(mark.finkle)
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.
Attachment #745078 - Flags: review?(mark.finkle) → review+
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
(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] ).
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.
(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.
Rebase the patch.
Attachment #745078 - Attachment is obsolete: true
Attachment #745634 - Attachment is obsolete: true
Attachment #745635 - Attachment is obsolete: true
Attachment #745634 - Flags: review?(mark.finkle)
Attachment #745635 - Flags: review?(mark.finkle)
Attachment #745784 - Flags: review?(mark.finkle)
Attachment #745784 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/5ff22928369b
Assignee: nobody → saneyuki.s.snyk
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.