Closed Bug 1004025 Opened 10 years ago Closed 10 years ago

Move themes/*/devtools/font-inspector.css into themes/shared/devtools/font-inspector.css

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: bgrins, Assigned: johannes)

References

Details

(Whiteboard: [good first bug][mentor=bgrins][lang=css])

Attachments

(1 file, 1 obsolete file)

As part of the process of DRYing up our CSS, we should move the font-inspector CSS file into the shared folder.

The main steps for merging these files into one are:

1) Make sure there aren't any differences between the windows/linux/osx versions of the files.  You can use a diffing tool for this, or just look at them:

  diff browser/themes/linux/devtools/font-inspector.css browser/themes/osx/devtools/font-inspector.css
  diff browser/themes/linux/devtools/font-inspector.css browser/themes/windows/devtools/font-inspector.css

2) If there are no changes, then all we need to do is make a new file called browser/themes/shared/devtools/font-inspector.css, and copy the contents of one of the other files into it.  If there are changes, we will need to navigate them individually and decide if they should be kept or not.  We can keep them with preprocessor directives if needed.

3) Delete browser/themes/linux/devtools/font-inspector.css, browser/themes/windows/devtools/font-inspector.css, browser/themes/osx/devtools/font-inspector.css

4) Modify browser/themes/*/jar.mn to replace (devtools/font-inspector.css) with (../shared/devtools/font-inspector.css).  Note that the windows/jar.mn file has two places that need to change.
Priority: -- → P3
Johannes, let me know if you need any more info to get started on this.  Basically we want these three files to go away:

browser/themes/linux/devtools/font-inspector.css
browser/themes/osx/devtools/font-inspector.css
browser/themes/windows/devtools/font-inspector.css

And to be replaced with a single new one:

browser/themes/shared/devtools/font-inspector.css
Assignee: nobody → johannes
Status: NEW → ASSIGNED
(In reply to Brian Grinstead [:bgrins] from comment #0)
> As part of the process of DRYing up our CSS, we should move the
> font-inspector CSS file into the shared folder.
> 
> The main steps for merging these files into one are:
> 
> 1) Make sure there aren't any differences between the windows/linux/osx
> versions of the files.  You can use a diffing tool for this, or just look at
> them:
> 
>   diff browser/themes/linux/devtools/font-inspector.css
> browser/themes/osx/devtools/font-inspector.css
>   diff browser/themes/linux/devtools/font-inspector.css
> browser/themes/windows/devtools/font-inspector.css
> 
> 2) If there are no changes, then all we need to do is make a new file called
> browser/themes/shared/devtools/font-inspector.css, and copy the contents of
> one of the other files into it.  If there are changes, we will need to
> navigate them individually and decide if they should be kept or not.  We can
> keep them with preprocessor directives if needed.
> 
> 3) Delete browser/themes/linux/devtools/font-inspector.css,
> browser/themes/windows/devtools/font-inspector.css,
> browser/themes/osx/devtools/font-inspector.css
> 
> 4) Modify browser/themes/*/jar.mn to replace (devtools/font-inspector.css)
> with (../shared/devtools/font-inspector.css).  Note that the windows/jar.mn
> file has two places that need to change.

I should add instead of step 2, we should actually use this command instead of creating a new brand new file:

hg rename browser/themes/linux/devtools/font-inspector.css browser/themes/shared/devtools/font-inspector.css

This should preserve history of the new shared file.
Attached patch movethemes (obsolete) — Splinter Review
This should work.
Attachment #8415493 - Flags: review?(bgrinstead)
OK. I just have seen your latest comment. One moment please.
Attached patch movethemesV2Splinter Review
Attachment #8415493 - Attachment is obsolete: true
Attachment #8415493 - Flags: review?(bgrinstead)
Attachment #8415511 - Flags: review?(bgrinstead)
Comment on attachment 8415511 [details] [diff] [review]
movethemesV2

Review of attachment 8415511 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.  Thanks!
Attachment #8415511 - Flags: review?(bgrinstead) → review+
Fine. I'm going to flag it for check in.
Keywords: checkin-needed
(In reply to Johannes Mittendorfer from comment #7)
> Fine. I'm going to flag it for check in.

Sounds good.  If you don't mind continuing to help with this, my goal is to eventually empty out https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/ so we only have one copy of each file.  The next step is to do all of the images in Bug 1004111.
Maybe tomorrow. It's a bit late in my time zone. I will ask for the assignment when/if I have time.

Thanks so far!
(In reply to Johannes Mittendorfer from comment #9)
> Maybe tomorrow. It's a bit late in my time zone. I will ask for the
> assignment when/if I have time.
> 
> Thanks so far!

OK, thanks for the help
Apparently a try push is needed with checkin-needed (See : https://bugzilla.mozilla.org/show_bug.cgi?id=989751#21 )
Could you may give me a voucher for commit level 1, so I can run these tests myself the next time? 

https://bugzilla.mozilla.org/show_bug.cgi?id=1004398#c1
https://hg.mozilla.org/integration/fx-team/rev/44f24051261e
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=bgrins][lang=css] → [good first bug][mentor=bgrins][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/44f24051261e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=bgrins][lang=css][fixed-in-fx-team] → [good first bug][mentor=bgrins][lang=css]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.