Last Comment Bug 701212 - Get rid of "browser" sub-folder in browser/themes/*stripe/
: Get rid of "browser" sub-folder in browser/themes/*stripe/
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on:
Blocks: 700333 700770
  Show dependency treegraph
 
Reported: 2011-11-09 15:22 PST by Dão Gottwald [:dao]
Modified: 2012-02-01 13:58 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix


Attachments
patch (136.29 KB, patch)
2011-11-09 15:22 PST, Dão Gottwald [:dao]
gavin.sharp: review+
Details | Diff | Review
patch v2 (132.69 KB, patch)
2011-11-09 23:58 PST, Dão Gottwald [:dao]
bugzilla: approval‑mozilla‑aurora-
Details | Diff | Review

Description Dão Gottwald [:dao] 2011-11-09 15:22:48 PST
Created attachment 573347 [details] [diff] [review]
patch

E.g. browser/themes/winstripe/browser/browser.css becomes browser/themes/winstripe/browser.css.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-09 17:17:52 PST
Comment on attachment 573347 [details] [diff] [review]
patch

I think you should leave the "communicator" directories alone - we should probably just get rid of those entirely (if not now then at some point in the future), and merging them in with the other useful stuff makes that easier to forget about. I suppose you could alternately consolidate it and move it up to just be directly in browser/themes.

r=me with that.
Comment 2 Dão Gottwald [:dao] 2011-11-09 23:58:22 PST
Created attachment 573435 [details] [diff] [review]
patch v2
Comment 3 Dão Gottwald [:dao] 2011-11-10 05:51:02 PST
https://hg.mozilla.org/mozilla-central/rev/a41fa578cfc1
Comment 4 Gregory Szorc [:gps] 2011-11-10 23:36:31 PST
When configure creates the Makefile's, I see the following now:

creating browser/installer/windows/Makefile
creating browser/locales/Makefile
creating browser/themes/Makefile
creating browser/themes/pinstripe/browser/Makefile
can't read c:/dev/jenkins/mozilla-central-git/browser/themes/pinstripe/browser/Makefile.in: No such file or directory
creating browser/themes/pinstripe/communicator/Makefile
creating browser/themes/pinstripe/Makefile
creating browser/themes/winstripe/browser/Makefile
can't read c:/dev/jenkins/mozilla-central-git/browser/themes/winstripe/browser/Makefile.in: No such file or directory
creating browser/themes/winstripe/communicator/Makefile
creating browser/themes/winstripe/Makefile
creating browser/branding/nightly/Makefile
creating browser/branding/nightly/content/Makefile
creating browser/branding/nightly/locales/Makefile

You can see this on a Linux build machine: https://tbpl.mozilla.org/php/getParsedLog.php?id=7339477&tree=Firefox&full=1

I'm not sure if the build is now missing Makefiles or what. But, some references definitely need cleaned up.
Comment 5 Ed Morley [:emorley] 2011-11-11 00:28:53 PST
(In reply to Gregory Szorc [:gps] from comment #4)
> I'm not sure if the build is now missing Makefiles or what. But, some
> references definitely need cleaned up.

The |browser/themes/winstripe/browser/Makefile| entries need to be removed from browser/makefiles.sh (the *makefiles.sh files are for speeding up srcdir -> objdir makefile file creation only, so the warnings aren't dangerous).

I'm doing an overhaul of all of the *makefiles.sh scripts in bug 696498, so will clean this up there by the middle of next week, if someone else hasn't done so by then. I'll also be posting to dev.platform asking people to remember to remove and most importantly add entries when touching makefiles, since bug 696498 is having to deal with ~200 makefiles (out of 1100 in the tree) missing from that list, so it would be good for it to stay as up to date as long as possible.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-11-23 02:25:26 PST
Comment on attachment 573435 [details] [diff] [review]
patch v2

would like to land this in aurora as it will make subsequent style changes easier. Low risk, simple file moves.
Comment 7 Dão Gottwald [:dao] 2011-11-23 08:19:50 PST
Have you verified that this applies cleanly and moves all files on aurora?
Comment 8 Rob Campbell [:rc] (:robcee) 2011-11-24 09:42:48 PST
verifying. Patch applies cleanly, but I'll check out a build to make sure that everything's where it should be.
Comment 9 Rob Campbell [:rc] (:robcee) 2011-11-24 11:15:21 PST
verified. Build completed, opened up and ran a full suite of mochi-browser-tests with no errors.
Comment 10 Dão Gottwald [:dao] 2011-11-24 12:24:41 PST
browser-chrome tests don't seem interesting here. Also, the patch applies to all three themes separately. When you apply it, are the three browser sub folders gone?
Comment 11 Rob Campbell [:rc] (:robcee) 2011-11-24 14:13:52 PST
(In reply to Dão Gottwald [:dao] from comment #10)
> browser-chrome tests don't seem interesting here.

I would expect some of our UI tests to fail if there was broken styling. Seemed a possible way to check for brokenness.

> Also, the patch applies to
> all three themes separately. When you apply it, are the three browser sub
> folders gone?

yes. R+.
Comment 12 christian 2011-11-28 12:57:50 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #6)
> Comment on attachment 573435 [details] [diff] [review] [diff] [details] [review]
> patch v2
> 
> would like to land this in aurora as it will make subsequent style changes
> easier. Low risk, simple file moves.

[triage comment]
Easier for? We generally don't expect to take these type of fixes on Beta. Are there still outstanding issues on Aurora that this would help with?
Comment 13 Rob Campbell [:rc] (:robcee) 2011-11-29 06:10:01 PST
Easier for the people landing style-related changes to Aurora, i.e., (in this case), me.

This helps with organization and maintaining consistency between the nightly and aurora branches. In the future, it will help with landing fixes on Beta, should we need to.
Comment 14 Johnathan Nightingale [:johnath] 2011-11-29 14:40:18 PST
Comment on attachment 573435 [details] [diff] [review]
patch v2

We discussed this in triage today and felt that the maintenance cost of prospective future changes to aurora was not sufficient reason to take an organizational cleanup patch on a branch that should be only for security/stability/feature backouts.

We are known to be pretty damned risk averse, so please re-nom if you think we're underestimating the degree of pain caused by leaving this directory move to ride the trains.
Comment 15 Rob Campbell [:rc] (:robcee) 2011-11-30 05:54:06 PST
VERY WELL! (oops, caps got locked somehow)
Comment 16 Ed Morley [:emorley] 2011-11-30 17:40:29 PST
(In reply to Gregory Szorc [:gps] from comment #4)
> I'm not sure if the build is now missing Makefiles or what. But, some
> references definitely need cleaned up.

Now done in: https://hg.mozilla.org/integration/mozilla-inbound/rev/79f0181b02f8

Note You need to log in before you can comment on or make changes to this bug.