1498195 - remove the code of the old about:performance

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, enhancement)

Comment 1

[Florian Quèze [:florian]]

Attachment: part 1 - remove the code of the old about:performance

Comment 2

[Florian Quèze [:florian]]

Attachment: part 2 - adapt the browser_aboutperformance.js test

Green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b8d5011143514d9c0fa420678bed871817583e2

Comment 3

[Florian Quèze [:florian]]

Attachment: part 3 - CSS cleanup

Comment 4

[Florian Quèze [:florian]]

Attachment: part 3 v2 - CSS cleanup

Fixed an obvious mistake in the CSS cleanup patch.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 9034745 [details] [diff] [review]
part 2 - adapt the browser_aboutperformance.js test

Review of attachment 9034745 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +304,5 @@
>        } else if (/^[a-f0-9]{8}(-[a-f0-9]{4}){3}-[a-f0-9]{12}$/.test(host)) {
>          let addon = WebExtensionPolicy.getByHostname(host);
> +        if (!addon) {
> +          continue;
> +        }

Does this belong in this bug?

::: toolkit/components/aboutperformance/tests/browser/browser_aboutperformance.js
@@ +40,5 @@
>  
> +  // Ensure it is reported as a medium or high energy impact.
> +  let l10nId = row.children[2].getAttribute("data-l10n-id");
> +  Assert.ok(["energy-impact-medium", "energy-impact-high"].includes(l10nId),
> +            "our test tab is medium or high energy impact");

The biggest risk for this test is being intermittent here.. Did you run this on test-verify to make sure it is reliable?

If you think this could be unreliable, how about instead of using the mutation observer above (to give the other tab time 1 refresh interval to use the CPU), convert this to a waitForCondition?

Comment 6

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes (needinfo me!) from comment #5)

Thanks for the reviews!

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +304,5 @@

   } else if (/^[a-f0-9]{8}(-[a-f0-9]{4}){3}-[a-f0-9]{12}$/.test(host)) {
     let addon = WebExtensionPolicy.getByHostname(host);

•    if (!addon) {
  

•      continue;
  

•    }
  

Does this belong in this bug?

Yes, this was causing a lot of noise when running the test.

::: toolkit/components/aboutperformance/tests/browser/browser_aboutperformance.js
@@ +40,5 @@

• // Ensure it is reported as a medium or high energy impact.
• let l10nId = row.children[2].getAttribute("data-l10n-id");
• Assert.ok(["energy-impact-medium", "energy-impact-high"].includes(l10nId),

•        "our test tab is medium or high energy impact");
  

The biggest risk for this test is being intermittent here.. Did you run this on test-verify to make sure it is reliable?

It passes test-verify locally. I just added all the TV jobs on my try push in comment 2 to be extra confident.

If you think this could be unreliable, how about instead of using the mutation observer above (to give the other tab time 1 refresh interval to use the CPU), convert this to a waitForCondition?

By default waitForCondition will wait for up to 3s, so unless I increase the try count, it would cover no more than one refresh.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99db2eabbf07
remove the code of the old about:performance, r=felipe.
https://hg.mozilla.org/integration/mozilla-inbound/rev/99fad7aa0cb7
adapt the browser_aboutperformance.js test to cover the new about:performance, r=felipe.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7c37b0ce9c
cleanup CSS selectors that dealt with about:performance containing multiple tables, r=felipe.

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/99db2eabbf07
https://hg.mozilla.org/mozilla-central/rev/99fad7aa0cb7
https://hg.mozilla.org/mozilla-central/rev/2c7c37b0ce9c