Closed Bug 1010959 Opened 5 years ago Closed 5 years ago

Move themes/*/devtools/computedview.css into themes/shared/devtools/computedview.css

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: bgrins, Assigned: alexharris6)

References

Details

(Whiteboard: [mentor=bgrins][lang=css])

Attachments

(1 file, 1 obsolete file)

As part of the process of DRYing up our CSS, we should move the computed-view 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/computedview.css browser/themes/osx/devtools/computedview.css
  diff browser/themes/linux/devtools/computedview.css browser/themes/windows/devtools/computedview.css

2) If there are no changes, then all we need to do is delete the two of the versions then hg rename the other (to preserve history):

hg rename browser/themes/linux/devtools/computedview.css browser/themes/shared/devtools/computedview.css

2a) If there are minor changes between OS, then the new file will have to have a * in front of it in the jar.mn files, and we will need to add some conditions, like %ifdef XP_MACOSX to handle them specifically.

3) Modify browser/themes/*/jar.mn to replace (devtools/computedview.css) with (../shared/devtools/computedview.css).  Note that the windows/jar.mn file has two places that need to change.
Hi, I can take this on. I'll get started with step 1 and 2, which seem straightforward. And we'll see if 2a+ becomes necessary.
(In reply to alexharris6 from comment #1)
> Hi, I can take this on. I'll get started with step 1 and 2, which seem
> straightforward. And we'll see if 2a+ becomes necessary.

I'm pretty sure it will be, I think there may be an extra couple rules just for Linux.
Assignee: nobody → alexharris6
Status: NEW → ASSIGNED
Ok, it is as you say. Here are the extra Linux rules:

/* Take away these two :visited rules to get a core dumper     */
/* See https://bugzilla.mozilla.org/show_bug.cgi?id=575675#c30 */
.link,
.link:visited {
  color: #0091ff;
}
.link,
.helplink,
.link:visited,
.helplink:visited {
  text-decoration: none;
}
.link:hover {
  text-decoration: underline;
}
Rather, the rules that are missing from Linux.
Attached patch Patch for bug 1010959 (obsolete) — Splinter Review
Here is a patch that:

1. Deletes computedview.css from Windows and Linux themes
2. Moves computedview.css from MacOSX theme to /shared
3. Adds stars and fixes pathes in jar.mn files in mac/windows/linux themes
4. Adds an %if condition to the CSS so that rules not present in Linux version of computedview are only for XP_MACOSX and XP_WIN

I am new to this jar thing, css conditions, etc. so please keep an eye out for syntax errors, obvious things, etc. Not even sure I am doing it right :)

Also, I had a little bit of an issue creating my patch with hg, so the formatting might not be right. I just copied the first few lines from a previous patch file, added a new title, but there is no "#parent" reference.
Attachment #8423631 - Flags: review?(bgrinstead)
> Also, I had a little bit of an issue creating my patch with hg, so the
> formatting might not be right. I just copied the first few lines from a
> previous patch file, added a new title, but there is no "#parent" reference.

It seems like I will be able to apply (except that there are merge errors right now before the color swatch changes haven't yet landed, but they should soon).  But manually formatting the first couple of lines is not something you will want to deal with in the future.  See this documentation for more info about generating the patches, maybe it will help get that sorted out: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F.
Comment on attachment 8423631 [details] [diff] [review]
Patch for bug 1010959

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

OK, good find on the multiple preprocessor checks in the CSS, I've actually not seen that - seems handy.  However, I've checked about the history behind that conditional rule and checked the UI on Linux with the extra rules (it looks the same).  I think we are OK to remove the conditional here, and remove the * from the jar.min files after all.
Attachment #8423631 - Flags: review?(bgrinstead)
Bgrins, yeah I am new to Mercurial Queues and I think I accidentally started two patches at once and then deleted one or some such, and it made things wonky.

I will remove the CSS condition and the jar stars, and reattach the patch.
Ok, this patch now just updates jar files with new paths (no stars), removes the duplicate CSS files, and removes the CSS conditions.

I have also moved the now non-conditional CSS lower down in the document, below the main .link class, since as far as I can tell it no longer needs any special treatment by being at the top of the file.
Attachment #8423631 - Attachment is obsolete: true
Attachment #8423965 - Flags: review?(bgrinstead)
Whiteboard: [mentor=bgrins][lang=css]
Comment on attachment 8423965 [details] [diff] [review]
Another patch, removing jar *s and CSS conditions

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

Looks good to me, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=85300f29bd6a
Attachment #8423965 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/integration/fx-team/rev/40fae2294f22
https://tbpl.mozilla.org/?tree=Fx-Team&rev=40fae2294f22
Whiteboard: [mentor=bgrins][lang=css] → [mentor=bgrins][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/40fae2294f22
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bgrins][lang=css][fixed-in-fx-team] → [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.