remove the code of the old about:performance

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

6 months ago
Blocks: 1406872
(Assignee)

Updated

3 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 months ago
Posted patch part 3 - CSS cleanup (obsolete) — Splinter Review
Attachment #9034746 - Flags: review?(felipc)
(Assignee)

Comment 4

3 months ago
Fixed an obvious mistake in the CSS cleanup patch.
Attachment #9034768 - Flags: review?(felipc)
(Assignee)

Updated

3 months ago
Attachment #9034746 - Attachment is obsolete: true
Attachment #9034746 - Flags: review?(felipc)
Attachment #9034744 - Flags: review?(felipc) → review+
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?
Attachment #9034745 - Flags: review?(felipc) → review+
Attachment #9034768 - Flags: review?(felipc) → review+
(Assignee)

Comment 6

3 months ago

(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

3 months ago
Pushed by florian@queze.net:
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

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.