Write reflow tests for the Photon menu and its subview transitions

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(1 attachment)

Summary says it all.
Assignee

Updated

2 years ago
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Assignee

Updated

2 years ago
Whiteboard: [photon-performance]
Iteration: --- → 55.7 - Jun 12
Flags: qe-verify-

Comment 2

2 years ago
mozreview-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

::: 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+
Assignee

Comment 3

2 years ago
mozreview-review-reply
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!
Comment hidden (mozreview-request)

Comment 5

2 years ago
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

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b5bcdc6c93b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.