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

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Florian Quèze [:florian]]

Attachment: Patch

Currently that big "+" button does nothing, due to some code not being implemented yet and some bugs in the current code.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0008-Bug-1142217-Make-the-button-in-reader-view-work.-r.patch

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)

Comment 2

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 3

[Florian Quèze [:florian]]

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.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)

As discussed in IRC, I'm on-board with all of that!

Comment 5

[Florian Quèze [:florian]]

(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.

Comment 6

[Florian Quèze [:florian]]

Attachment: Updated patch

Patch updated to take into account comments 1 to 5.

Comment 7

[Florian Quèze [:florian]]

Attachment: Using message managers instead of ReadingList listeners

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).

Comment 8

[Drew Willcoxon :adw]

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.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 10

[Drew Willcoxon :adw]

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?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/90abee1a4bc0

Comment 13

[Marco Mucci [:MarcoM]]

Hi Florian, can you provide a point value.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/5e2220ad9486 for bc1 orange:

https://treeherder.mozilla.org/logviewer.html#?job_id=2255851&repo=fx-team

It also hit the e10s variant of bc1 with the same error.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0007-Bug-1142217-Make-the-button-in-reader-view-work.-r-m.patch

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

Comment 16

[Mark Hammond [:markh] [:mhammond]]

oops - sorry Marco, I thought the needinfo was due to the backout!

Comment 17

[Mark Hammond [:markh] [:mhammond]]

try looks green

https://hg.mozilla.org/integration/fx-team/rev/f3fd3b32f8a4

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3fd3b32f8a4

Comment 19

[Andrei Vaida [:avaida]]

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

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c8f5fabac369

Comment 21

[Andrei Vaida [:avaida]]

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.