1379464 - Enable context paint for moz-extension:// images

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Tim Nguyen :ntim]

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

Comment 2

[John Gruen [:jgruen]]

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

Comment 3

[Jonathan Watt [:jwatt]]

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.

Comment 4

[John Gruen [:jgruen]]

(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.

Comment 5

[Tim Nguyen :ntim]

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.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1379464 - Enable context-paint for mozilla-created webextensions.

Review commit: https://reviewboard.mozilla.org/r/155750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155750/

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155750/diff/1-2/

Comment 9

[Kris Maglione [:kmag]]

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))) ...`

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155750/diff/2-3/

Comment 11

[Cameron McCormack (:heycam)]

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.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155750/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/352d62b0ad07
Enable context-paint for mozilla-created webextensions. r=heycam,kmag

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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).