Write a test to measure how many reflows occur when typing individual characters into the AwesomeBar

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [photon-performance] )

Attachments

(1 attachment)

Similar to bug 1356271, except this time, instead of dropping the whole string into the AwesomeBar, I want to type individual characters, which is a more realistic approximation of user behaviour.
Flags: qe-verify-
Priority: -- → P2
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8899461 [details]
Bug 1387088 - Add a reflow test for when the user is typing a search into the AwesomeBar.

https://reviewboard.mozilla.org/r/170746/#review176318

Thanks for writing this test! Looks good overall, but I pointed out enough details that I should probably have a quick look at the next version.

::: browser/base/content/test/performance/browser_urlbar_keyed_search_reflows.js:8
(Diff revision 2)
> +// There are a _lot_ of reflows in this test, and processing them takes
> +// time. On slower builds, we need to boost our allowed test time.
> +requestLongerTimeout(5);
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
> +                                  "resource://testing-common/PlacesTestUtils.jsm");

This is already imported from the head.js file, so we don't need it here. And you can also remove it from browser_urlbar_search_reflows.js.

::: browser/base/content/test/performance/browser_urlbar_keyed_search_reflows.js:41
(Diff revision 2)
> +    times: 1, // This number should only ever go down - never up.
> +  },
> +
> +  {
> +    stack: [
> +      "adjustSiteIconStart@chrome://global/content/bindings/autocomplete.xml",

FYI, paolo has an r+'ed patch for the adjustSiteIconStart issues in bug 1358792, so he may bitrot your reflow list... in a nice way :-).

::: browser/base/content/test/performance/browser_urlbar_keyed_search_reflows.js:139
(Diff revision 2)
> +    ],
> +  },
> +
> +  {
> +    stack: [
> +      "synthesizeKey@chrome://mochikit/content/tests/SimpleTest/EventUtils.js",

It's unfortunate that our test harness is causing sync reflows visible within the test. Should we filter this out from head.js instead of in the test?

::: browser/base/content/test/performance/browser_urlbar_keyed_search_reflows.js:155
(Diff revision 2)
> + *        The window to do the search in.
> + * @returns Promise
> + */
> +async function promiseSearchComplete(win) {
> +  let URLBar = win.gURLBar;
> +  URLBar.controller.startSearch(URLBar.value);

Why do we need the startSearch and stopSearch explicit calls? Shouldn't all of that be triggered by the key events we are synthesizing?

::: browser/base/content/test/performance/browser_urlbar_keyed_search_reflows.js:214
(Diff revision 2)
> +      oldResultsAdded();
> +    };
> +
> +    // Only keying in 6 characters because the number of reflows triggered
> +    // is so high that we risk timing out the test if we key in any more.
> +    let query = `ows-10`;

Should this be in the SEARCH_TERM constant instead? Why the ` ?

::: browser/base/content/test/performance/head.js:5
(Diff revision 2)
> +"use strict";
> +
> +let tmp = {};
> +Cu.import("resource://testing-common/PlacesTestUtils.jsm", tmp);
> +let { PlacesTestUtils } = tmp;

Why is this not just Cu.import("resource://testing-common/PlacesTestUtils.jsm"); ?

Or should it be:

XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
  "resource://testing-common/PlacesTestUtils.jsm");

to avoid loading this for unrelated performance tests?
Attachment #8899461 - Flags: review?(florian)
(Assignee)

Comment 4

2 years ago
mozreview-review-reply
Comment on attachment 8899461 [details]
Bug 1387088 - Add a reflow test for when the user is typing a search into the AwesomeBar.

https://reviewboard.mozilla.org/r/170746/#review176318

> FYI, paolo has an r+'ed patch for the adjustSiteIconStart issues in bug 1358792, so he may bitrot your reflow list... in a nice way :-).

Excellent! I'll wait until that lands before I land this (with the stacks adjusted).

> It's unfortunate that our test harness is causing sync reflows visible within the test. Should we filter this out from head.js instead of in the test?

Yeah, good idea. I'll add a list of test harness reflows that get filtered out in withReflowObserver.

> Should this be in the SEARCH_TERM constant instead? Why the ` ?

Leftovers. Thanks!
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8899461 [details]
Bug 1387088 - Add a reflow test for when the user is typing a search into the AwesomeBar.

https://reviewboard.mozilla.org/r/170746/#review176318

> Why do we need the startSearch and stopSearch explicit calls? Shouldn't all of that be triggered by the key events we are synthesizing?

You're right! Unnecessary leftovers. Good eye.
(Assignee)

Comment 6

2 years ago
Wiring up a dependency so that this lands after bug 1358792.
Depends on: 1358792
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8899461 [details]
Bug 1387088 - Add a reflow test for when the user is typing a search into the AwesomeBar.

https://reviewboard.mozilla.org/r/170746/#review176764

Looks great, thanks!
Attachment #8899461 - Flags: review?(florian) → review+

Comment 9

2 years ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e903ef2fd85a
Add a reflow test for when the user is typing a search into the AwesomeBar. r=florian
(Assignee)

Updated

2 years ago
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/569927697a09
Add a reflow test for when the user is typing a search into the AwesomeBar. r=florian

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/569927697a09
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.