Closed
Bug 1339610
Opened 7 years ago
Closed 7 years ago
Not possible to use the contextualIdentity API icon
Categories
(WebExtensions :: Frontend, defect, P5)
WebExtensions
Frontend
Tracking
(firefox54 affected, firefox57 fixed)
RESOLVED
FIXED
mozilla57
People
(Reporter: andy+bugzilla, Assigned: jkt)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [contextualIdentities], triaged)
Attachments
(2 files, 1 obsolete file)
The contextualIdentity API returns an icon in the format: icon: "fingerprint" That's not an image I can use anywhere in my WebExtension, because I can't access the Chrome. Would it be possible to expose it to an extension?
Comment 1•7 years ago
|
||
We can add an extra field in the ContextualIdentity with the image path. How does it sound?
Flags: needinfo?(amckay)
Assignee | ||
Comment 2•7 years ago
|
||
As Andy pointed out I don't think these paths are extension accessible. I have a feeling <img can be used but backgrounds can't (haven't tested recently). The other issue not mentioned here but should be is the colours used by the browser don't match the keyword we return. For example pink and blue look very different from the colours used. We however use two slightly different colours for text and icon because the contrast doesn't work correctly on the colours. This likely will be an issue in exposing the colour values.
Reporter | ||
Comment 3•7 years ago
|
||
Sounds great, as long as the path is webextension accessible. Should I file a seperate bug for the colours mentioned in comment 2?
Flags: needinfo?(amckay)
Comment 4•7 years ago
|
||
Yes, please. About the path, we can maybe move the image to a resource:// URL. this should be accessible to addons.
Reporter | ||
Updated•7 years ago
|
Whiteboard: [contextualIdentities]
Updated•7 years ago
|
Priority: -- → P5
Whiteboard: [contextualIdentities] → [contextualIdentities], triaged
Assignee | ||
Updated•7 years ago
|
Blocks: ContextualIdentity
Assignee | ||
Comment 5•7 years ago
|
||
This is actually harder than it seems at present. We currently use context-fill to colour these icons which isn't exposed to anything other than browser code. I would have to see if people can use them as mask-icons or something such that they could use this.
Assignee | ||
Comment 6•7 years ago
|
||
I managed to get this working with the patch and the following: div { mask: url(resource://gre-resources/usercontext/briefcae.svg); mask-size: contain; height: 100px; width: 100px; background: red; }
Assignee: nobody → jkt
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
:dao Is this something you would be interested in reviewing also? We can do a follow up to expand the api to add "iconPath" or something
Flags: needinfo?(dao+bmo)
Comment 9•7 years ago
|
||
(In reply to Jonathan Kingston [:jkt] from comment #8) > :dao > Is this something you would be interested in reviewing also? I think you'd want a layout peer to review this.
Flags: needinfo?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8901149 -
Flags: review?(amarchesini) → review?(gijskruitbosch+bugs)
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8901149 [details] Bug 1339610 - Move chrome:// container icons to be resource:// paths so extension developers can use them. https://reviewboard.mozilla.org/r/172622/#review177938 1) I'm heading out for PTO 2) I'm not a layout peer 3) I think this patch as-is is broken by bug 863246.
Attachment #8901149 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 11•7 years ago
|
||
Thanks Gijs for linking to that bug. It looks to me that opening these up to content is no riskier than opening anything up to content as it's not platform specific and won't even depend on if containers are enabled either. :ethan or :billm could you confirm this assumption is correct? I can move the location of the files to be in one of the new public directories if this is ok.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(ettseng)
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8901149 [details] Bug 1339610 - Move chrome:// container icons to be resource:// paths so extension developers can use them. https://reviewboard.mozilla.org/r/172622/#review177948
Attachment #8901149 -
Flags: review+
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8901149 [details] Bug 1339610 - Move chrome:// container icons to be resource:// paths so extension developers can use them. https://reviewboard.mozilla.org/r/172622/#review177950 You must ask a review request to a layout peer as well.
Assignee | ||
Updated•7 years ago
|
Attachment #8901149 -
Flags: review?(dholbert)
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
Sorry for the few days' delay -- I was on PTO, just back now. Question: Can you explain why these belong in the layout/style subdirectory? I'd rather not clutter up layout/style with firefox-UI toolbar icons (which is I think maybe what these are?), if we can avoid it. I suspect you're maybe just picking there because you want to use a resource://gre-resources URI, and we have a few resource://gre-resources files there already. But I don't think resource://gre-resources stuff has to live in layout/style exclusively. From a bit of DXR searching, it seems like we have resource://gre-resources sprinkled elsewhere in the tree, and all it needs is a jar.mn file with an appropriate line, or something. For example, this file... resource://gre-resources/hiddenWindow.html ...is set up here: https://dxr.mozilla.org/mozilla-central/source/dom/jar.mn#6 And this file: resource://gre-resources/loading-image.png ...is set up here: https://dxr.mozilla.org/mozilla-central/source/layout/generic/jar.mn#6 (in layout, but not layout/style). So I don't think there's any particular reason that these belong in layout/style. So, if I'm not misunderstanding, can you see if you can find a more appropriate place for these and figure out how to set them up with a maybe-new jar.mn file? (maybe via discussion with build peers if you need help on that)
Updated•7 years ago
|
Flags: needinfo?(jkt)
Comment 16•7 years ago
|
||
(In reply to :Gijs (out until Sep 1, expect no replies to requests) from comment #10) > 3) I think this patch as-is is broken by bug 863246. Note that this ^^ bug *just* landed on autoland (probably not on m-c quite yet), so it'd probably be worth retesting the current patch layered on top of that just-landed fix before pursuing this approach too much further. (I haven't read the details; I just clicked through & noticed that it had just landed.)
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8901149 [details] Bug 1339610 - Move chrome:// container icons to be resource:// paths so extension developers can use them. https://reviewboard.mozilla.org/r/172622/#review178694 Calling this r- per my recent comments (these probably want to live elsewhere, or need a compelling explanation for why layout/style is an appropriate home)
Attachment #8901149 -
Flags: review?(dholbert) → review-
Assignee | ||
Comment 18•7 years ago
|
||
> Can you explain why these belong in the layout/style subdirectory? I'd rather not clutter up layout/style with firefox-UI toolbar icons (which is I think maybe what these are?), if we can avoid it. I have no real rationale, I certainly could make this a containers specific jar file as you mention. It looks actually like resource://gre/contentaccessible is the correct path that it should reside in now due to: https://hg.mozilla.org/integration/autoland/rev/ac41228312b8 Thanks for the heads up about that autoland. Will fix and update thanks.
Flags: needinfo?(jkt)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8901510 -
Attachment is obsolete: true
Assignee | ||
Comment 20•7 years ago
|
||
I updated the patch with different paths and using an existing jar file, is this more acceptable? For example: resource://usercontext-content/vacation.svg Stored in: browser/components/contextualidentity/content/vacation.svg Was: layout/style/res/usercontext/vacation.svg
Flags: needinfo?(dholbert)
Flags: needinfo?(amarchesini)
Comment 21•7 years ago
|
||
Seems reasonably sane to me, though I don't know much about our management of assets like these, so I'm not really sure. (My only involvement here was as someone with say-so over the layout/style subdirectory. :))
Flags: needinfo?(dholbert)
Updated•7 years ago
|
Flags: needinfo?(ettseng)
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8902526 [details] Bug 1339610 - Web extension API for container icon and colors. https://reviewboard.mozilla.org/r/174120/#review179374 You caught me about to leave for a few days of PTO, redirect to Kris.
Attachment #8902526 -
Flags: review?(aswan)
Updated•7 years ago
|
Attachment #8902526 -
Flags: review?(kmaglione+bmo)
Seems fine to expose this as long as there's nothing platform- or configuration-specific that's being exposed.
Flags: needinfo?(wmccloskey)
Comment hidden (mozreview-request) |
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8902526 [details] Bug 1339610 - Web extension API for container icon and colors. https://reviewboard.mozilla.org/r/174120/#review180118
Attachment #8902526 -
Flags: review?(amarchesini) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8902526 -
Flags: review?(aswan) → review?(kmaglione+bmo)
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8902526 [details] Bug 1339610 - Web extension API for container icon and colors. https://reviewboard.mozilla.org/r/174120/#review180662 r=me with the getContainerColor and over-strict test issues fixed. ::: toolkit/components/extensions/ext-contextualIdentities.js:23 (Diff revision 2) > }; > > const CONTAINERS_ENABLED_SETTING_NAME = "privacy.containers"; > > +function getContainerIcon(iconName) { > + return `resource://usercontext-content/${iconName}.svg`; Do we need to validate this against existing icon names? ::: toolkit/components/extensions/ext-contextualIdentities.js:38 (Diff revision 2) > + pink: "#ff4bda", > + purple: "#af51f5" > +} > + > +function getContainerColor(colorName) { > + if (colorName in CONTAINER_COLORS) { This is going to return weird things for values like "hasOwnProperty". Can we make `CONTAINER_COLORS` a map, and then just return `CONTAINER_COLORS.get(colorName) || null`? Something like this should do: const CONTAINER_COLORS = new Map(Object.entries({ ... }); ::: toolkit/components/extensions/ext-contextualIdentities.js:41 (Diff revision 2) > + > +function getContainerColor(colorName) { > + if (colorName in CONTAINER_COLORS) { > + return CONTAINER_COLORS[colorName]; > + } > + return false; Seems like this should be null rather than false? ::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:52 (Diff revision 2) > + function getIconUrl(iconName) { > + return `resource://usercontext-content/${iconName}.svg`; > + } > + > + function getColorCode(colorName) { > + const CONTAINER_COLORS = { > + blue: "#37adff", > + turquoise: "#00c79a", > + green: "#51cd00", > + yellow: "#ffcb00", > + orange: "#ff9f00", > + red: "#ff613d", > + pink: "#ff4bda", > + purple: "#af51f5" > + } > + return CONTAINER_COLORS[colorName]; > + } I'm not a fan of tests that just assert that the production code is the same as the code in a test file. Can we just test that the API returns sensible values, rather than explicitly enforcing what values it returns?
Attachment #8902526 -
Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•7 years ago
|
||
Given more than just your feedback has changed could you have an another look? https://reviewboard.mozilla.org/r/174120/diff/2-7/ - Changed methods to async to simplify handling exceptions - Handled exceptions in observer functions - Threw errors instead of allowing null This is based on the feedback for Bug 1395659, which will make that solution much simpler too. I can file a follow up to change all the return Promise.rejects into throwing errors also.
Flags: needinfo?(amarchesini) → needinfo?(kmaglione+bmo)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 36•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d08ea6b95494 Move chrome:// container icons to be resource:// paths so extension developers can use them. r=baku https://hg.mozilla.org/integration/autoland/rev/34d6906c2559 Web extension API for container icon and colors. r=baku,kmag
Keywords: checkin-needed
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d08ea6b95494 https://hg.mozilla.org/mozilla-central/rev/34d6906c2559
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 38•7 years ago
|
||
I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities/ContextualIdentity, please let me know if this covers it.
Flags: needinfo?(jkt)
Assignee | ||
Comment 39•7 years ago
|
||
Hey Will, Thanks for updating this. I added the missing icons to the list too. The rest looks great though.
Flags: needinfo?(jkt)
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 40•7 years ago
|
||
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(jkt)
Assignee | ||
Comment 41•7 years ago
|
||
Should be fine without, it's been verified to work in various extensions already.
Flags: needinfo?(jkt) → qe-verify-
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•