Closed Bug 1339610 Opened 7 years ago Closed 7 years ago

Not possible to use the contextualIdentity API icon

Categories

(WebExtensions :: Frontend, defect, P5)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox54 affected, firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox54 --- affected
firefox57 --- fixed

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)

Bug 1339610 - Move chrome:// container icons to be resource:// paths so extension developers can use them.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1339610 - Web extension API for container icon and colors.
59 bytes, text/x-review-board-request
baku
: review+
kmag
: review+
Details 
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?
We can add an extra field in the ContextualIdentity with the image path. How does it sound?
Flags: needinfo?(amckay)
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.
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)
Yes, please.

About the path, we can maybe move the image to a resource:// URL. this should be accessible to addons.
Whiteboard: [contextualIdentities]
Priority: -- → P5
Whiteboard: [contextualIdentities] → [contextualIdentities], triaged
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.
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
: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)
(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)
Attachment #8901149 - Flags: review?(amarchesini) → review?(gijskruitbosch+bugs)
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)
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 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 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.
Attachment #8901149 - Flags: review?(dholbert)
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)
Flags: needinfo?(jkt)
(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 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-
> 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)
Attachment #8901510 - Attachment is obsolete: true
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)
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)
Flags: needinfo?(ettseng)
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)
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 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+
Attachment #8902526 - Flags: review?(aswan) → review?(kmaglione+bmo)
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+
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)
lgtm
Flags: needinfo?(kmaglione+bmo)
Keywords: checkin-needed
Keywords: dev-doc-needed
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
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)
Hey Will,

Thanks for updating this. I added the missing icons to the list too. The rest looks great though.
Flags: needinfo?(jkt)
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)
Should be fine without, it's been verified to work in various extensions already.
Flags: needinfo?(jkt) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: