The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla21

Status

()

Toolkit
Places
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

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

Trunk
mozilla21
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 2

4 years ago
Created attachment 706384 [details] [diff] [review]
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?
Attachment #706384 - Flags: feedback?(mano)
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
Created attachment 706672 [details] [diff] [review]
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!
Attachment #706384 - Attachment is obsolete: true
Attachment #706384 - Flags: feedback?(mano)
Attachment #706672 - Flags: review?(mano)
(Assignee)

Updated

4 years ago
Summary: nsNavHistoryContainerResultNode::MergeResults sucks → Avoid replacing nodes in Places query results
(Assignee)

Comment 5

4 years ago
Created attachment 706766 [details] [diff] [review]
patch v1.1

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+
(Assignee)

Comment 7

4 years ago
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
(Assignee)

Comment 8

4 years ago
Created attachment 710648 [details] [diff] [review]
patch v1.2

addressed comments, asking SR for the notification removal.
Attachment #706766 - Attachment is obsolete: true
Attachment #710648 - Flags: superreview?
(Assignee)

Updated

4 years ago
Attachment #710648 - Flags: superreview? → superreview?(gavin.sharp)
(Assignee)

Updated

4 years ago
Summary: Avoid replacing nodes in Places query results → Avoid replacing nodes in Places query results and remove nodeReplaced views notification
(Assignee)

Comment 9

4 years ago
Created attachment 710723 [details] [diff] [review]
patch v1.3

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!
(Assignee)

Comment 12

4 years ago
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.
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b183775ad02
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/6b183775ad02
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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 :)
(Assignee)

Comment 16

4 years ago
(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)

Updated

4 years ago
Depends on: 874407
You need to log in before you can comment on or make changes to this bug.