1380570 - browser_bookmark_titles.js fails with async Places Transactions turned on

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Mark Banner (:standard8)]

I spotted this during a try run for async places transactions. I've a fix in progress.

FAIL | browser/base/content/test/general/browser_bookmark_titles.js | Bookmark got a good default title.
Got , expected https://untrusted.example.com/somepage.html
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:967
chrome://mochitests/content/browser/browser/base/content/test/general/browser_bookmark_titles.js:checkBookmark:82

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1380570 - Fix indentation of browser_bookmark_titles.js.

Review commit: https://reviewboard.mozilla.org/r/156830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156830/

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1380570 - Update browser_bookmark_titles.js to use newer APIs and utility functions.

Review commit: https://reviewboard.mozilla.org/r/156832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156832/

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1380570 - Fix setting the title when bookmarking a url from an error page when the url is not found in the history.

Review commit: https://reviewboard.mozilla.org/r/156834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156834/

Comment 4

[Mark Banner (:standard8)]

Here's a try run with async transactions turned on and the changes here pushed as well:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b71c8e774f46124becb0097c1501660fc7a150d

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8886042 [details]
Bug 1380570 - Fix indentation of browser_bookmark_titles.js.

https://reviewboard.mozilla.org/r/156830/#review162204

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8886043 [details]
Bug 1380570 - Update browser_bookmark_titles.js to use newer APIs and utility functions.

https://reviewboard.mozilla.org/r/156832/#review162206

::: browser/base/content/test/general/browser_bookmark_titles.js:32
(Diff revision 1)
>    for (let i = 0; i < tests.length; ++i) {
> -    let [uri, title] = tests[i];
> +    let [url, title] = tests[i];
>  
>      let promiseLoaded = promisePageLoaded(browser);
> -    BrowserTestUtils.loadURI(browser, uri);
> +    BrowserTestUtils.loadURI(browser, url);
>      await promiseLoaded;

Should probably use await BrowserTestUtils.browserLoaded instead of a custom function.

::: browser/base/content/test/general/browser_bookmark_titles.js:57
(Diff revision 1)
>  
> -  let [uri, title] = tests[0];
> +  let [url, title] = tests[0];
>  
>    let promiseLoaded = promisePageLoaded(browser);
> -  BrowserTestUtils.loadURI(browser, uri);
> +  BrowserTestUtils.loadURI(browser, url);
>    await promiseLoaded;

ditto

::: browser/base/content/test/general/browser_bookmark_titles.js:76
(Diff revision 1)
> -async function checkBookmark(uri, expected_title) {
> -  is(gBrowser.selectedBrowser.currentURI.spec, uri,
> +async function checkBookmark(url, expected_title) {
> +  Assert.equal(gBrowser.selectedBrowser.currentURI.spec, url,
> -     "Trying to bookmark the expected uri");
> +    "Trying to bookmark the expected uri");
>  
> -  let promiseBookmark = promiseOnBookmarkItemAdded(gBrowser.selectedBrowser.currentURI);
> +  let promiseBookmark = PlacesTestUtils.waitForNotification("onItemAdded",
> +    (id, parentId, index, type, itemUrl) => itemUrl.spec == gBrowser.selectedBrowser.currentURI.spec);

itemURI.equals(gBrowser.selectedBrowser.currentURI)

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8886044 [details]
Bug 1380570 - Fix setting the title when bookmarking a url from an error page when the url is not found in the history.

https://reviewboard.mozilla.org/r/156834/#review162214

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8886042 [details]
Bug 1380570 - Fix indentation of browser_bookmark_titles.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156830/diff/1-2/

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8886043 [details]
Bug 1380570 - Update browser_bookmark_titles.js to use newer APIs and utility functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156832/diff/1-2/

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8886044 [details]
Bug 1380570 - Fix setting the title when bookmarking a url from an error page when the url is not found in the history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156834/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4fb58ba50e5
Fix indentation of browser_bookmark_titles.js. r=mak
https://hg.mozilla.org/integration/autoland/rev/e664a64cb729
Update browser_bookmark_titles.js to use newer APIs and utility functions. r=mak
https://hg.mozilla.org/integration/autoland/rev/c3beaef116ef
Fix setting the title when bookmarking a url from an error page when the url is not found in the history. r=mak

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4fb58ba50e5
https://hg.mozilla.org/mozilla-central/rev/e664a64cb729
https://hg.mozilla.org/mozilla-central/rev/c3beaef116ef