Write reflow tests for the Photon menu and its subview transitions

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Menus
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

unspecified
Firefox 55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
Summary says it all.
(Assignee)

Updated

3 months ago
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Whiteboard: [photon-performance]

Updated

3 months ago
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+
(Assignee)

Comment 3

3 months 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

3 months 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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b5bcdc6c93b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 months ago
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.