1498185 - Implement the 'type' and action columns in the new about:performance

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, enhancement)

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

I was initially thinking I wouldn't show the "Show in add-on manager" arrow for system add-ons, as users can't disable them, but I haven't found a synchronous way to figure out if an add-on is a system add-on or not. Interestingly, the add-on manager can totally open a details page for system add-ons (even though access to that page is currently not exposed in the add-on manager UI), however it shows a broken "disable" button (broken because it just throws an error when clicking it).

I think it should be fine to figure this out (either a way to not show the icon for system add-ons, or a way to hide the broken "disable" button) in a follow-up discussed with add-on manager folks.

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 9017518 [details] [diff] [review]
Patch

The changes in toolkit/components/aboutperformance/jar.mn are leftover from a previous local version; I've just reverted them locally, please ignore them.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 9017518 [details] [diff] [review]
Patch

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

Why add new icons and not use ones already available in the tree?

(You mentioned this in the meeting today but I don't remember the reasoning for that.)

Also, why is the add-on icon called shortcut.svg and the close icon called dismiss.svg?

I already looked at everything else in the patch and it looks fine, just want to know why not use https://searchfox.org/mozilla-central/source/toolkit/themes/shared/icons/close.svg for example

Comment 4

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> Comment on attachment 9017518 [details] [diff] [review]
> Patch
> 
> Review of attachment 9017518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why add new icons and not use ones already available in the tree?

I was initially using the close.svg icon, but it has a square background that Bryan didn't want, so he pushed me toward using the other icon that we already have in the tree, which is https://searchfox.org/mozilla-central/source/browser/components/newtab/data/content/assets/glyph-dismiss-16.svg
I didn't want to re-use that one at its current url, as background-image: url("resource://activity-stream/data/content/assets/glyph-dismiss-16.svg"); wouldn't look correct in about:performance.

I thought about it some more, and I can force the close.svg icon to have a transparent background, and resize it to have it at about the same size as the dismiss icon. The result isn't exactly identical. To compare the two with the existing patch applied, you can load this url:

data:text/html,<img src="chrome://global/skin/icons/close.svg" style="-moz-context-properties: fill, fill-opacity;fill-opacity: 0;height: 24px;"><img src="chrome://global/skin/icons/dismiss.svg" style="height: 16px;position: relative;top: -4px;">

Here is a screenshot of the two icons: https://i.imgur.com/3TLTzve.png (on the left the current close.svg icon, rescaled; on the right the icon that Bryan had recommended).

This seems close enough to me, but I'm not a designer :-).

I'll attach another version of the patch using the close.svg icon instead of the dismiss one.

> Also, why is the add-on icon called shortcut.svg and the close icon called
> dismiss.svg?

The file was named shortcut-16.svg when Bryan sent it to me. My guess is that the curved arrow in the icon should remain Windows users of desktop shortcuts. I think the idea here is that the icon means that clicking it will bring the user to some other UI, rather than perform an action on the add-on directly.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch reusing close.svg

Feel free to r+ either version of the patch after reading comment 4.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37647eaadbc2
Implement the type and action columns in the new about:performance, r=felipe.

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e10a4e70394
Backed out changeset 37647eaadbc2 for browser chrome failure at browser/base/content/test/static/browser_all_files_referenced.js on a CLOSED TREE

Comment 8

[Daniel Varga [:dvarga]]

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=206231415&repo=mozilla-inbound&lineNumber=4222

[task 2018-10-17T21:46:16.887Z] 21:46:16     INFO - indirectly whitelisted file: resource://gre/modules/accessibility/Traversal.jsm used from resource://gre/modules/accessibility/ContentControl.jsm
[task 2018-10-17T21:46:16.888Z] 21:46:16     INFO - Buffered messages finished
[task 2018-10-17T21:46:16.889Z] 21:46:16     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 1, expected 0
[task 2018-10-17T21:46:16.890Z] 21:46:16     INFO - Stack trace:
[task 2018-10-17T21:46:16.891Z] 21:46:16     INFO - chrome://mochikit/content/browser-test.js:test_is:1295
[task 2018-10-17T21:46:16.901Z] 21:46:16     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:771
[task 2018-10-17T21:46:16.901Z] 21:46:16     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
[task 2018-10-17T21:46:16.901Z] 21:46:16     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
[task 2018-10-17T21:46:16.901Z] 21:46:16     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:986
[task 2018-10-17T21:46:16.902Z] 21:46:16     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-10-17T21:46:16.903Z] 21:46:16     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-10-17T21:46:16.904Z] 21:46:16     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: chrome://global/skin/icons/shortcut.svg - 
[task 2018-10-17T21:46:16.904Z] 21:46:16     INFO - Stack trace:
[task 2018-10-17T21:46:16.905Z] 21:46:16     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:775
[task 2018-10-17T21:46:16.906Z] 21:46:16     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
[task 2018-10-17T21:46:16.907Z] 21:46:16     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
[task 2018-10-17T21:46:16.908Z] 21:46:16     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:986
[task 2018-10-17T21:46:16.909Z] 21:46:16     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-10-17T21:46:16.910Z] 21:46:16     INFO - ignored unused whitelist entry: resource://app/blocklist.xml
[task 2018-10-17T21:46:16.911Z] 21:46:16     INFO - ignored unused whitelist entry: resource://gre/gmp-clearkey/0.1/manifest.json
[task 2018-10-17T21:46:16.912Z] 21:46:16     INFO - ignored unused whitelist entry: resource://gre/res/test.properties
[task 2018-10-17T21:46:16.913Z] 21:46:16     INFO - missing file: resource://gre/modules/commonjs/toolkit/loader.js

Comment 9

[Florian Quèze [:florian]]

Attachment: Patch passing test

The reference to shortcut.svg from aboutPerformance.xhtml wasn't detected by the browser_all_files_references.js test because CSS code included inside a .xhtml file is parsed as a random code file, rather than as CSS code. The patch in bug 1497202 would fix this by moving the about:performance css code to a separate file. I fixed the test failure here by adding double quotes around the urls used in CSS, which makes the file's codying style more consistent anyway.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b06ecb59a60
Implement the type and action columns in the new about:performance, r=felipe.

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8b06ecb59a60