Closed Bug 1498185 Opened Last year Closed Last year

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

Categories

(Toolkit :: Performance Monitoring, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #9017518 - Flags: review?(felipc)
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 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
(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.
Feel free to r+ either version of the patch after reading comment 4.
Attachment #9017831 - Flags: review?(felipc)
Attachment #9017518 - Attachment is obsolete: true
Attachment #9017518 - Flags: review?(felipc)
Attachment #9017831 - Flags: review?(felipc) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37647eaadbc2
Implement the type and action columns in the new about:performance, r=felipe.
Backout by dvarga@mozilla.com:
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
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
Flags: needinfo?(florian)
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.
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b06ecb59a60
Implement the type and action columns in the new about:performance, r=felipe.
https://hg.mozilla.org/mozilla-central/rev/8b06ecb59a60
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: needinfo?(florian)
You need to log in before you can comment on or make changes to this bug.