Themes should be allowed to override things in chrome skin packages with other things in chrome skin packages

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, firefox41 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r?bsmedberg
Attachment #8613575 - Flags: review?(benjamin)
(Assignee)

Comment 2

4 years ago
https://reviewboard.mozilla.org/r/9759/#review8555

::: chrome/nsChromeRegistryChrome.cpp:962
(Diff revision 1)
> +      chromeSkinOnly = Substring(chromePath, 0, 6).Equals("/skin/") &&
> +                       Substring(resolvedPath, 0, 6).Equals("/skin/");

As a non-native C++ speaker, I wonder if this is safe? I tried testing, but it looks like if you don't use "content", "locale" or "skin", creating the URL fails and that means we never get here. If that is right, this is always safe, but I'd like to hear from someone else that this isn't going to create issues. :-)
(Assignee)

Comment 3

4 years ago
To be clear, because the blocked bug is targeting 40, it'd be good if this could make 40...
Status: NEW → ASSIGNED

Comment 4

4 years ago
Yes, please!

Comment 5

4 years ago
Comment on attachment 8613575 [details]
MozReview Request: Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r?bsmedberg

https://reviewboard.mozilla.org/r/9761/#review8963

::: xpcom/components/ManifestParser.cpp:671
(Diff revision 1)
> +      if (strcmp(directive->directive, "override")) {

I don't think you want to special-case this. Can you just change the flags at line 138?

::: chrome/nsChromeRegistryChrome.cpp:962
(Diff revision 1)
> +      chromeSkinOnly = Substring(chromePath, 0, 6).Equals("/skin/") &&

Use StringBeginsWith.
Attachment #8613575 - Flags: review?(benjamin)
(Assignee)

Comment 6

4 years ago
Comment on attachment 8613575 [details]
MozReview Request: Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r?bsmedberg

Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r?bsmedberg
(Assignee)

Updated

4 years ago
Attachment #8613575 - Flags: review?(benjamin)

Updated

4 years ago
Attachment #8613575 - Flags: review?(benjamin) → review+

Comment 7

4 years ago
Comment on attachment 8613575 [details]
MozReview Request: Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r?bsmedberg

https://reviewboard.mozilla.org/r/9761/#review9099

Ship It!
https://hg.mozilla.org/mozilla-central/rev/264fb3d4d4c6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 10

4 years ago
Comment on attachment 8613575 [details]
MozReview Request: Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r?bsmedberg

Approval Request Comment
[Feature/regressing bug #]: bug 706103
[User impact if declined]: third-party themes have to do a lot of work in order not to break
[Describe test coverage new/current, TreeHerder]: no explicit automated coverage, but if this didn't work we'd be seeing missing images/styles everywhere, as we did when bug 1150417 landed without the fix here
[Risks and why]: low/none
[String/UUID change made/needed]: nope
Attachment #8613575 - Flags: approval-mozilla-aurora?
Comment on attachment 8613575 [details]
MozReview Request: Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r?bsmedberg

We want to simplify the life of addons developers, taking it.
Attachment #8613575 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Blocks: 1022354
You need to log in before you can comment on or make changes to this bug.