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

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1387088 - Add a reflow test for when the user is typing a search into the AwesomeBar.

Review commit: https://reviewboard.mozilla.org/r/170746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170746/

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170746/diff/1-2/

Comment 3

[Florian Quèze [: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

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?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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 5

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Wiring up a dependency so that this lands after bug 1358792.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170746/diff/2-3/

Comment 8

[Florian Quèze [: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/#review176764

Looks great, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This is failing on at least Windows debug like https://treeherder.mozilla.org/logviewer.html#?job_id=125222796&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/a79a150fb5a580d09df1b1768e36b5e65581e469

Comment 11

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4971b32d9f80021d1c68845218d5e3fc620e55ca

Comment 12

[Mike Conley (:mconley) (:⚙️)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170746/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/569927697a09