Closed Bug 1363505 Opened 7 years ago Closed 7 years ago

Write more tab and window reflow tests for the browser UI

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.7 - Jun 12
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-performance])

Attachments

(4 files)

Bug 1363505 - Add tab squeeze reflow test.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1363505 - Add tab growth reflow test.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1363505 - Add tab switch reflow test.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1363505 - Add reflow test for window closing.
59 bytes, text/x-review-board-request
florian
: review+
Details
Blocks: photon-performance-triage
Whiteboard: [photon-performance]
Flags: qe-verify?
Priority: -- → P2
Flags: qe-verify? → qe-verify-
I've spun out bug 1367797 to handle the overflowing tab cases, since that's blocked on some tab strip work.

So the full list now is:

* Window closing
* Tab opening (existing tabs must shrink)
* Tab closing (existing tabs must grow)
* Tab switching (both tabs in view)
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Priority: P2 → P1
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Comment on attachment 8871877 [details]
Bug 1363505 - Add tab switch reflow test.

https://reviewboard.mozilla.org/r/143392/#review148016

::: browser/base/content/test/performance/browser_tabswitch_reflows.js:48
(Diff revision 1)
> +
> +  let firstSwitchDone = BrowserTestUtils.waitForEvent(window, "TabSwitchDone");
> +  let otherTab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
> +  await firstSwitchDone;
> +
> +  // Add a reflow observer and open a new tab.

the "open a new tab" part of the comment is incorrect.
Attachment #8871877 - Flags: review?(florian) → review+
Comment on attachment 8871875 [details]
Bug 1363505 - Add tab squeeze reflow test.

https://reviewboard.mozilla.org/r/143388/#review148018

Looks reasonable overall, but there are enough comments that I would like to have another quick look.

::: browser/base/content/test/performance/browser_tabopen_squeeze_reflows.js:59
(Diff revision 1)
> +
> +  await withReflowObserver(async function() {
> +    let switchDone = BrowserTestUtils.waitForEvent(window, "TabSwitchDone");
> +    BrowserOpenTab();
> +    await BrowserTestUtils.waitForEvent(gBrowser.selectedTab, "transitionend",
> +        false, e => e.propertyName === "max-width");

nit: the indent feels wrong here. Either use 2 spaces, or align with the first parameter of the waitForEvent call.

::: browser/base/content/test/performance/head.js:187
(Diff revision 1)
> +let tabMinWidth =
> +  parseInt(getComputedStyle(gBrowser.selectedTab, null).minWidth, 10);
> +
> +const TAB_COUNT_MAX = Math.floor(tabStripWidth / tabMinWidth);
> +const TAB_COUNT_FOR_SQUEEZE = TAB_COUNT_MAX - 1;
> +const TAB_COUNT_FOR_OVERFLOW = TAB_COUNT_MAX + 1;

I don't feel good about running this tab size computation code for all tests when only 2 of them actually need it. Could you change this code to a function called by the 2 tests?

TAB_COUNT_FOR_OVERFLOW isn't used (yet, I assume), so it's tempting to say the function should return TAB_COUNT_FOR_SQUEEZE, but otherwise I would just make a function returning the max tab count, and let the individual tests deal with the +/- 1.

::: browser/base/content/test/performance/head.js:201
(Diff revision 1)
> + * @param howMany (int)
> + *        How many about:blank tabs to open.
> + */
> +async function createTabs(howMany) {
> +  let uris = [];
> +  while (--howMany) {

Did you mean howMany-- rather than --howMany? If 'howMany' is 1, you'll currently never enter the loop.
Attachment #8871875 - Flags: review?(florian) → review-
Comment on attachment 8871876 [details]
Bug 1363505 - Add tab growth reflow test.

https://reviewboard.mozilla.org/r/143390/#review148020

::: browser/base/content/test/performance/browser_tabclose_grow_reflows.js:56
(Diff revision 1)
> +  await withReflowObserver(async function() {
> +    let switchDone = BrowserTestUtils.waitForEvent(window, "TabSwitchDone");
> +    let tab = gBrowser.tabs[gBrowser.tabs.length - 1];
> +    gBrowser.removeTab(tab, { animate: true });
> +    await BrowserTestUtils.waitForEvent(tab, "transitionend",
> +        false, e => e.propertyName === "max-width");

same indent nit here.
Attachment #8871876 - Flags: review?(florian) → review+
Comment on attachment 8871875 [details]
Bug 1363505 - Add tab squeeze reflow test.

https://reviewboard.mozilla.org/r/143388/#review148386

::: browser/base/content/test/performance/head.js:194
(Diff revision 2)
> +  let tabStripRect = gBrowser.tabContainer.mTabstrip.getBoundingClientRect();
> +  let tabStripWidth = tabStripRect.width;
> +  let tabMinWidth =
> +    parseInt(getComputedStyle(gBrowser.selectedTab, null).minWidth, 10);
> +
> +  let maxTabCount = Math.floor(tabStripWidth / tabMinWidth) - 1;

What's this magic - 1 constant here? Should it instead be the current tab count? (if not, I think we need a comment explaining why we take one off).
Attachment #8871875 - Flags: review?(florian) → review+
Comment on attachment 8871875 [details]
Bug 1363505 - Add tab squeeze reflow test.

https://reviewboard.mozilla.org/r/143388/#review148386

> What's this magic - 1 constant here? Should it instead be the current tab count? (if not, I think we need a comment explaining why we take one off).

Yeah, it's the current tab count - I'll just use that instead. Thanks!
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afc25136f9ae
Add tab squeeze reflow test. r=florian
https://hg.mozilla.org/integration/autoland/rev/b6447762b05a
Add tab growth reflow test. r=florian
https://hg.mozilla.org/integration/autoland/rev/b0c79c15c6ff
Add tab switch reflow test. r=florian
The browser_roundedWindow_newWindow.js failures were caused by bug 1355764, not this one.
Turns out I also forgot to post a test that I'd written in this series!

I also think I've fixed the breakage that was caused by bug 1355764; I have to account for the width of the newtab button when calculating how many tabs can fit into the tab strip without overflowing.
Flags: needinfo?(mconley)
Comment on attachment 8873278 [details]
Bug 1363505 - Add reflow test for window closing.

https://reviewboard.mozilla.org/r/144732/#review148634

::: browser/base/content/test/performance/browser_windowclose_reflows.js:14
(Diff revision 1)
> + * See https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers
> + * for tips on how to do that.
> + */
> +const EXPECTED_REFLOWS = [
> +  /**
> +   * Nothing here! Please don't anything new!

Nit: don't add.
Comment on attachment 8873278 [details]
Bug 1363505 - Add reflow test for window closing.

https://reviewboard.mozilla.org/r/144732/#review148730

::: browser/base/content/test/performance/browser_windowclose_reflows.js:25
(Diff revision 1)
> + * uninterruptible reflows when closing windows. When the
> + * window is closed, the test waits until the original window
> + * has activated.
> + */
> +add_task(function*() {
> +  // Ensure that this browser window starts focused.

Doesn't hurt to check I guess... but isn't this always the case when a bc test starts?
Attachment #8873278 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Doesn't hurt to check I guess... but isn't this always the case when a bc
> test starts?

Perhaps, but not necessarily when the test itself repeats. I've been attempting to pre-emptively stamp out intermittents on these tests by running them (and the whole directory) on repeat a bunch, and making sure they pass. This was one of the things I needed to do to stamp out an intermittent.
(In reply to Mike Conley (:mconley) from comment #27)
> (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > Doesn't hurt to check I guess... but isn't this always the case when a bc
> > test starts?
> 
> Perhaps, but not necessarily when the test itself repeats. I've been
> attempting to pre-emptively stamp out intermittents on these tests by
> running them (and the whole directory) on repeat a bunch, and making sure
> they pass. This was one of the things I needed to do to stamp out an
> intermittent.

You may want to explain this in a comment to stop the next reader of the file from 'cleaning it up' ;).
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c50bc93a3dc
Add reflow test for window closing. r=florian
https://hg.mozilla.org/integration/autoland/rev/6f6b73f4f305
Add tab squeeze reflow test. r=florian
https://hg.mozilla.org/integration/autoland/rev/494590d4307a
Add tab growth reflow test. r=florian
https://hg.mozilla.org/integration/autoland/rev/c6b6bc96e99c
Add tab switch reflow test. r=florian
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e603b83ca395
Backed out 4 changesets to avoid conflicts with bug 1369140 on inbound.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/798f637191ec
Add reflow test for window closing. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e037ab2ee52
Add tab squeeze reflow test. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ae492c2da06
Add tab growth reflow test. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd3380db77f
Add tab switch reflow test. r=florian on a CLOSED TREE
See Also: → 1385034
You need to log in before you can comment on or make changes to this bug.