Enable context paint for moz-extension:// images

VERIFIED FIXED in Firefox 55

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: ntim, Assigned: jaws)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified, firefox56 verified)

Details

Attachments

(1 attachment)

Updated

2 years ago
Duplicate of this bug: 1373729

Comment 2

2 years ago
Jaws, looks like this is the final piece of the puzzle to get Screenshots icons rendering with context-fill!
Flags: needinfo?(jaws)
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

2 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.
Flags: needinfo?(jwatt)
(Reporter)

Comment 5

2 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.
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.
Flags: needinfo?(jaws)
Assignee: nobody → jaws
Attachment #8884913 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)

Comment 9

2 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))) ...`
Attachment #8884913 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
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.
Attachment #8884913 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)

Comment 13

2 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
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
Flags: needinfo?(jwatt)
Attachment #8884913 - Flags: approval-mozilla-beta?

Updated

2 years ago
Blocks: 1380119

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/352d62b0ad07
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
Attachment #8884913 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Contact: iulia.cristescu
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.