Please remote the "/themes", part. Because then I can redirect the "chrome://devtools/skin/" to "browser/devtools" in my themes, which allows compatibility across version of Firefox. Otherwise, the "themes" part will seriously break full themes.

(In reply to Alfred Kayser from comment #0) > Please remote the "/themes", part. > Because then I can redirect the "chrome://devtools/skin/" to > "browser/devtools" in my themes, which allows compatibility across version > of Firefox. > > Otherwise, the "themes" part will seriously break full themes. Are you saying it will break any full theme that attempts to provide custom styling for devtools, or that it would break any full theme?

Themes that want to support Firefox before and after this change need to provide the same files in two locations. Also because themes are not allowed to use override in chrome.manifest. But if new url path is chrome://devtools/skin/common.css etc, one can direct devtools in chrome.manifest to browser/devtools, allowing maintain all these devtools in one place. In summary, this change breaks all full themes that support devtools and the only way to fix is provide the same theme files (css and images) in two locations, increasing the size of these themes unnecessarily.

(In reply to Alfred Kayser from comment #2) > Themes that want to support Firefox before and after this change need to > provide the same files in two locations. Also because themes are not allowed > to use override in chrome.manifest. > But if new url path is chrome://devtools/skin/common.css etc, one can direct > devtools in chrome.manifest to browser/devtools, allowing maintain all these > devtools in one place. > > In summary, this change breaks all full themes that support devtools and the > only way to fix is provide the same theme files (css and images) in two > locations, increasing the size of these themes unnecessarily. Do themes have to opt-in to support devtools? Any way we can check to see a list of which ones do if so?

Themes can always decide to not style everything (via chrome.manifest), in my full themes the default Firefox styling for devtools conflicts with the theme styling (or other way around, depending from which viewpoint...)

This is how it looks with full theme support.

(In reply to Brian Grinstead [:bgrins] from comment #3) > (In reply to Alfred Kayser from comment #2) > > Themes that want to support Firefox before and after this change need to > > provide the same files in two locations. Also because themes are not allowed > > to use override in chrome.manifest. > > But if new url path is chrome://devtools/skin/common.css etc, one can direct > > devtools in chrome.manifest to browser/devtools, allowing maintain all these > > devtools in one place. > > > > In summary, this change breaks all full themes that support devtools and the > > only way to fix is provide the same theme files (css and images) in two > > locations, increasing the size of these themes unnecessarily. > > Do themes have to opt-in to support devtools? Any way we can check to see a > list of which ones do if so? I believe themes can override whatever chrome skin URLs they wish[1]. They don't really opt-in for DevTools, but instead would just override DevTools images and styles if those files exist in the theme. Themes don't seem to be reliably indexed in the add-on MXR, so it's hard to say who is customizing DevTools from a theme. [1]: https://developer.mozilla.org/en-US/docs/Building_a_Theme

We've decided to leave the theme URLs as they are. The file migration has had breaking changes for both themes and add-ons, but we're hopeful the simpler file structure will help us all improve DevTools more efficiently going forward. It is possible for theme authors to override the new URLs without duplicating images, such as this example: https://bug1210652.bmoattachments.org/attachment.cgi?id=8669402 As the DevTools file migration makes its way to release channel, these path overrides will no longer be needed, since the themes can instead override only the new paths.

After more discussion, I believe it may be worth doing this after all. A fully correct solution would require an uplift to 44 as well.

Bug 1207976 - Remove /themes from DevTools jar.mn. r=ochameau

Bug 1207976 - Remove /themes from DevTools URLs. r=ochameau

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0eed4d5d08f

https://reviewboard.mozilla.org/r/23855/#review21515 Looks good, you would have to rebase against bug 1216590 which (re)moved some app-manager files.

Comment on attachment 8681541 [details] MozReview Request: Bug 1207976 - Remove /themes from DevTools jar.mn. r=ochameau https://reviewboard.mozilla.org/r/23857/#review21517 I'm happy to land whenever try is happy (which isn't the case on the last push)

Comment on attachment 8681542 [details] MozReview Request: Bug 1207976 - Remove /themes from DevTools URLs. r=ochameau https://reviewboard.mozilla.org/r/23859/#review21519

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9937eaf4c359

https://hg.mozilla.org/integration/fx-team/rev/50427c173f14 https://hg.mozilla.org/integration/fx-team/rev/29d8f6c59b8b https://hg.mozilla.org/integration/fx-team/rev/826db47272a3

I expect the uplift will require a separate patch, so pre-marking that to save everyone time.

https://hg.mozilla.org/mozilla-central/rev/50427c173f14 https://hg.mozilla.org/mozilla-central/rev/29d8f6c59b8b https://hg.mozilla.org/mozilla-central/rev/826db47272a3

Comment on attachment 8681541 [details] MozReview Request: Bug 1207976 - Remove /themes from DevTools jar.mn. r=ochameau Approval Request Comment [Feature/regressing bug #]: In bug 912121, we moved DevTools themes and changed to new URLs for them in 44. [User impact if declined]: The purpose of this change is to simplify work for theme authors. If we don't uplift, then there would now be 3 sets of URLs across Firefox versions, instead of 2. (I would likely back out the change from 45 if uplift is denied.) [Describe test coverage new/current, TreeHerder]: On m-c, covered by existing DevTools tests [Risks and why]: Low given the test coverage [String/UUID change made/needed]: None

Comment on attachment 8681542 [details] MozReview Request: Bug 1207976 - Remove /themes from DevTools URLs. r=ochameau Approval Request Comment [Feature/regressing bug #]: In bug 912121, we moved DevTools themes and changed to new URLs for them in 44. [User impact if declined]: The purpose of this change is to simplify work for theme authors. If we don't uplift, then there would now be 3 sets of URLs across Firefox versions, instead of 2. (I would likely back out the change from 45 if uplift is denied.) [Describe test coverage new/current, TreeHerder]: On m-c, covered by existing DevTools tests [Risks and why]: Low given the test coverage [String/UUID change made/needed]: None

Comment on attachment 8681541 [details] MozReview Request: Bug 1207976 - Remove /themes from DevTools jar.mn. r=ochameau I reviewed this and the 2nd patch and the sheer number of files that are being changed is huge! This is quite unusual and does not fall in the typical Aurora uplift category. While I understand that the issue is of concern, I do not believe this needs to jump trains. Please let it stay in Nightly45 and become effective when 45 goes to Aurora channel.

Comment on attachment 8681542 [details] MozReview Request: Bug 1207976 - Remove /themes from DevTools URLs. r=ochameau Please see my last comment.

(In reply to Ritu Kothari (:ritu) from comment #21) > Comment on attachment 8681541 [details] > MozReview Request: Bug 1207976 - Remove /themes from DevTools jar.mn. > r=ochameau > > I reviewed this and the 2nd patch and the sheer number of files that are > being changed is huge! This is quite unusual and does not fall in the > typical Aurora uplift category. While I understand that the issue is of > concern, I do not believe this needs to jump trains. Please let it stay in > Nightly45 and become effective when 45 goes to Aurora channel. It is a large diff, but the change is entirely robotic: A URL value it changed throughout the code base wherever it is found. The size of the diff should not increase the risk here: either DevTools looks normal because the themes are loaded, or it does not. In this bug, I am attempting to make things easier for theme authors who have requested the URL change here. However, if Aurora uplift is denied, then their work is even more difficult than if I had done nothing on Nightly. Without an Aurora uplift, we're forcing them to support 3 URL types, which is even more than the 2 from before. If it still seems bad, let's try to discuss more on IRC.

Alfred, can you test that the modified URLs in Nightly 45 (which no longer have "/themes") work well for your theme development? This will help guide our decision about uplifting it to 44.

JRyans and I discussed this uplift request at length. I would like some more testing and stabilization before considering uplifting to Aurora44. Hoping that Alfred can help us here. JRyans will also be nominating aurora-specific patches for uplift soon and we can re-review these after the feedback from Alfred.

magiccp, would you be able to test this fix on the latest Nightly build and let us know if it addresses your concerns in bug 1210652? Thanks in advance.

(In reply to Ritu Kothari (:ritu) from comment #26) > magiccp, would you be able to test this fix on the latest Nightly build and > let us know if it addresses your concerns in bug 1210652? Thanks in advance. To be clear, a theme author *does* still need to make a change to support Nightly 45, so they are not fixed in general for all themes. What this bug has achieved it is *easier* for them to do that support that it was without this change. (Or, that's the intent and hope!)

(In reply to Ritu Kothari (:ritu) from comment #26) > magiccp, would you be able to test this fix on the latest Nightly build and > let us know if it addresses your concerns in bug 1210652? Thanks in advance. Qute 5++/6++(version 1.6.4+) has already fixed for Bug 1210652 by changing chrome.manifest and url() in css files. Unfortunately this change was needed a minor additional work for backward compatibility. But it will be good for all themes.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24) > Alfred, can you test that the modified URLs in Nightly 45 (which no longer > have "/themes") work well for your theme development? This will help guide > our decision about uplifting it to 44. Yes, it works. I have included the following line in chrome.manifest: skin devtools nautipolis browser/devtools/ (replace nautipolis with the name of your theme)

Comment on attachment 8681541 [details] MozReview Request: Bug 1207976 - Remove /themes from DevTools jar.mn. r=ochameau Clearing approval status, will submit separate aurora patch.

Comment on attachment 8681542 [details] MozReview Request: Bug 1207976 - Remove /themes from DevTools URLs. r=ochameau Clearing approval status, will submit separate aurora patch.

Approval Request Comment [Feature/regressing bug #]: In bug 912121, we moved DevTools themes and changed to new URLs for them in 44. [User impact if declined]: The purpose of this change is to simplify work for theme authors. If we don't uplift, then there would now be 3 sets of URLs across Firefox versions, instead of 2. (I would likely back out the change from 45 if uplift is denied.) [Describe test coverage new/current, TreeHerder]: On m-c, covered by existing DevTools tests [Risks and why]: Low given the test coverage [String/UUID change made/needed]: None

Approval Request Comment [Feature/regressing bug #]: In bug 912121, we moved DevTools themes and changed to new URLs for them in 44. [User impact if declined]: The purpose of this change is to simplify work for theme authors. If we don't uplift, then there would now be 3 sets of URLs across Firefox versions, instead of 2. (I would likely back out the change from 45 if uplift is denied.) [Describe test coverage new/current, TreeHerder]: On m-c, covered by existing DevTools tests [Risks and why]: Low given the test coverage [String/UUID change made/needed]: None

Comment on attachment 8688023 [details] [diff] [review] Aurora: Part 1 This change (though big) is mostly limited to overriding themes in DevEdition. We have at least two sets of users who have verified this on Nightly. This seems safe to uplift to Aurora44.

Comment on attachment 8688025 [details] [diff] [review] Aurora: Part 2 Aurora44+

https://hg.mozilla.org/releases/mozilla-aurora/rev/24e48f5abcf2 https://hg.mozilla.org/releases/mozilla-aurora/rev/a04d0be8fa98