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

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Blocks: 739218
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
Posted patch wip (obsolete) — Splinter Review
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?
Attachment #706384 - Flags: feedback?(mano)
(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.
Posted patch patch v1.0 (obsolete) — Splinter Review
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!
Attachment #706384 - Attachment is obsolete: true
Attachment #706384 - Flags: feedback?(mano)
Attachment #706672 - Flags: review?(mano)
Summary: nsNavHistoryContainerResultNode::MergeResults sucks → Avoid replacing nodes in Places query results
Posted patch patch v1.1 (obsolete) — Splinter Review
Fixed some trailing spaces
Attachment #706672 - Attachment is obsolete: true
Attachment #706672 - Flags: review?(mano)
Attachment #706766 - Flags: review?(mano)
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
Attachment #706766 - Flags: review?(mano) → review+
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)
Keywords: addon-compat
Posted patch patch v1.2 (obsolete) — Splinter Review
addressed comments, asking SR for the notification removal.
Attachment #706766 - Attachment is obsolete: true
Attachment #710648 - Flags: superreview?
Attachment #710648 - Flags: superreview? → superreview?(gavin.sharp)
Summary: Avoid replacing nodes in Places query results → Avoid replacing nodes in Places query results and remove nodeReplaced views notification
Posted patch patch v1.3Splinter Review
s/do_QueryInterface/do_QueryObject/
Attachment #710648 - Attachment is obsolete: true
Attachment #710648 - Flags: superreview?(gavin.sharp)
Attachment #710723 - Flags: superreview?(gavin.sharp)
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?
Attachment #710723 - Flags: superreview?(gavin.sharp) → superreview+
Oh, that is one of ithinc's addons - we can just CC him here!
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.
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/6b183775ad02
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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 :)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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)
Depends on: 874407
You need to log in before you can comment on or make changes to this bug.