Closed Bug 1178548 Opened 7 years ago Closed 7 years ago

Clean up more SVGs

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: ntim, Assigned: ntim)

Details

Attachments

(2 obsolete files)

This file was already cleaned up, but taking a look again, we can push this further :
- There is a fill style declaration that is overriding a fill attribute. We can just set this fill once here.
https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/themes/shared/heartbeat-star-lit.svg#l1.444
- There is a bunch of useless whitespace within the path defs
- Those fill-rule attributes can probably be removed, but I need to check first
Heh, even better, I've just found out that most of the paths in that file are offscreen, and only one of them is relevant.
(In reply to Tim Nguyen [:ntim] from comment #1)
> Heh, even better, I've just found out that most of the paths in that file
> are offscreen, and only one of them is relevant.

When you reveal them, those off-screen paths include 2 back icons, a back icon, a forward icon, a home icon, a unfilled and filled bookmark icon, a history icon and a downloads icon.
Summary: Further clean up to heartbeat-star-lit.svg → Clean up more SVGs
Attached patch Patch (obsolete) — Splinter Review
Couldn't see visible changes.
Attachment #8627463 - Flags: review?(dao)
Attachment #8627463 - Flags: review?(dao) → review?(gijskruitbosch+bugs)
Comment on attachment 8627463 [details] [diff] [review]
Patch

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

Egh. Are there lots of whitespace changes? Why all the changes to lines that look identical? Can you either push to mozreview or attach a diff -w ? Thanks!
Attachment #8627463 - Flags: review?(gijskruitbosch+bugs)
Bug 1178548 - Clean up more SVGs. r=Gijs
Attachment #8646423 - Flags: review?(gijskruitbosch+bugs)
Attachment #8627463 - Attachment is obsolete: true
Comment on attachment 8646423 [details]
MozReview Request: Bug 1178548 - Clean up more SVGs. r?MattN

https://reviewboard.mozilla.org/r/15739/#review14025

I don't think I can review the heartbeat SVG changes because I don't understand why that file is the way it is and why what you're doing is OK (or not). Please get review from Matt for those changes.

::: browser/themes/windows/caption-buttons.svg:30
(Diff revision 1)
>      }
> -
> +    .themes {
> -    g.themes {

I don't understand why you're removing whitespace here. You do it in some places in this file, but not others, and you've moved some rules and added whitespace there. Why do we need to mess with this file quite so much? I thought just adding some classes and replacing the id attribute selectors with class selectors was going to be enough. Isn't it?

::: browser/themes/shared/drm-icon.svg:14
(Diff revision 1)
> -      fill: black;
> +      fill: #000;

There is no need to do this.
Attachment #8646423 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8646423 [details]
> MozReview Request: Bug 1178548 - Clean up more SVGs. r=Gijs
> 
> https://reviewboard.mozilla.org/r/15739/#review14025
> 
> I don't think I can review the heartbeat SVG changes because I don't
> understand why that file is the way it is and why what you're doing is OK
> (or not). Please get review from Matt for those changes.
For the heartbeat star icons, the paths I've removed appeared offscreen, those include a back/forward icon, a history icon and a downloads icon. The files clearly have `star` in their names, and looking at the UI, I suspect we only want the star.

For the heartbeat heart icon, there were some useless style resets (the first <g> element makes the shape invisible, then the children make the shapes visible again). There was also a <g> element applying a transform to the children, and some of the children also had transforms, so I combined them in one when possible.

Anyway, there were no path data change in any of the files, and I've tested these changes.

> ::: browser/themes/windows/caption-buttons.svg:30
> (Diff revision 1)
> >      }
> > -
> > +    .themes {
> > -    g.themes {
> 
> I don't understand why you're removing whitespace here. You do it in some
> places in this file, but not others, and you've moved some rules and added
> whitespace there. Why do we need to mess with this file quite so much? I
> thought just adding some classes and replacing the id attribute selectors
> with class selectors was going to be enough. Isn't it?
> 
> ::: browser/themes/shared/drm-icon.svg:14
> (Diff revision 1)
> > -      fill: black;
> > +      fill: #000;
> 
> There is no need to do this.
Most of our SVGs use the shorthex/hex format for colors (including the rest of the drm-icon file), it's nice to be consistent here.
(In reply to :Gijs Kruitbosch from comment #6)
> ::: browser/themes/windows/caption-buttons.svg:30
> (Diff revision 1)
> >      }
> > -
> > +    .themes {
> > -    g.themes {
> 
> I don't understand why you're removing whitespace here. You do it in some
> places in this file, but not others, and you've moved some rules and added
> whitespace there. Why do we need to mess with this file quite so much? I
> thought just adding some classes and replacing the id attribute selectors
> with class selectors was going to be enough. Isn't it?

Didn't realize I forgot to reply to this comment. 
I'm moving rules because I wanted to order them in a logical order.

The first block is for the symlink styles (hiding shapes depending on the url symlink).
The second block are the most global styles that affect most of the shapes.
The third block is for the theme specific styles (default, highcontrast, lw-theme, white).
The last block is for the helper styles (outer-stroke, restore-background-window)
Comment on attachment 8646423 [details]
MozReview Request: Bug 1178548 - Clean up more SVGs. r?MattN

Bug 1178548 - Clean up more SVGs. r?MattN
Attachment #8646423 - Attachment description: MozReview Request: Bug 1178548 - Clean up more SVGs. r=Gijs → MozReview Request: Bug 1178548 - Clean up more SVGs. r?MattN
Attachment #8646423 - Flags: review?(MattN+bmo)
Attachment #8646423 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8646423 [details]
MozReview Request: Bug 1178548 - Clean up more SVGs. r?MattN

https://reviewboard.mozilla.org/r/15739/#review14191

Ship It!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fb143f3b36a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Attachment #8646423 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.