Closed Bug 1363505 Opened 8 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.

Attachment

General

Creator:
Created:
Updated:
Size: