We should wait for the ability to move bookmarks between folders before doing this, but once that's possible, adding an item to your reading list should be equivalent to moving a bookmark to a special folder.

I think we should consider doing this before removing "add to reading list" in other places. I'm starting to think that we should make the reading list folder just be a magical folder of your saved reader-view-ed pages, not something that users can organize things into or out of. Removing this button would help solve our problem of letting users add non-reader-view pages to the reading list. Although to do that completely we'd also need to remove the "add to reading list" button from the share overlay, which I'd be in favor of. Question: if we remove this item from the main menu, what should the menu look like?

Attaching mock. Removing the "Add to RL" button in the current design puts our menu in bit of a weird position. Without the "shareplane" here by default (bug 1140048), we will have 5 icons. To add one more space for quick share icons, I tried making room by changing the top row to 4 icons but that seemed too cramped. I think the better design is to keep it at 3 per row. Given that and looking at our telemetry, the <- back button gets pressed a fair amount but there's also a hardware back button that does the same thing. I'd like to propose that we remove this icon and simplify the here. Bug 1189272 mentions moving the menu position up as well and I think that'll help not only make us more visually consistent with Material design, but also requires less movement for common menu actions like 'Refresh'.

... and this is what the default "share" would look like for the time being (given that we still can't actually keep the share icon around as I learned in bug 1140048). Feel free to ignore the new carets next to Page and Tools for the time being, I think that can be out of scope of this bug.

I like the new menu. My only concern is the back button. Without it there's no visual indication that you can go back. The device's back button might go back or close the app.

1) Are we scope-creeping here by adding the removal of the back button? More/Complicated Eng work? 2) Based on the telemetry info you mentioned for the back button, maybe we "need to" keep it, any other ideas/compromise how to keep it but solve the removal of the "add to reading list"? Also what are we talking about when you say "pressed a fair amount", compared to? I'm in favor of removing the back button but would like us to be able to answer the questions above. As per Sebastian's comment, close the app/switch the another app would only happen on first load of a page though, no?

I didn't actually notice the removal of the back button when I looked at this briefly yesterday... this does feel like scope creep that could lead to its own set of problems. Looking at UI telemetry, it looks like people use this button about the same amount as the forward button. Here's some telemetry data for non-tablets: https://www.dropbox.com/s/sa9siibgkyhdk2a/Screenshot%202016-01-21%2014.23.06.PNG?dl=0 What did we do in the past? Can we have 4 items across the top of the menu? I do see that Chrome only has a forward button, so maybe this isn't a problem in practice... but I don't want to go with a design just because Chrome does it.

(In reply to Sebastian Kaspari (:sebastian) from comment #4) > I like the new menu. My only concern is the back button. Without it there's > no visual indication that you can go back. The device's back button might go > back or close the app. I'm not too worried about users not knowing they can go back without a back arrow. Since, "going back" to the last page is just something that's become so synonymous with browsing in general. Also, no other apps have "back" in the menu, it's usually in the top left (like our Settings). The hardware back button is also so ubiquitous to Android devices as well that I think it's the primary way users "go back". Are there devices that don't have a back button? But, I do share a concern about the inconsistency in it's behavior - sometimes close sometimes back. At the same time though, we already design our experience around the hardware back (tabs close if you came in from another app for example), so I don't see this as a major blocking issue. (In reply to Barbara Bermes [:barbara] from comment #5) > 1) Are we scope-creeping here by adding the removal of the back button? > More/Complicated Eng work? Perhaps a bit. But removing the "Add to RL" alone is going to be hard because of the design of our UI here. > 2) Based on the telemetry info you mentioned for the back button, maybe we > "need to" keep it, any other ideas/compromise how to keep it but solve the > removal of the "add to reading list"? Also what are we talking about when > you say "pressed a fair amount", compared to? (In reply to :Margaret Leibovic from comment #6) > I didn't actually notice the removal of the back button when I looked at > this briefly yesterday... this does feel like scope creep that could lead to > its own set of problems. Looking at UI telemetry, it looks like people use > this button about the same amount as the forward button. > > Here's some telemetry data for non-tablets: > https://www.dropbox.com/s/sa9siibgkyhdk2a/Screenshot%202016-01-21%2014.23.06. > PNG?dl=0 This is where it starts to sound weird to me actually. Since this is telemetry in our "menu" only, we're also not capturing the system's more reliable way of "going back" (the hardware button). Perhaps these numbers for "back" are simply the left-overs? > What did we do in the past? Can we have 4 items across the top of the menu? We've had 4 items in the quick share level before. > I do see that Chrome only has a forward button, so maybe this isn't a > problem in practice... but I don't want to go with a design just because > Chrome does it. Yeah, I agree. But given their large market share, I think we should be aware that they have an ability to influence user's expectations and their appetite for this button (or not having it). Also, removing the button and simplifying to just 1 spot for 1 thing is helpful to our UX.

Review commit: https://reviewboard.mozilla.org/r/38597/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38597/

Review commit: https://reviewboard.mozilla.org/r/38599/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38599/

These two patches only deal with removing the reading list button, the remaining changes are on my TODO list for now.

Will this bug also cover "add to reading list" from the book icon in the urlbar? I didn't see a bug for that.

(In reply to Chenxia Liu [:liuche] from comment #11) > Will this bug also cover "add to reading list" from the book icon in the > urlbar? I didn't see a bug for that. Is that the long-press to add to reading list? I think that was removed in Bug 1234335.

Yep! You're totally right!

Comment on attachment 8727666 [details] MozReview Request: Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella https://reviewboard.mozilla.org/r/38599/#review38641 There are some unused references to `onReadingListEventListener` here – I don't think this compiles.

Comment on attachment 8727666 [details] MozReview Request: Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella https://reviewboard.mozilla.org/r/38599/#review38649 r+ when you remove those unused references.

https://reviewboard.mozilla.org/r/38599/#review38651 ::: mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java:35 (Diff revision 1) > import android.util.Log; > > public final class ReadingListHelper implements NativeEventListener { > private static final String LOGTAG = "GeckoReadingListHelper"; > > public interface OnReadingListEventListener { This interface is also unused.

Comment on attachment 8727665 [details] MozReview Request: Bug 1234319 - Remove reading list button from main menu r=mcomella https://reviewboard.mozilla.org/r/38597/#review38647 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3202 (Diff revision 1) > return true; > } > > final boolean inGuestMode = GeckoProfile.get(this).inGuestMode(); > > final boolean isAboutReader = AboutPages.isAboutReader(tab.getURL()); unused

Review commit: https://reviewboard.mozilla.org/r/42799/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42799/

Review commit: https://reviewboard.mozilla.org/r/42801/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42801/

Comment on attachment 8727665 [details] MozReview Request: Bug 1234319 - Remove reading list button from main menu r=mcomella Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38597/diff/1-2/

Comment on attachment 8727666 [details] MozReview Request: Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38599/diff/1-2/

Comment on attachment 8735479 [details] MozReview Request: Bug 1234319 - Post: Don't track or care about the reading list in Tabs r?mcomella https://reviewboard.mozilla.org/r/42799/#review39351

Comment on attachment 8735480 [details] MozReview Request: Bug 1234319 - Post: purge ReadingListAccessor and ReadingListProvider r?mcomella https://reviewboard.mozilla.org/r/42801/#review39353 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java (Diff revision 1) > } else if ("Telemetry:Gather".equals(event)) { > final BrowserDB db = getProfile().getDB(); > final ContentResolver cr = getContentResolver(); > Telemetry.addToHistogram("PLACES_PAGES_COUNT", db.getCount(cr, "history")); > Telemetry.addToHistogram("FENNEC_BOOKMARKS_COUNT", db.getCount(cr, "bookmarks")); > - Telemetry.addToHistogram("FENNEC_READING_LIST_COUNT", db.getReadingListAccessor().getCount(cr)); Do we want to keep this for the reading list smart folder?

https://reviewboard.mozilla.org/r/42801/#review39353 > Do we want to keep this for the reading list smart folder? We'll be adding new telemetry in Bug 1246159, and I think we also want to move to UI telemetry in general, hence removing this is probably the right thing to do for now?

https://hg.mozilla.org/integration/fx-team/rev/1ee8f56284d9887ac08b92063ba47416b29aa661 Bug 1234319 - Remove reading list button from main menu r=mcomella https://hg.mozilla.org/integration/fx-team/rev/32c0ef305a73ad75a61e0a6b4e25f514ef6aec53 Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella https://hg.mozilla.org/integration/fx-team/rev/3d82a3fa52dbe1b7621364a4c3ad3d92c5ce5d12 Bug 1234319 - Post: Don't track or care about the reading list in Tabs r=mcomella https://hg.mozilla.org/integration/fx-team/rev/94e0c7b66782e265c22aa6a90c519ced4a2562de Bug 1234319 - Post: purge ReadingListAccessor and ReadingListProvider r=mcomella

https://hg.mozilla.org/mozilla-central/rev/1ee8f56284d9 https://hg.mozilla.org/mozilla-central/rev/32c0ef305a73 https://hg.mozilla.org/mozilla-central/rev/3d82a3fa52db https://hg.mozilla.org/mozilla-central/rev/94e0c7b66782

Verified as fixed using: Device: Nexus 6 (Android 6.0) and Nexus 7 (Android 5.1) Build: Firefox for Android 48.0a1 (2016-04-10)