Closed Bug 1369491 Opened 7 years ago Closed 7 years ago

Write reflow tests for the Photon menu and its subview transitions

Categories

(Firefox :: Menus, enhancement, P1)

enhancement

Tracking

()

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

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [photon-performance])

Attachments

(1 file)

Summary says it all.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Whiteboard: [photon-performance]
Iteration: --- → 55.7 - Jun 12
Flags: qe-verify-
Comment on attachment 8873563 [details]
Bug 1369491 - Write reflow tests for the new Photon appmenu and its subviews.

https://reviewboard.mozilla.org/r/144948/#review148942

::: browser/base/content/test/performance/browser_appmenu_reflows.js:113
(Diff revision 1)
> +      if (!navButtons) {
> +        return;
> +      }
> +
> +      for (let button of navButtons) {
> +        await new Promise(resolve => setTimeout(resolve, 0));

Why do we need this? Should this be a requestAnimationFrame instead of a setTimeout(..., 0) to ensure that we are not causing several changes in a row that don't get actually displayed? Or does it not matter because of the waitForEvent promises we already await?

::: browser/base/content/test/performance/browser_appmenu_reflows.js:117
(Diff revision 1)
> +      for (let button of navButtons) {
> +        await new Promise(resolve => setTimeout(resolve, 0));
> +        button.click();
> +        await BrowserTestUtils.waitForEvent(PanelUI.panel, "ViewShown");
> +        // Unfortunately, I can't find a better accessor to the current
> +        // subview, so I have to reach instance the PanelMultiView instance

typo: remove the first 'instance'
Attachment #8873563 - Flags: review?(florian) → review+
Comment on attachment 8873563 [details]
Bug 1369491 - Write reflow tests for the new Photon appmenu and its subviews.

https://reviewboard.mozilla.org/r/144948/#review148942

> Why do we need this? Should this be a requestAnimationFrame instead of a setTimeout(..., 0) to ensure that we are not causing several changes in a row that don't get actually displayed? Or does it not matter because of the waitForEvent promises we already await?

Yikes - leftovers. Will remove. Thanks!
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b5bcdc6c93b
Write reflow tests for the new Photon appmenu and its subviews. r=florian
https://hg.mozilla.org/mozilla-central/rev/2b5bcdc6c93b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: