Initial idea is to add a browser menu item for that. When pages are added to reading list, they should available for offline usage later (see bug 750702). This bug is about implementing the basic operation of adding a page to the reading list.

Again, I'll probably squash all those db patches in one single change. I only split them to make them easier to review.

Ok, here is where the fun actually starts :-) This patch adds the reader button in the browser toolbar. As you can see, I added all the plumbing for dynamically showing/hiding it but the actual code to check if the current page is "reader-friendly" is not there yet. It should be just about sending a "ReaderEnabled" message to Java. So, for now, the reader button will never show up. I'll implement this in bug 750683. This patch also adds a dummy "Read now" button that does nothing (it will be implemented in bug 750698).

Here's how the reader icon looks in the browser toolbar

I've proposed (on IRC) to concentrate all reader operations in a doorhanger activated from the reader button in the browser toolbar. This seems like a better approach than splitting it in different places. ibarlow and patryc seem to agree. This is how the doorhanger looks right now. Patryk and Madhava, this needs proper design and assets.

Just one observation: I think the icons for reader and identity are going well together (see attached image). Maybe the reader icon should be a bit more subtle (lighter grey or something?)

Forgot to mention: need feedback from UX team on the toast notification text for when you add/remove items to/from the Reading List. For now, I'm using: "Page added to your Reading List" "Page removed from your Reading List" Not entirely happy about it, suggestions for simpler messages are welcome.

Here are some updated icons for the urlbar, to make the lock, stop/go/search, and reader mode icons feel more in line. Please replace all of these at the same time. http://cl.ly/3O42022J1H0N2E1Y3K11 Also, attached is a menu design we can pursue. I've added Sriram to this bug, as this design follows the same look as the custom overflow menu he has been implementing, so I imagine there is some code that can be shared here :)

(In reply to Lucas Rocha (:lucasr) from comment #5) > Created attachment 627284 [details] [diff] [review] > Pack URL bar buttons in same layout to allow showing multiple icons You might need to change the layout in layout-land-v14/ also. With the tablet UI code pushed in, there are newer browser_toolbar.xml in layout-xlarge/ and layout-sw600dp/. Please make similar changes there too.

Comment on attachment 627809 [details] [diff] [review] Factor out method to add a bookmark, generalize folder GUID argument Looks good to me. >diff --git a/mobile/android/base/db/LocalBrowserDB.java b/mobile/android/base/db/LocalBrowserDB.java >@@ -441,52 +441,54 @@ public class LocalBrowserDB implements B > Uri contentUri = mBookmarksUriWithProfile; Not part of your patch, but this local variable seems pretty useless. I feel like we could get rid of this and just be using mBookmarksUriWithProfile directly.

Comment on attachment 627811 [details] [diff] [review] Add method to add reading list items in BrowserDB We really need to finish bug 723623.

Comment on attachment 627813 [details] [diff] [review] Check for bookmarks should only consider normal bookmarks Unless one of your patches in bug 750684 handles this case, I think we're also going to need another patch to make sure we don't show the bookmark star on items in the awesome screen, since AwesomeBarTabs's updateBookmarkStar method doesn't go through this method. So many patches! Hard to keep track :)

Comment on attachment 627284 [details] [diff] [review] Pack URL bar buttons in same layout to allow showing multiple icons Note Sriram's comment about the additional browser_toolbar.xml files that now exist to support tablets.

Comment on attachment 627286 [details] [diff] [review] Add reader button to the browser toolbar >diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java > public ImageButton mSiteSecurity; >+ public ImageButton mReader; >+ private boolean mShowSiteSecurity; >+ private boolean mShowReader; Just an observation: Along with my desire to create a PageActions API, which would allow add-ons to add images to the URLbar like you added the "reader", I see a code-scaling problem. We are creating a specific imageview and boolean for each actions. We'll need to get to a generic system when we add the next action, or the pageaction API itself. >+ private ReaderPopup(Context context) { >+ Button readingListButton = (Button) layout.findViewById(R.id.reading_list); >+ readingListButton.setOnClickListener(new Button.OnClickListener() { What happens if the page is already on the reading list? Should the popup be ignored and "read now" be the default action if I tap the "reader" action? >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java > tab.setContentType(contentType); > tab.updateFavicon(null); > tab.updateFaviconURL(null); > tab.updateIdentityData(null); >+ tab.setReaderEnabled(false); > tab.removeTransientDoorHangers(); > tab.setAllowZoom(true); > tab.setDefaultZoom(0); > tab.setMinZoom(0); > tab.setMaxZoom(0); > tab.setHasTouchListeners(false); > tab.setCheckerboardColor(Color.WHITE); Well this block of code is screaming out for a helper method to be added to Tab. File a followup bug? > mBrowserToolbar.setTitle(uri); > mBrowserToolbar.setFavicon(null); > mBrowserToolbar.setSecurityMode(tab.getSecurityMode()); >+ mBrowserToolbar.setReaderVisibility(tab.getReaderEnabled()); > mDoorHangerPopup.updatePopup(); > mBrowserToolbar.setShadowVisibility(!(tab.getURL().startsWith("about:"))); BrowserToolbar.refresh() should be capable of doing this too. There was a patch somewhere to remove all of these patterns and just call mBrowserToolbar.refresh() > void handleDocumentStart(int tabId, final boolean showProgress, String uri) { > tab.setState("about:home".equals(uri) ? Tab.STATE_SUCCESS : Tab.STATE_LOADING); > tab.updateIdentityData(null); >+ tab.setReaderEnabled(false); Does this handle back/forward session updating too? I think it should, but just asking. >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java >+ public void addToReadingList() { >+ BrowserDB.addReadingListItem(mContentResolver, getTitle(), url); I assume this code handles duplicates (i.e. we don't want dupes in the list) >diff --git a/mobile/android/base/resources/layout/reader_popup.xml b/mobile/android/base/resources/layout/reader_popup.xml >+</LinearLayout> >\ No newline at end of file Add a newline

(In reply to Margaret Leibovic [:margaret] from comment #19) > Comment on attachment 627813 [details] [diff] [review] > Check for bookmarks should only consider normal bookmarks > > Unless one of your patches in bug 750684 handles this case, I think we're > also going to need another patch to make sure we don't show the bookmark > star on items in the awesome screen, since AwesomeBarTabs's > updateBookmarkStar method doesn't go through this method. I'm handling icons in awesome bar items in bug 757776 (yeah, more patches!).

(In reply to Margaret Leibovic [:margaret] from comment #17) > Comment on attachment 627809 [details] [diff] [review] > Factor out method to add a bookmark, generalize folder GUID argument > > Looks good to me. > > >diff --git a/mobile/android/base/db/LocalBrowserDB.java b/mobile/android/base/db/LocalBrowserDB.java > >@@ -441,52 +441,54 @@ public class LocalBrowserDB implements B > > > Uri contentUri = mBookmarksUriWithProfile; > > Not part of your patch, but this local variable seems pretty useless. I feel > like we could get rid of this and just be using mBookmarksUriWithProfile > directly. Will submit a patch now.

(In reply to Mark Finkle (:mfinkle) from comment #20) > Comment on attachment 627284 [details] [diff] [review] > Pack URL bar buttons in same layout to allow showing multiple icons > > Note Sriram's comment about the additional browser_toolbar.xml files that > now exist to support tablets. Done. Change applied to all 4 (!) browser_toolbar.xml we have now.

(In reply to Mark Finkle (:mfinkle) from comment #21) > Comment on attachment 627286 [details] [diff] [review] > Add reader button to the browser toolbar > > >diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java > > public ImageButton mSiteSecurity; > >+ public ImageButton mReader; > > >+ private boolean mShowSiteSecurity; > >+ private boolean mShowReader; > > Just an observation: > Along with my desire to create a PageActions API, which would allow add-ons > to add images to the URLbar like you added the "reader", I see a > code-scaling problem. We are creating a specific imageview and boolean for > each actions. We'll need to get to a generic system when we add the next > action, or the pageaction API itself. Totally agree. Just don't think I should block on this for now. > >+ private ReaderPopup(Context context) { > > >+ Button readingListButton = (Button) layout.findViewById(R.id.reading_list); > >+ readingListButton.setOnClickListener(new Button.OnClickListener() { > > What happens if the page is already on the reading list? Should the popup be > ignored and "read now" be the default action if I tap the "reader" action? I'd probably avoid changing the reader button behaviour dynamically but adapting the content of the popup (hiding/disabling "add to reading list" when url is already there) depending on the case is definitely something we should do. Need to check with UX team. Filed bug 760645 to track this. > >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java > > > tab.setContentType(contentType); > > tab.updateFavicon(null); > > tab.updateFaviconURL(null); > > tab.updateIdentityData(null); > >+ tab.setReaderEnabled(false); > > tab.removeTransientDoorHangers(); > > tab.setAllowZoom(true); > > tab.setDefaultZoom(0); > > tab.setMinZoom(0); > > tab.setMaxZoom(0); > > tab.setHasTouchListeners(false); > > tab.setCheckerboardColor(Color.WHITE); > > Well this block of code is screaming out for a helper method to be added to > Tab. File a followup bug? This has been replaced by a refresh() call. > > mBrowserToolbar.setTitle(uri); > > mBrowserToolbar.setFavicon(null); > > mBrowserToolbar.setSecurityMode(tab.getSecurityMode()); > >+ mBrowserToolbar.setReaderVisibility(tab.getReaderEnabled()); > > mDoorHangerPopup.updatePopup(); > > mBrowserToolbar.setShadowVisibility(!(tab.getURL().startsWith("about:"))); > > BrowserToolbar.refresh() should be capable of doing this too. There was a > patch somewhere to remove all of these patterns and just call > mBrowserToolbar.refresh() Yeah, updated the patch to use mBrowserToolbar.refresh() whenever applicable. > > void handleDocumentStart(int tabId, final boolean showProgress, String uri) { > > > tab.setState("about:home".equals(uri) ? Tab.STATE_SUCCESS : Tab.STATE_LOADING); > > tab.updateIdentityData(null); > >+ tab.setReaderEnabled(false); > > Does this handle back/forward session updating too? I think it should, but > just asking. Yep. > >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java > >+ public void addToReadingList() { > > >+ BrowserDB.addReadingListItem(mContentResolver, getTitle(), url); > > I assume this code handles duplicates (i.e. we don't want dupes in the list) Yep. We first try to update an existing entry and insert only if there isn't. > >diff --git a/mobile/android/base/resources/layout/reader_popup.xml b/mobile/android/base/resources/layout/reader_popup.xml > >+</LinearLayout> > >\ No newline at end of file > > Add a newline Done.

Comment on attachment 629295 [details] [diff] [review] Remove unnecessary local variable in addBookmarkItem() Not to scope creep here or anything, but we have the same unnecessary local variable down below in removeBookmark and removeBookmarksWithURL, so you could get rid of those, too.

(In reply to Ian Barlow (:ibarlow) from comment #15) > Created attachment 628001 [details] > mockup of reader mode menu > > Here are some updated icons for the urlbar, to make the lock, > stop/go/search, and reader mode icons feel more in line. Please replace all > of these at the same time. http://cl.ly/3O42022J1H0N2E1Y3K11 FYI: I decided to update all icons at once in a separate bug 760724, just to avoid blocking on this for now.

Here's how it looks now.

Pushed: http://hg.mozilla.org/integration/mozilla-inbound/rev/4168c98dd747 http://hg.mozilla.org/integration/mozilla-inbound/rev/315b7cb0a7ef http://hg.mozilla.org/integration/mozilla-inbound/rev/95006ffc47e1 http://hg.mozilla.org/integration/mozilla-inbound/rev/456839be8a72 http://hg.mozilla.org/integration/mozilla-inbound/rev/26a5bfa9733a http://hg.mozilla.org/integration/mozilla-inbound/rev/1330924c44f2 http://hg.mozilla.org/integration/mozilla-inbound/rev/d4a962a62d63

https://hg.mozilla.org/mozilla-central/rev/4168c98dd747 https://hg.mozilla.org/mozilla-central/rev/315b7cb0a7ef https://hg.mozilla.org/mozilla-central/rev/95006ffc47e1 https://hg.mozilla.org/mozilla-central/rev/456839be8a72 https://hg.mozilla.org/mozilla-central/rev/26a5bfa9733a https://hg.mozilla.org/mozilla-central/rev/1330924c44f2 https://hg.mozilla.org/mozilla-central/rev/d4a962a62d63

(In reply to Lucas Rocha (:lucasr) from comment #25) > (In reply to Mark Finkle (:mfinkle) from comment #20) > > Comment on attachment 627284 [details] [diff] [review] > > Pack URL bar buttons in same layout to allow showing multiple icons > > > > Note Sriram's comment about the additional browser_toolbar.xml files that > > now exist to support tablets. > > Done. Change applied to all 4 (!) browser_toolbar.xml we have now. Based on what was checked in, this was not done. Bug 761131 is tracking crashes as a result.

(In reply to Mark Finkle (:mfinkle) from comment #32) > (In reply to Lucas Rocha (:lucasr) from comment #25) > > (In reply to Mark Finkle (:mfinkle) from comment #20) > > > Comment on attachment 627284 [details] [diff] [review] > > > Pack URL bar buttons in same layout to allow showing multiple icons > > > > > > Note Sriram's comment about the additional browser_toolbar.xml files that > > > now exist to support tablets. > > > > Done. Change applied to all 4 (!) browser_toolbar.xml we have now. > > Based on what was checked in, this was not done. Bug 761131 is tracking > crashes as a result. OK, maybe I assumed too much here. The "reader" button needed to be added to all 4 browser_toolbar files too. I think you only added the linear layout to all 4 files.