The ReaderMode controls have a "Open Reading List" button - on desktop, this doesn't currently do anything. It should open the sidebar. On the chrome side of this, it just needs to call the API added in bug 1123517: ReadingListUI.showSidebar(); See browser-readinglist-js for that object, as we may want that button to toggle the sidebar and/or show depressed when the sidebar is open.

Scoped out the basic portion, just opens the sidebar ... depends on bug 1123517 going in first of course Haven't caught up to conversation in bug 1124011 yet so not sure timing wise when you'd want this or how it fits with the current plans.

Comment on attachment 8566162 [details] [diff] [review] bug1133489.diff may as well get it in the review queue

Comment on attachment 8566162 [details] [diff] [review] bug1133489.diff Review of attachment 8566162 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/reader/AboutReader.jsm @@ +283,5 @@ > }, > > + // Toggle Sidebar based on current status. > + _onSidebarListToggle: function() { > + this._mm.sendAsyncMessage("Reader:ShowList"); Nit: This should probably the "Reader:ToggleList" if it's toggling the list (rather than just showing it). And then we could add a method to handle the toggling in browser-readinglist.js.

Tweaked a bit.

Comment on attachment 8566162 [details] [diff] [review] bug1133489.diff Review of attachment 8566162 [details] [diff] [review]: ----------------------------------------------------------------- Just re-landed bug 1123517, FWIW. Should stick this time. Although I did bitrot this a little - sorry! Tests? ::: toolkit/components/reader/AboutReader.jsm @@ +283,5 @@ > }, > > + // Toggle Sidebar based on current status. > + _onSidebarListToggle: function() { > + this._mm.sendAsyncMessage("Reader:ShowList"); So, checking the mockup, the state of this button should reflect whether the sidebar is open or not: https://projects.invisionapp.com/share/NK1ZBQ6SY#/screens/60126996 (And what Margaret said: Should toggle.)

Comment on attachment 8566162 [details] [diff] [review] bug1133489.diff Ugh, bugzilla.

Comment on attachment 8566368 [details] [diff] [review] bug1133489.diff Review of attachment 8566368 [details] [diff] [review]: ----------------------------------------------------------------- Comment 3 and comment 5 still apply here.

Oh ha! I didn't see margaret sneak in the drive-by ... I was kind of hoping we didn't want to toggle (versus just open) nor to maintain an actually useful button state. (I remember going through this for the Android version back when :-) ) Here's a closer approximation to what I think you'll want. I'm still dogfooding, testing permutations of object open/close, View->Sidebar toggle, tab/switch/navigate and etc. but so far fingers crossed. Tests ???? mmmmmm ...

Screen snips: https://www.dropbox.com/s/wj3o5pl8yq4bqb0/bug1133489_NotReading.png?dl=0 https://www.dropbox.com/s/j21y3j1d1x6q833/bug1133489_Reading.png?dl=0

You'll want to base this on the patch for bug 1131303 - it adds strings for the tooltip: * aboutReader.toolbar.openReadingList * aboutReader.toolbar.closeReadingList (Sorry, lots of churn at the moment - rushing to get strings landed before the merge)

Not an issue ... simple rebase.

Comment on attachment 8566909 [details] [diff] [review] bug1133489.diff Review of attachment 8566909 [details] [diff] [review]: ----------------------------------------------------------------- Would be good to have a test for this, in browser/components/readinglist/test/browser. The existing tests in there should help - ReadingListTestUtils has various utility functions to make it easier. ::: browser/base/content/browser-readinglist.js @@ +91,5 @@ > + > + /** > + * Respond to messages. > + */ > + receiveMessage: function(message) { Can use the newer style method definition syntax here too: receiveMessage(message) { @@ +102,5 @@ > + break; > + } > + > + case "ReadingList:ToggleVisibility": { > + if (this.enabled) { Hm, this check should probably be moved into toggleSidebar() and showSidebar() ::: toolkit/components/reader/AboutReader.jsm @@ +299,5 @@ > }, > > + /* > + * Toggle ReadingList Sidebar visibility. SidebarUI will trigger > + * _updateListButtonStyle(). Nit: Our documentation comments are rather a mess throughout the tree, used inconsistently and in numerous formats. I've been trying to push that along to a more consistent state - there's already a vague leaning towards JSDoc (http://usejsdoc.org/) formatted documentation comments (and I'm using that everywhere now). Would be good to have any new comments here use that too. Which in these cases, just means starting the comment with "/**". Documenting params using @param is useful too. @@ +313,5 @@ > + let handleToggleStatus = (message) => { > + this._mm.removeMessageListener("ReadingList:VisibilityStatus", handleToggleStatus); > + this._updateListButtonStyle(message.data.isOpen); > + }; > + this._mm.addMessageListener("ReadingList:VisibilityStatus", handleToggleStatus); This isn't safe for mobile, since these messages aren't supported by the parent process. You're adding a listener for the "ReadingList:VisibilityStatus" message, which will never get sent on mobile. So every time page visibility changes, it'll add a new listener and never remove it. It's a new listener because the `handleToggleStatus` function is re-defined every time this function is called. As long as this isn't requiring that "ReadingList:VisibilityStatus" gets received, this will be fine. So if you use `this` as the message handler (letting it be handled in receiveMessage), it'll be safe - the listener won't be added multiple times. And, if receiveMessage is handling that message, I think this can be refactored - you won't need _updateListButton. The listener for "ReadingList:VisibilityStatus" can just be added/removed in constructor/unload. ::: toolkit/locales/en-US/chrome/global/aboutReader.properties @@ +36,1 @@ > aboutReader.toolbar.closeReadingList=Close Reading List Heh, think you mis-understood my comment about this. You should be using the strings that are already here (aboutReader.toolbar.openReadingList and aboutReader.toolbar.closeReadingList) - they were pre-landed for this bug to ensure they're in the tree before the merge. Relevant string should be used based on whether the sidebar is current open or not, of course.

re; |This isn't safe for mobile| ... yah, I'd decided we were protected on the mobile side by the nothing triggering _updateListButton() ... but forgot about the initial call that always happens. I wonder how you feel about this solution? It feels like pre-processor overkill, but basically it's correct, as Readinglist on mobile is no longer provided in the banner.

Simple ReadingList test.

folded and rebased on m-c

Comment on attachment 8567640 [details] [diff] [review] bug1133489.diff Review of attachment 8567640 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, sorry, I somehow didn't add one important thing I meant to mention in the previous review: We need to try to avoid pre-processor directives. This recent firefox-dev thread is relevant: https://mail.mozilla.org/pipermail/firefox-dev/2015-February/002689.html But TL;DR is that we need to invest in moving away from pre-processor directives, as they break all JS tooling (linters, refactorers, code analysis, etc etc). We're missing out on a huge amount of potential gains from various JS tooling, a large part due to use of pre-processor directives everywhere. Did the solution I proposed in comment 12 not work out? (I don't think we need to worry about the overhead of one unused message on mobile) ::: browser/base/content/test/general/browser_readerMode.js @@ +58,5 @@ > + // After we click ReadingList button, status should be "open". > + listButton.click(); > + yield promiseWaitForCondition(() => listButton.classList.contains("on")); > + ok(listButton.classList.contains("on"), > + "List button should now indicate SideBar-ReadingList open."); Check ReadingListUI.isSidebarOpen too ?

re; |Did the solution I proposed in comment 12 not work out?| It works fine :-) tl;dr I didn't like code sending requests that aren't always responded to, so I wrote my next patch to mobile/.../Reader.js to catch and reply in the negative to close the loop. Then I really crawled out on a limb and decided the super-optimized version just avoids the whole thing and avoids wasted .apk size ... Let's go with your best thought. I just added a few descriptive comments.

Unfocused, what's your plan for uplifting the reading list UI work to 38? We should coordinate, since this overlaps with the work I've been doing in AboutReader.jsm. My plan was to just wait until the dust settles on all the changes in 39 (except for changes with strings), then uplift everything all together.

(Discussed on IRC)

I just uplifted this myself, since some more patches I needed to uplift depended on it: https://hg.mozilla.org/releases/mozilla-aurora/rev/da4f8b182959

Verified fixed on Nightly 39.0a1 (2015-03-08) and Aurora 38.0a2 (2015-03-08), using Windows 7 (x64), Ubuntu 14.04 (x86) and Mac OS X 10.9.5.