Last Comment Bug 831094 - Avoid replacing nodes in Places query results and remove nodeReplaced views notification
: Avoid replacing nodes in Places query results and remove nodeReplaced views n...
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Marco Bonardo [::mak]
:
Mentors:
Depends on: 874407
Blocks: 739218 752217
  Show dependency treegraph
 
Reported: 2013-01-15 16:54 PST by Marco Bonardo [::mak]
Modified: 2013-05-22 13:43 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (38.59 KB, patch)
2013-01-25 07:17 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.0 (51.80 KB, patch)
2013-01-25 16:48 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (51.80 KB, patch)
2013-01-26 01:43 PST, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Splinter Review
patch v1.2 (51.75 KB, patch)
2013-02-06 05:44 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.3 (51.75 KB, patch)
2013-02-06 08:05 PST, Marco Bonardo [::mak]
gavin.sharp: superreview+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2013-01-15 16:54:24 PST
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] 2013-01-21 19:47:29 PST
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] 2013-01-25 07:17:49 PST
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?
Comment 3 Marco Bonardo [::mak] 2013-01-25 14:46:13 PST
(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] 2013-01-25 16:48:52 PST
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!
Comment 5 Marco Bonardo [::mak] 2013-01-26 01:43:33 PST
Created attachment 706766 [details] [diff] [review]
patch v1.1

Fixed some trailing spaces
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-02-06 04:49:59 PST
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] 2013-02-06 05:36:16 PST
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] 2013-02-06 05:44:16 PST
Created attachment 710648 [details] [diff] [review]
patch v1.2

addressed comments, asking SR for the notification removal.
Comment 9 Marco Bonardo [::mak] 2013-02-06 08:05:02 PST
Created attachment 710723 [details] [diff] [review]
patch v1.3

s/do_QueryInterface/do_QueryObject/
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-06 09:51:42 PST
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: gavin@gavinsharp.com] 2013-02-06 09:52:06 PST
Oh, that is one of ithinc's addons - we can just CC him here!
Comment 12 Marco Bonardo [::mak] 2013-02-06 09:56:09 PST
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 14 Ryan VanderMeulen [:RyanVM] 2013-02-06 18:09:09 PST
https://hg.mozilla.org/mozilla-central/rev/6b183775ad02
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-07 11:21:04 PST
(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] 2013-02-07 12:29:12 PST
(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)

Note You need to log in before you can comment on or make changes to this bug.