Closed Bug 1170207 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file)

      No description provided.
Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r?bsmedberg
Attachment #8613575 - Flags: review?(benjamin)
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. :-)
To be clear, because the blocked bug is targeting 40, it'd be good if this could make 40...
Status: NEW → ASSIGNED
Yes, please!
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)
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
Attachment #8613575 - Flags: review?(benjamin)
Attachment #8613575 - Flags: review?(benjamin) → review+
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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+
Blocks: 1022354
You need to log in before you can comment on or make changes to this bug.