Closed Bug 1402707 Opened 4 years ago Closed 4 years ago

Rewrite browser_library*middleclick.js to use Places' async APIs

Categories

(Firefox :: Bookmarks & History, enhancement, P2)

Product:
Firefox
Component:
Bookmarks & History
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

Bug 1402707 - Change BrowserTestUtils.waitForNewTab to allow waiting for tabs to open a new URL when the tab might not be the next one opened.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1402707 - Rewrite browser_library_*_middleclick.js to use Places' async APIs.
59 bytes, text/x-review-board-request
mak
: review+
Details 
As part of the ongoing rewrite of Places' unit tests to use the async APIs, we need to do the same for the browser_library*middleclick.js tests.

We can also remove a lot of tab listener code for these tests if we make a slight alteration to BrowserTestUtils.waitForNewTab that allows it to wait for tab opens that aren't the first one after it is called.

Hence splitting this out to a separate bug from bug 1094864 due to the wider impact.
Somehow this has decided to leak places.xul on shutdown. I'll take a look at that on Monday.
Comment on attachment 8911593 [details]
Bug 1402707 - Change BrowserTestUtils.waitForNewTab to allow waiting for tabs to open a new URL when the tab might not be the next one opened.

Thinking about it, some tests might rely on waitForNewTab to detect if it isn't the next tab for test failure purposes.

I think I should make it an option.
Attachment #8911593 - Flags: review?(mak77)
Attachment #8911594 - Flags: review?(mak77)
It looks like there's still memory leaks here. I'll have to have another think about what that could be.
Attachment #8911593 - Flags: review?(mak77)
Attachment #8911594 - Flags: review?(mak77)
For some reason the leak is still happening even if I null out gLibrary.

I can't see anything obvious here at the moment, so I'm going to put it on the back burner for a few days, whilst I look at other items.
Blocks: 1068032
Priority: P1 → P2
I think I've now fixed the leak issues. I've triggered a try build for the new patch.
Try runs are now green :-)
Comment on attachment 8911593 [details]
Bug 1402707 - Change BrowserTestUtils.waitForNewTab to allow waiting for tabs to open a new URL when the tab might not be the next one opened.

https://reviewboard.mozilla.org/r/183014/#review202288
Attachment #8911593 - Flags: review?(mak77) → review+
Comment on attachment 8911594 [details]
Bug 1402707 - Rewrite browser_library_*_middleclick.js to use Places' async APIs.

https://reviewboard.mozilla.org/r/183016/#review202308

::: browser/components/places/tests/browser/browser_library_left_pane_middleclick.js:17
(Diff revision 4)
> -var gTests = [];
> -var gCurrentTest = null;
>  
> -// Listener for TabOpen and tabs progress.
> -var gTabsListener = {
> -  _loadedURIs: [],
> +add_task(async function test_setup() {
> +  // Temporary disable history, so we won't record pages navigation.
> +  Services.prefs.setBoolPref(ENABLE_HISTORY_PREF, false);

use await SpecialPowers.pushPrefEnv?

::: browser/components/places/tests/browser/browser_library_middleclick.js:21
(Diff revision 4)
> -      is(gBrowser.tabs.length, gCurrentTest.URIs.length + 1,
> -         "We have opened " + gCurrentTest.URIs.length + " new tab(s)");
> -    }
>  
> -    var tab = aEvent.target;
> -    is(tab.ownerGlobal, window,
> +  // Temporary disable history, so we won't record pages navigation.
> +  Services.prefs.setBoolPref(ENABLE_HISTORY_PREF, false);

ditto

::: browser/components/places/tests/browser/head.js:45
(Diff revision 4)
>  
> +function promiseFocus(win) {
> +  return new Promise(resolve => {
> +    waitForFocus(resolve, win);
> +  });
> +}

This is the 4th head.js wit this util, what about a follow-up to move it to TestUtils.waitForFocus()?
Attachment #8911594 - Flags: review?(mak77) → review+
Comment on attachment 8911594 [details]
Bug 1402707 - Rewrite browser_library_*_middleclick.js to use Places' async APIs.

https://reviewboard.mozilla.org/r/183016/#review202308

> This is the 4th head.js wit this util, what about a follow-up to move it to TestUtils.waitForFocus()?

I've just discovered there's SimpleTest.promiseFocus(), so I'm switching it to use that. afaict there's only one other head.js that defines the function (in downloads).
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1cadcb3b1963
Change BrowserTestUtils.waitForNewTab to allow waiting for tabs to open a new URL when the tab might not be the next one opened. r=mak
https://hg.mozilla.org/integration/autoland/rev/d069236cdcef
Rewrite browser_library_*_middleclick.js to use Places' async APIs. r=mak
https://hg.mozilla.org/mozilla-central/rev/1cadcb3b1963
https://hg.mozilla.org/mozilla-central/rev/d069236cdcef
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.