Tim Nguyen :ntim Reporter
	Description • 7 years ago

See https://bugzilla.mozilla.org/show_bug.cgi?id=1377302#c20

	John Gruen [:jgruen]
	Comment 2 • 7 years ago

Jaws, looks like this is the final piece of the puzzle to get Screenshots icons rendering with context-fill!

	Jonathan Watt [:jwatt]
	Comment 3 • 7 years ago

Note that I do *not* want to expose the context paint mechanism to code that we (Mozilla) do not write ourselves. I'd like to support something like it on the web and for Web Extensions, but the current mechanism is not great as a general solution to the problem of passing values into an external resource. If supporting moz-extension:// would mean that Web Extensions that we don't write could use the context paint mechanism then I'm against fixing this bug I'm afraid.

	John Gruen [:jgruen]
	Comment 4 • 7 years ago

(In reply to Jonathan Watt (back 21 July) [:jwatt] from comment #3)
> Note that I do *not* want to expose the context paint mechanism to code that
> we (Mozilla) do not write ourselves. I'd like to support something like it
> on the web and for Web Extensions, but the current mechanism is not great as
> a general solution to the problem of passing values into an external
> resource. If supporting moz-extension:// would mean that Web Extensions that
> we don't write could use the context paint mechanism then I'm against fixing
> this bug I'm afraid.

We *did* write Screenshots, so it stands to reason that we should be able to reference this context in this case. A few questions:

1. Why would letting all WebExtensions access to context paint values be problematic?

2. Could we special case the Screenshots add-on for the time being since it's a first party property shipping with Firefox by default? Seems like a reasonable solution.

FWIW. I'm fairly certain the design team have in mind that toolbar icons (whether first or third party) should be able to inherit global color values. That's probably out of scope for this bug, bug something to consider going forward.

	Tim Nguyen :ntim Reporter
	Comment 5 • 7 years ago

In case this wasn't clear, this bug isn't about enabling -moz-context-properties: fill, stroke; to all webextensions pages.

This is simply about making https://hg.mozilla.org/mozilla-central/rev/657ef58ae035#l1.11 apply for webextension icons.
Not sure if there's a way to make *just* that apply, without enabling the whole mecanism for all pages.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 7 • 7 years ago

ntim, your proposal for a filter-based approach will probably be what we want long-term as well as for non-mozilla-created webextensions. The patch I just attached only fixes the problem for mozilla-created webextensions to allow us to uplift this and get something working quickly.

	Kris Maglione [:kmag]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8884913 [details]
Bug 1379464 - Enable context-paint for mozilla-created webextensions.

https://reviewboard.mozilla.org/r/155750/#review160920

r=me with nits addressed.

::: layout/svg/SVGContextPaint.cpp:65
(Diff revision 2)
>    //
>    nsAutoCString scheme;
> -  if (NS_SUCCEEDED(aURI->GetScheme(scheme)) &&
> -      (scheme.EqualsLiteral("chrome") || scheme.EqualsLiteral("resource"))) {
> +  if (NS_SUCCEEDED(aURI->GetScheme(scheme))) {
> +    if (scheme.EqualsLiteral("chrome") || scheme.EqualsLiteral("resource")) {
> -    return true;
> +      return true;
> +    } else if (scheme.EqualsLiteral("moz-extension")) {

Please don't check the URI scheme. Just create a principal and check its addonId.

::: layout/svg/SVGContextPaint.cpp:67
(Diff revision 2)
> -  if (NS_SUCCEEDED(aURI->GetScheme(scheme)) &&
> -      (scheme.EqualsLiteral("chrome") || scheme.EqualsLiteral("resource"))) {
> +  if (NS_SUCCEEDED(aURI->GetScheme(scheme))) {
> +    if (scheme.EqualsLiteral("chrome") || scheme.EqualsLiteral("resource")) {
> -    return true;
> +      return true;
> +    } else if (scheme.EqualsLiteral("moz-extension")) {
> +      OriginAttributes attrs;
> +      nsCOMPtr<nsIPrincipal> principal = BasePrincipal::CreateCodebasePrincipal(aURI, attrs);

Nit:

    RefPtr<BasePrincipal> principal = BasePrincipal::CreateCodebasePrincipal(aURI, OriginAttributes());

::: layout/svg/SVGContextPaint.cpp:68
(Diff revision 2)
> -      (scheme.EqualsLiteral("chrome") || scheme.EqualsLiteral("resource"))) {
> +    if (scheme.EqualsLiteral("chrome") || scheme.EqualsLiteral("resource")) {
> -    return true;
> +      return true;
> +    } else if (scheme.EqualsLiteral("moz-extension")) {
> +      OriginAttributes attrs;
> +      nsCOMPtr<nsIPrincipal> principal = BasePrincipal::CreateCodebasePrincipal(aURI, attrs);
> +      if (principal) {

`CreateCodebasePrincipal` never returns null.

::: layout/svg/SVGContextPaint.cpp:70
(Diff revision 2)
> +    } else if (scheme.EqualsLiteral("moz-extension")) {
> +      OriginAttributes attrs;
> +      nsCOMPtr<nsIPrincipal> principal = BasePrincipal::CreateCodebasePrincipal(aURI, attrs);
> +      if (principal) {
> +        nsString addonId;
> +        principal->GetAddonId(addonId);

`if (NS_SUCCEEDED(principal->GetAddonId(addonId))) ...`

	Cameron McCormack (:heycam)
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8884913 [details]
Bug 1379464 - Enable context-paint for mozilla-created webextensions.

https://reviewboard.mozilla.org/r/155750/#review161042

Seems like a reasonable approach for now.

::: layout/svg/SVGContextPaint.cpp:39
(Diff revision 3)
>    // Context paint is pref'ed off for Web content.  Ideally we'd have some
>    // easy means to determine whether the frame that has linked to the image
>    // is a frame for a content node that originated from Web content.
>    // Unfortunately different types of anonymous content, about: documents
>    // such as about:reader, etc. that are "our" code that we ship are
>    // sometimes hard to distinguish from real Web content.  As a result,
>    // instead of trying to figure out what content is "ours" we instead let
>    // any content provide image context paint, but only if the image is
>    // chrome:// or resource:// do we return true.  This should be sufficient
>    // to stop the image context paint feature being useful to (and therefore
>    // used by and relied upon by) Web content.  (We don't want Web content to
>    // use this feature because we're not sure that image context paint is a
>    // good mechanism for wider use, or suitable for specification.)
>    //
>    // One case where we may return false here and prevent image context paint
>    // being used by "our" content is in-tree WebExtensions.  These have scheme
>    // 'moz-extension://', but so do other developers' extensions, and we don't
>    // want extension developers coming to rely on image context paint either.
>    // We may be able to provide our in-tree extensions access to context paint
>    // once they are signed. For more information see:
>    // https://bugzilla.mozilla.org/show_bug.cgi?id=1359762#c5

Please update this comment now that we also return true for Mozilla-provided WebExtensions.

::: layout/svg/SVGContextPaint.cpp:66
(Diff revision 3)
> +    RefPtr<BasePrincipal> principal = BasePrincipal::CreateCodebasePrincipal(aURI, OriginAttributes());
> +    nsString addonId;
> +    if (NS_SUCCEEDED(principal->GetAddonId(addonId))) {
> +      return StringEndsWith(addonId, NS_LITERAL_STRING("@mozilla.org"));
> +    }
> +  }

Nit: I guess it doesn't matter so much, but I'd probably put this code outside of the |if (NS_SUCCEEDED(aURI->GetScheme(scheme)))| block, since the check it does is unrelated to whether that GetScheme call succeded.

	Pulsebot
	Comment 13 • 7 years ago

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/352d62b0ad07
Enable context-paint for mozilla-created webextensions. r=heycam,kmag

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 14 • 7 years ago

Comment on attachment 8884913 [details]
Bug 1379464 - Enable context-paint for mozilla-created webextensions.

Approval Request Comment
[Feature/Bug causing the regression]: not a regression, needed to support various themes with system add-ons (Screenshots) in v55.
[User impact if declined]: Screenshots will show the wrong color icon if users enable a "dark" theme
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, Screenshots is being updated to support this feature. The Screenshots icon in the toolbar should always match the colors of the other icons in the toolbar, when the user has a "light" or a "dark" theme enabled.
[List of other uplifts needed for the feature/fix]: yes, bug 1377302
[Is the change risky?]: no
[Why is the change risky/not risky?]: extends support for 'context-fill' to addons but only mozilla.org addons
[String changes made/needed]: none

	Carsten Book [:Tomcat]
	Comment 15 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/352d62b0ad07

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 16 • 7 years ago

Comment on attachment 8884913 [details]
Bug 1379464 - Enable context-paint for mozilla-created webextensions.

Theme fix for Screenshots, let's uplift this for beta 9.

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/e537bfdef7c9

	Iulia Cristescu, QA [:JuliaC] (away, please needinfo? cornel.ionce@softvision.ro)
	Comment 18 • 7 years ago

I can confirm the issue is verified fixed on Windows 10 x64, Ubuntu 16.04 x86 and Mac macOS 10.12, using 55.0b13 (20170727114534) and 56.0a1 (2017-07-31).