Closed Bug 1133489 Opened 5 years ago Closed 5 years ago

Hook up "Open ReadingList" button in desktop ReaderMode

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
mozilla39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: Unfocused, Assigned: capella)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Blocks: 1132074
Attached patch bug1133489.diff (obsolete) — Splinter Review
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
Attachment #8566162 - Flags: review?(bmcbride)
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.
Attachment #8566162 - Flags: feedback+
Assignee: nobody → markcapella
Attached patch bug1133489.diff (obsolete) — Splinter Review
Tweaked a bit.
Attachment #8566162 - Attachment is obsolete: true
Attachment #8566162 - Flags: review?(bmcbride)
Attachment #8566368 - Flags: review?(bmcbride)
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.)
Attachment #8566162 - Attachment is obsolete: false
Comment on attachment 8566162 [details] [diff] [review]
bug1133489.diff

Ugh, bugzilla.
Attachment #8566162 - Attachment is obsolete: true
Comment on attachment 8566368 [details] [diff] [review]
bug1133489.diff

Review of attachment 8566368 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 3 and comment 5 still apply here.
Attachment #8566368 - Flags: review?(bmcbride) → review-
Attached patch bug1133489.diff (obsolete) — Splinter Review
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 ...
Attachment #8566368 - Attachment is obsolete: true
Attachment #8566556 - Flags: review?(bmcbride)
No longer blocks: 1124011
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)
Attached patch bug1133489.diff (obsolete) — Splinter Review
Not an issue ... simple rebase.
Attachment #8566556 - Attachment is obsolete: true
Attachment #8566556 - Flags: review?(bmcbride)
Attachment #8566909 - Flags: review?(bmcbride)
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.
Attachment #8566909 - Flags: review?(bmcbride) → review-
Attached patch bug1133489.diff (obsolete) — Splinter Review
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.
Attachment #8566909 - Attachment is obsolete: true
Attachment #8566974 - Flags: review?(bmcbride)
Attached patch bug1133489_test.diff (obsolete) — Splinter Review
Simple ReadingList test.
Attachment #8567065 - Flags: review?(bmcbride)
Attached patch bug1133489.diff (obsolete) — Splinter Review
folded and rebased on m-c
Attachment #8566974 - Attachment is obsolete: true
Attachment #8567065 - Attachment is obsolete: true
Attachment #8566974 - Flags: review?(bmcbride)
Attachment #8567065 - Flags: review?(bmcbride)
Attachment #8567640 - Flags: review?(bmcbride)
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 ?
Attachment #8567640 - Flags: review?(bmcbride) → review-
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.
Status: NEW → ASSIGNED
Attached patch bug1133489.diffSplinter Review
Attachment #8567640 - Attachment is obsolete: true
Attachment #8568426 - Flags: review?(bmcbride)
Attachment #8568426 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/0ca14dd6fa2a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Iteration: --- → 39.1 - 9 Mar
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.
Flags: needinfo?(bmcbride)
(Discussed on IRC)
Flags: needinfo?(bmcbride)
QA Contact: andrei.vaida
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.