752217 - Replace usage of setPageTitle with updatePlaces

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Some tests are using nsIGlobalHistory2::SetPageTitle Places implementation, we want to stop implementing nsIGlobalHistory2, so we should replace those calls.

mxr.mozilla.org/mozilla-central/search?string=setPageTitle&find=.js%24

If it may be hard to make some test async, we can temporarily spin the events loop (see for example the utilities used in sync source code to do that).

Comment 1

[Raymond Lee [:raymondlee]]

(In reply to Marco Bonardo [:mak] from comment #0)
> 
> If it may be hard to make some test async, we can temporarily spin the
> events loop (see for example the utilities used in sync source code to do
> that).

Could you point to a file which I can see the example you mentioned please?

Comment 2

[Marco Bonardo [:mak]]

http://mxr.mozilla.org/mozilla-central/source/services/common/async.js#130
the helper makeSpinningCallback, you can see how it's being used here
http://mxr.mozilla.org/mozilla-central/ident?i=makeSpinningCallback

Btw, this must be used as a last resource when the test is really complicated and should be completely rewritten to be async.

Comment 3

[Marco Bonardo [:mak]]

as an additional hint, many places tests now use add_task instead of add_test, the difference is that add_task allows to easily use Promise and yield. So you could easily create a promiseSetTitle function and yield promiseSetTitle(aURI, aTitle); inside an add_task test. for examples you may see some of the patches pushed as dependencies of bug 700250.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v1 (WIP)

I've created promiseSetTitle function and use yield promiseSetTitle(aURI, aTitle); to replace usage of setPageTitle.  

This works fine in test_000_frecency.js and test_history_observer.js.  However, inside test_history_queries_titles_liveUpdate.js, those loops with yield promiseSetTitle() inside stop working.  I have spent some time to debug but without luck.

Marco: can you spot any reasons why that happens? thanks!

Comment 5

[Marco Bonardo [:mak]]

At first glance I suspect the problem may be testQueryContents invokes the callback internally, maybe you may try removing that helper, just embedding its code in the first test-case and see if it passes that way (comment all of the other tests just to verify if the first one proceeds).

As a side note, setting the title this way we also add a visit, and that may change behavior of some tests, better to name that promiseAddVisitWithTitle for now.
If some test should never need to set title without a visit we will still be able to do that with a custom async query to the database.

Comment 6

[Marco Bonardo [:mak]]

ehr, even better you can just use promiseAddVisits and provide a "title" property in the object describing the page... so this promiseSetTitle is not needed!

Comment 7

[Marco Bonardo [:mak]]

yes I think that makes sense:
1. all cases where you can set a title AND add a visit, just use promiseAddVisits providing the title
2. cases where you need to set a title but if adding a visit things break or the test is no more valid, create promiseSetTitle that runs an async query on DBConn(). These should be rare though.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v2 (WIP)

I have switched to use the promiseAddVisits for other tests. For test_history_queries_titles_liveUpdate.js, I have moved the internal callback out and it works but nodes don't get updated about the title changes.  PlacesUtils.history.setPageTitle probably fires a notification for observers to update the relevant node (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/History.cpp#811), however, I think PlacesUtils.asyncHistory.updatePlaces doesn't do that.  Am I missing something?

Comment 9

[Marco Bonardo [:mak]]

updatePlaces should fire a title change notification, _if the title changes_. Though promiseSetTitle won't since it's just a query, so if you use that, the results won't live update. On the other side if you set the title with promiseSetTitle and then updatePlaces it with the same title, it won't yet notify a title change cause the title stays the same.

that notification you found in History.cpp is hit also by updatePlaces, so both notify.

What's the test where updatePlaces doesn't fire the notification? Maybe I could debug it and see why it doesn't fire.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3 (WIP)

Marco: please try run test_history_queries_titles_liveUpdate.js and see below comment

 add_task(function pages_query()
 {
   let [query, options] = newQueryWithOptions();
-  testQueryContents(query, options, function (root) {
-    compareArrayToResult([gTestData[0], gTestData[1], gTestData[2]], root);
-    for (let i = 0; i < root.childCount; i++) {
-      let node = root.getChild(i);
-      let uri = NetUtil.newURI(node.uri);
-      do_check_eq(node.title, gTestData[i].title);
-      PlacesUtils.history.setPageTitle(uri, "changedTitle");
-      do_check_eq(node.title, "changedTitle");
-      PlacesUtils.history.setPageTitle(uri, gTestData[i].title);
-      do_check_eq(node.title, gTestData[i].title);
-    }
-  });
+  let root = PlacesUtils.history.executeQuery(query, options).root;
+  root.containerOpen = true;
+  compareArrayToResult([gTestData[0], gTestData[1], gTestData[2]], root);
+  for (let i = 0; i < root.childCount; i++) {
+    let node = root.getChild(i);
+    let uri = NetUtil.newURI(node.uri);
+    do_check_eq(node.title, gTestData[i].title);
+    yield promiseAddVisits({uri: uri, title: "changedTitle"});

>> This line update the title and notification should be fired. However, the below check doesn't pass, the node.title is still "title1", not "changedTitle"

+    do_check_eq(node.title, "changedTitle");
+    yield promiseSetTitle(uri, gTestData[i].title);
+  }
+  root.containerOpen = false;
 });


Thanks!

Comment 11

[Marco Bonardo [:mak]]

sorry for late, my build was broken by something that landed today and I had to figure out what.
I can reproduce the failure indeed, debugging it.

Comment 12

[Marco Bonardo [:mak]]

So, actually we notify properly, and the result updates, but then something strange happens:
1. we replace the node, so after addvisit we have node != root.getChild(i) (node is basically a zombie now)
2. we change sorting of nodes to 2,1,3 after the first loop, when we change title of the second node

At first glance none of the 2 is expected, though I have to verify directly in the backend which code path we are taking and why. This is most likely a bug in nsNavHistoryResult.cpp. Will bring on investigation and try to come with a patch (hope tomorrow).

In the meanwhile, bug 752218 is similar if you are looking for this kind of bugs to help refactoring with updatePlaces.

Comment 13

[Marco Bonardo [:mak]]

may we start updating the tests that don't have issue here, and just leave test_history_queries_titles_liveUpdate.js for bug 831094?

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v4

Leave test_history_queries_titles_liveUpdate.js for bug 831094.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 704755 [details] [diff] [review]
v4

Review of attachment 704755 [details] [diff] [review]:
-----------------------------------------------------------------

thanks

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=2dbb4a4465e1

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Fixed the commit description

Comment 18

[Raymond Lee [:raymondlee]]

Passed try

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73c201cb0912

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73c201cb0912