Closed
Bug 1170207
Opened 10 years ago
Closed 10 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)
Toolkit
Startup and Profile System
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
benjamin
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
No description provided.
Assignee | ||
Comment 1•10 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•10 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•10 years ago
|
||
To be clear, because the blocked bug is targeting 40, it'd be good if this could make 40...
Status: NEW → ASSIGNED
status-firefox40:
--- → affected
Comment 4•10 years ago
|
||
Yes, please!
Comment 5•10 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•10 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•10 years ago
|
Attachment #8613575 -
Flags: review?(benjamin)
Updated•10 years ago
|
Attachment #8613575 -
Flags: review?(benjamin) → review+
Comment 7•10 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!
Comment 9•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 10•10 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 11•10 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
We want to simplify the life of addons developers, taking it.
Attachment #8613575 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 12•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•