831094 - Avoid replacing nodes in Places query results and remove nodeReplaced views notification

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

basically, it always replaces nodes, when often it could just update them, that means:
let root = PlacesUtils.history.executeQuery(query, options).root; root.containerOpen = true;
let node = root.getChild(0);
// add a visit or change title or whatever...
node != root.getChild(0);

moreover if this is an history query with SORT_BY_NONE sorting, insertions can change sorting order of entries (are likely done in the wrong place).

see https://bugzilla.mozilla.org/show_bug.cgi?id=752217#c12

Both are totally unpredictable for a consumer, even if it always worked like this.

Comment 1

[Raymond Lee [:raymondlee]]

Please also Replace usage of setPageTitle with updatePlaces for test_history_queries_titles_liveUpdate.js.  See this of reference: https://bugzilla.mozilla.org/attachment.cgi?id=702281

Comment 2

[Marco Bonardo [:mak]]

Attachment: wip

This is incomplete, I'm still investigating a failure in searchTerms liveupdating, the node gets wrongly removed even if it matches the searchTerm, but that looks unrelated to my changes, could be I found an existing bug, but I have no more time right now, will investigate later.

Apart this, I quite like the patch since we can get rid of nodeReplaced.

Mano, regarding nodeReplaced, could you please take a look if I may have missed something important?

Comment 3

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [:mak] from comment #2)
> This is incomplete, I'm still investigating a failure in searchTerms
> liveupdating, the node gets wrongly removed even if it matches the
> searchTerm

looks like somehow it ends up having hidden = 1, so it's properly removed since the query doesn't have includeHidden. though that hidden value is unexpected, investigating further.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I figured the "bug" was me not figuring out I was speaking to a RefPtr, not an object.

Btw, this passes all tests and looks good, so far.  I'm quite happy about it, it may cause regressions, but I'll be even happier when we'll have new tests for such regressions!

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Fixed some trailing spaces

Comment 6

[Asaf Romano (gone)]

Comment on attachment 706766 [details] [diff] [review]
patch v1.1

>     case QUERYUPDATE_HOST: {
...
>-      NS_ENSURE_SUCCESS(rv, rv);
>+      MOZ_ASSERT(mQueries.Count() == 1,
>+                 "Host updated queries can have only one object");
>+      nsCOMPtr<nsNavHistoryQuery> query = do_QueryInterface(mQueries[0]);

nsRefPtr

>+      NS_ENSURE_STATE(query);

unnecessary.


>     case QUERYUPDATE_TIME: {
...
>+      nsCOMPtr<nsNavHistoryQuery> query = do_QueryInterface(mQueries[0]);
>+      NS_ENSURE_STATE(query);
> 

ditto

Comment 7

[Marco Bonardo [:mak]]

adding addon-compat for the idl removal, should not affect many fwiw since this is an API used by the js views and the worst thing that may happen is that it won't be called (though view will keep working cause we invoke alternative notifications)

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.2

addressed comments, asking SR for the notification removal.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.3

s/do_QueryInterface/do_QueryObject/

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 710723 [details] [diff] [review]
patch v1.3

The only add-on I could find that _may_ be impacted (I didn't look too closely) is:
https://mxr.mozilla.org/addons/source/59961/chrome/chrome/content/tabutils.xml#91

Worth reaching out to that author specifically?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Oh, that is one of ithinc's addons - we can just CC him here!

Comment 12

[Marco Bonardo [:mak]]

I think nothing should be really "affected", nodeReplaced just notify that if you have an handle on such node it's becoming useless, we don't change the handles anymore, so that should just keep working.

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b183775ad02

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b183775ad02

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #12)
> I think nothing should be really "affected", nodeReplaced just notify that
> if you have an handle on such node it's becoming useless, we don't change
> the handles anymore, so that should just keep working.

The code I linked to seemed to be monkeypatching existing listener methods - if we remove our implementation of the method that might break it. But like I said, I didn't look too closely :)

Comment 16

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> The code I linked to seemed to be monkeypatching existing listener methods -
> if we remove our implementation of the method that might break it. But like
> I said, I didn't look too closely :)

yes it's intercepting the calls, the fact is that there is no call anymore, so whatever it does shouldn't break (unless it's really exotic)