Closed Bug 1142217 Opened 5 years ago Closed 5 years ago

Make the Add/Remove to Reading List toggle button of the reader view actually work

Categories

(Firefox Graveyard :: Reading List, defect)

defect
Not set

Tracking

(firefox38 verified, firefox39 verified)

VERIFIED FIXED
Firefox 39
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: florian, Assigned: florian)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Currently that big "+" button does nothing, due to some code not being implemented yet and some bugs in the current code.
Attachment #8576195 - Flags: review?(bmcbride)
Thanks for the patch Florian!

I was chatting with Unfocused, and we agreed that we could avoid most of the hacks to ReadingList.jsm by adding a new method to fetch an item given a URL.  I stole that part of the patch from bug 1131457. Drew, can you please have a look at that?

Also, for similar reasons to what I mentioned in bug 1131457 comment 8, I'm not sure this module should start growing knowledge about the message manager messages being sent around. Given the readinglist module already support listeners, I figured it was better to have this add such a listener and do the broadcasts there.  What do you think?

However, this exposed a problem with trying to use readinglist items via the message manager - see the "_unserializable" comments in the patch. Without this hack, the call to |mm.broadcastAsyncMessage("Reader:Added", item);| did the right thing (ie, the message handler saw what it expected) but the call to |mm.broadcastAsyncMessage("Reader:Removed", item);| didn't work as expected - the message handler saw the "raw" ReadingListItem object, ie, something looking like {"_properties": {...}, "_id": "xxxxx", "list": null}. Drew, what do you think about the approach I took?  (I guess an alternative would be to call onItemDeleted *before* setting .list to null, but then we'd probably need a .deleted attribute too or something)
Attachment #8576464 - Flags: feedback?(florian)
Attachment #8576464 - Flags: feedback?(bmcbride)
Attachment #8576464 - Flags: feedback?(adw)
Comment on attachment 8576464 [details] [diff] [review]
0008-Bug-1142217-Make-the-button-in-reader-view-work.-r.patch

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

::: browser/components/readinglist/ReadingList.jsm
@@ +272,5 @@
>      this._listeners.delete(listener);
>    },
>  
>    /**
> +   * Normalize a URI, stripping away extraneous parts we don't want to store

hmm - re-looking at this, I think this should probably just be a private helper rather than exposed on the object?
Flags: qe-verify+
QA Contact: andrei.vaida
Comment on attachment 8576464 [details] [diff] [review]
0008-Bug-1142217-Make-the-button-in-reader-view-work.-r.patch

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

(In reply to Mark Hammond [:markh] from comment #1)

> I'm not sure this module should start growing knowledge about the message
> manager messages being sent around. Given the readinglist module already
> support listeners, I figured it was better to have this add such a listener
> and do the broadcasts there.  What do you think?

I also considered adding a listener in the ReaderParent.jsm file like you did. The reason I didn't is that I am (was?) hoping we could delay loading the ReadingList.jsm file until the user actually does something that requires it.

Also, is there a reason why the support for these listeners is needed? Could we just use the message manager for the sidebar too?

::: browser/components/readinglist/ReadingList.jsm
@@ +282,5 @@
> +  normalizeURI(uri) {
> +    if (typeof uri == "string") {
> +      uri = Services.io.newURI(uri, "", null);
> +    }
> +    uri = uri.clone();

Maybe you want to use nsIURI's cloneIgnoringRef method?

@@ +398,5 @@
> +  // sees the "raw" object - ie, it sees "_properties" etc.
> +  // We work around this problem by *always* having an unserializable property
> +  // on the object - this way the implicit .toJSON call is always made, even
> +  // when |this.list| is null.
> +  this._unserializable = _unserializable;

Would adding a circular this._ref = this; reference be enough to make it unserializable?

::: browser/modules/ReaderParent.jsm
@@ +13,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode", "resource://gre/modules/ReaderMode.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ReadingList", "resource:///modules/readinglist/ReadingList.jsm");

I think loading ReadingList lazily becomes useless (and just adds overhead) if you are going to use it in the init method.
Attachment #8576464 - Flags: feedback?(florian) → feedback+
(In reply to Florian Quèze [:florian] [:flo] from comment #3)

As discussed in IRC, I'm on-board with all of that!
(In reply to Florian Quèze [:florian] [:flo] from comment #3)

> Also, is there a reason why the support for these listeners is needed? Could
> we just use the message manager for the sidebar too?

This change is large enough that I think it should be a separate patch; possibly in a separate bug.

> > +  // We work around this problem by *always* having an unserializable property
> > +  // on the object - this way the implicit .toJSON call is always made, even
> > +  // when |this.list| is null.
> > +  this._unserializable = _unserializable;
> 
> Would adding a circular this._ref = this; reference be enough to make it
> unserializable?

Well, actually a circular reference is unserializable, but is still transferable, so that didn't work.
Attached patch Updated patch (obsolete) — Splinter Review
Patch updated to take into account comments 1 to 5.
Attachment #8576195 - Attachment is obsolete: true
Attachment #8576464 - Attachment is obsolete: true
Attachment #8576195 - Flags: review?(bmcbride)
Attachment #8576464 - Flags: feedback?(bmcbride)
Attachment #8576464 - Flags: feedback?(adw)
Attachment #8576951 - Flags: review?(mhammond)
This patch doesn't work, the messages aren't received on the sidebar.js side. I don't know why. I'm attaching this mostly so that this attempt doesn't get lost; I'm not sure we should further pursue this in this bug (unless it's not working only due to a trivial mistake that can easily be corrected).
Attachment #8576951 - Flags: review?(mhammond) → review+
Comment on attachment 8576951 [details] [diff] [review]
Updated patch

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

::: browser/components/readinglist/ReadingList.jsm
@@ +401,5 @@
> +  // is null, the item *can* be directly serialized - so the message handler
> +  // sees the "raw" object - ie, it sees "_properties" etc.
> +  // We work around this problem by *always* having an unserializable property
> +  // on the object - this way the implicit .toJSON call is always made, even
> +  // when |this.list| is null.

You should just send item._properties instead of item.
(In reply to Drew Willcoxon :adw from comment #8)
> You should just send item._properties instead of item.

But then listeners can't use the item as a "real" item, which I felt was a bit of a deal-breaker - eg, onItemAdded() can't just keep the item locally and get the "live" updates etc.
But how can message manager listeners see the real item at all?  The item is serialized and deserialized into a new JS object across the message manager boundary, right?
(In reply to Drew Willcoxon :adw from comment #10)
> But how can message manager listeners see the real item at all?  The item is
> serialized and deserialized into a new JS object across the message manager
> boundary, right?

So in my version of the patch, an onItemAdded() listener was used to "relay" the item to the message manager.  IMO it doesn't make sense for the listener to know there is a ._properties attribute it should use on onItemDeleted but not in onItemAdded etc.

I agree it might make sense to send ._properties for messages being sent directly in that module, but that still leaves the underlying issue that future listeners might need to know to use ._properties in some contexts but not in the other.  I think there's value in the item object working consistently whether it's deleted or not.

OTOH, I wonder if we should abandon the live-list completely - as you mention, the object can't survive the cross-process boundaries, so I wonder who we are actually helping here.
Hi Florian, can you provide a point value.
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Flags: needinfo?(florian)
Flags: firefox-backlog+
Blocks: 1132074
We need to check the messageManager is non-null like the code above. This patch fixes this. Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=38fbf899bec5
Attachment #8576951 - Attachment is obsolete: true
Flags: needinfo?(florian)
Attachment #8577042 - Flags: review+
Flags: needinfo?(florian)
oops - sorry Marco, I thought the needinfo was due to the backout!
Points: --- → 3
Flags: needinfo?(florian)
https://hg.mozilla.org/mozilla-central/rev/f3fd3b32f8a4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Verified fixed on Nightly 39.0a1 (2015-03-15) using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Verified fixed on Aurora 38.0a2 (2015-03-29) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.