Closed Bug 1387088 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Address Bar, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
57.2 - Aug 29
Tracking Status
firefox57 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-performance] )

Attachments

(1 file)

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 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)
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!
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.
Wiring up a dependency so that this lands after bug 1358792.
Depends on: 1358792
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+
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
Flags: needinfo?(mconley)
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
https://hg.mozilla.org/mozilla-central/rev/569927697a09
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: