The Reader Mode toolbar has a button that takes you straight to the 'Reading List' page in about:home. The current code assumes that the 'Reading List' pane will always be in about:home. However, by making about:home configurable, this might not be the case anymore. mfinkle suggested that maybe we should simply force the 'Reading List' page to show up in about:home if coming from the Reader Mode UI (even if the user has hidden the Reading List page via Settings UI). The 'Reading List' button in the Reader Mode UI would become less relevant if we implement things like bug 750678 and bug 871593.

Doing a little investigation on the code for this. We send a message from JS to Java that gets handled here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1207 Which then lands us in here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1427 Which causes us to send a message back to JS about the reading list: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#727 Which we then get back later in Java: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#659 I think we should take this opportunity to take a step back and simplify the way all this works. I don't think Tab/Tabs should know about HomePager stuff. And I really don't think that JS should need to know about the HomePager pages. It looks like all this was added for this feature in the first place: http://hg.mozilla.org/mozilla-central/rev/3ce8681e5602 But maybe we can come up with a better solution now that this code has evolved a bit?

Feel free to redirect if you don't have time to work on this yourself.

(In reply to Lucas Rocha (:lucasr) from comment #0) > mfinkle suggested that maybe we should simply force the 'Reading List' page > to show up in about:home if coming from the Reader Mode UI (even if the user > has hidden the Reading List page via Settings UI). Suggestion from bug 963174: how about we just show the reading list fragment instead of the entire home pager? One of the benefits of fragments is that they're self-contained, reusable modules, so this seems like a good place to take advantage of that.

Ian, Finkle wants me to NI you so you know that this isn't happening in 29

What's the plan here? As a temporary measure, should we just hide the reading list button in reader mode? Right now, if the user doesn't have the reading list panel in their home config, we'll just land on the default panel, which isn't the end of the world, but it's not ideal. This issue affects Fx29, so we should decide if we're okay with this current situation, or if we want some temporary measure. We could try to implement a fix to just show the reading list fragment like bnicholson suggested in comment 4, but that probably isn't something we'll want to uplift. I vote that we should just land a patch to hide this button. With sola's fix for bug 909618, the user would be able to hit the back button to get back to their reading list after finishing an article (although that wouldn't be true if they entered reader mode from a webpage, as opposed to the reading list).

(In reply to :Margaret Leibovic from comment #6) > I vote that we should just land a patch to hide this button. With sola's fix > for bug 909618, the user would be able to hit the back button to get back to > their reading list after finishing an article +1, exactly this.

(In reply to Ian Barlow (:ibarlow) from comment #7) > (In reply to :Margaret Leibovic from comment #6) > > > I vote that we should just land a patch to hide this button. With sola's fix > > for bug 909618, the user would be able to hit the back button to get back to > > their reading list after finishing an article > > +1, exactly this. Sounds like a good plan.

It also looks like _readingListCount is only used for updating this button, but I held off on removing that logic in case there is some other planned use for it. However, if you think we should get rid of it, I can update the patch to do that.

I just adjusted the style to have the remaining 3 buttons fill up the space.

Comment on attachment 8409911 [details] [diff] [review] Remove reading list button from reader mode toolbar Review of attachment 8409911 [details] [diff] [review]: ----------------------------------------------------------------- I think you can safely remove the 'list count' message bits. Giving f+ to get an updated patch with these changes.

Updated to remove reading list count logic.

Comment on attachment 8410315 [details] [diff] [review] (v2) Remove reading list button from reader mode toolbar Review of attachment 8410315 [details] [diff] [review]: ----------------------------------------------------------------- Look good, thanks.

Comment on attachment 8410315 [details] [diff] [review] (v2) Remove reading list button from reader mode toolbar [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 942231 (too late for fx29) User impact if declined: reading list button in reader mode won't work properly if reading list panel is disabled Testing completed (on m-c, etc.): just landed on fx-team Risk to taking this patch (and alternatives if risky): low-risk, removes a button from reader mode toolbar String or IDL/UUID changes made by this patch: none

Verified as fixed in builds: - 30 beta 1; - 31.0a2 (2014-04-29); - 32.0a1 (2014-04-29). Device: Samsung Galaxy R (Android 2.3.4).

Comment on attachment 8409912 [details] screenshot just clearing some feedback flags, don't mind me

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release). Filter on epic-hub-bugs.