1207976 - Remove /themes from chrome://devtools/skin/themes/*

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Alfred Kayser]

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.

Comment 1

[Brian Grinstead [:bgrins]]

(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?

Comment 2

[Alfred Kayser]

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.

Comment 3

[Brian Grinstead [:bgrins]]

(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?

Comment 4

[Alfred Kayser]

Attachment: walnut_devtools.png

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...)

Comment 5

[Alfred Kayser]

Attachment: walnut_devtools_before.png

This is how it looks with full theme support.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1207976 - Remove /themes from DevTools jar.mn. r=ochameau

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

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1207976 - Remove /themes from DevTools URLs. r=ochameau

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

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 12

[Alexandre Poirot [:ochameau]]

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 13

[Alexandre Poirot [:ochameau]]

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 14

[Alexandre Poirot [:ochameau]]

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

https://reviewboard.mozilla.org/r/23859/#review21519

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 16

[Pulsebot]

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

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 18

[Carsten Book [:Tomcat]]

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 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Please see my last comment.

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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!)

Comment 28

[magicp]

(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.

Comment 29

[Alfred Kayser]

(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 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Clearing approval status, will submit separate aurora patch.

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Aurora: Part 1

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 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Aurora: Part 2

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 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Aurora44+

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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