1160771 - Clean up SVGs in browser theme

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Tim Nguyen :ntim]

Most of our SVG's have useless attributes or tags.

Here are some frequently seen useless things :
- x and y attributes on the <svg> root element
- Useless enable-background attribute (which isn't supported by Gecko)
- This : `<?xml version="1.0" encoding="utf-8"?>`

Here's a typical SVG clean up process : attachment 8564590 [details] [diff] [review]
It has pretty much a list of all the things that need to be changed/removed.

Comment 1

[Tim Nguyen :ntim]

List of SVGs :
- Browser directory : https://dxr.mozilla.org/mozilla-central/search?q=path%3Abrowser%2F*.svg&case=false
- Toolkit directory : https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2F*.svg&case=false

Other tips to clean up :
- Remove unnecessary IDs (unreferenced in the code)
- Remove version attribute from SVG tag.

We probably want to add a license header to each svg file too :
<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

Comment 2

[Tim Nguyen :ntim]

Note that this process should not be done in the browser/extensions/pdfjs directory, as the changes will be overwritten by the next PDF.JS merge.

Comment 3

[Miles Richardson]

I'd like to work on this as my first bug to get into the swing of things. I'll let you know if I have any questions about the contribution process.

Comment 4

[Miles Richardson]

A couple questions:

(1) There's quite a few SVG files. Should I submit a separate patch for each file, or a single patch with all of the changes?

(2) I see that in the included example (attachment 8564590 [details] [diff] [review]) the '<?xml ... ?>' tag was left at the top of the SVG file, but comment 1 mentions that this tag is useless. Is it safe to remove this tag from all the files?

Comment 5

[Tim Nguyen :ntim]

(In reply to Miles Richardson from comment #4)
> (1) There's quite a few SVG files. Should I submit a separate patch for each
> file, or a single patch with all of the changes?
A single patch file with all the changes is enough.

> (2) I see that in the included example (attachment 8564590 [details] [diff] [review]
> [review]) the '<?xml ... ?>' tag was left at the top of the SVG file, but
> comment 1 mentions that this tag is useless. Is it safe to remove this tag
> from all the files?
On second thought, if the file currently has it, it's better to keep it there. You don't need to add it if it's missing.

Comment 6

[Miles Richardson]

In the example (attachment 8564590 [details] [diff] [review]) the path element was collapsed onto a single line. However, some SVG files have very large paths that would span thousands of characters if collapsed.

Example: https://dxr.mozilla.org/mozilla-central/source/browser/branding/nightly/content/silhouette-40.svg

Should I worry about collapsing large elements like this while I'm cleaning up the SVG files?

Comment 7

[Tim Nguyen :ntim]

(In reply to Miles Richardson from comment #6)
> In the example (attachment 8564590 [details] [diff] [review]) the path
> element was collapsed onto a single line. However, some SVG files have very
> large paths that would span thousands of characters if collapsed.
> 
> Example:
> https://dxr.mozilla.org/mozilla-central/source/browser/branding/nightly/
> content/silhouette-40.svg
> 
> Should I worry about collapsing large elements like this while I'm cleaning
> up the SVG files?

Nope, it's better to keep it that way (80 chars per line).
Although, you might want to change the fill to lowercase short hex : #ccc instead #CCCCCC.

Comment 8

[Miles Richardson]

Attachment: Patch v1

Created Patch v1

This patch tidies up all of the SVG files in browser/ and toolkit/ except for the PDF.JS files.

How do I go about getting this patch reviewed?

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8606525 [details] [diff] [review]
Patch v1

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

Nice work ! I have a couple of comments, but other than that, it's good to go !

::: browser/base/content/test/general/title_test.svg
@@ +1,1 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

nit : Tests don't need a license header.

::: browser/components/loop/standalone/content/img/gum-opera.svg
@@ +6,3 @@
>    <g stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">
>      <g transform="translate(0.500000, 0.000000)">
> +      <rect stroke="#aae5f9" fill="#FFFFFF" x="0.990196078" y="1" width="99.0196078" height="60"/>

nit : #FFFFFF in short lowercase hex please

::: browser/themes/shared/aboutNetError_alert.svg
@@ +2,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 40 40">
> +	<linearGradient id="gradient1" gradientUnits="userSpaceOnUse" x1="20" y1="4" x2="20" y2="36">

I just noticed this file has tab indenting, can you change it to 2 spaces ?

::: browser/themes/shared/devtools/app-manager/images/index-icons.svg
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="160px" height="240px" viewBox="0 0 160 240">
> +  <rect y="40" display="none" fill="#22272d" width="84" height="160"/>
> +  <rect x="80.75" y="40" display="none" fill="#194866" width="84" height="160"/>

Does removing those hidden rects affect the SVG appearance ?
If not, can you remove them ?

::: browser/themes/shared/devtools/images/add.svg
@@ +4,2 @@
>  <svg width="18" height="18" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> +  <polygon fill="#eef0f2" points="4,7 8,7 8,3 10,3 10,7 14,7 14,9 10,9 10,13 8,13 8,9 4,9 4,7"/>

This file will get updated in bug 1136101. I will take care of cleaning up this file.

::: browser/themes/shared/devtools/images/filter-swatch.svg
@@ +12,1 @@
>    <g id="addpage-shape">

This id is unnecessary, can you remove it ?

::: browser/themes/shared/devtools/images/power.svg
@@ +9,5 @@
>  
>  The software is provided “as is”, without warranty of any kind, express or implied, including but not limited to the warranties of merchantability, fitness for a particular purpose and noninfringement. In no event shall the authors or copyright holders be liable for any claim, damages or other liability, whether in an action of contract, tort or otherwise, arising from, out of or in connection with the software or the use or other dealings in the software.
>  -->
>  <svg width="16" height="16" xmlns="http://www.w3.org/2000/svg">
> +  <path stroke="#edf0f1" stroke-width="0" fill="#edf0f1" d="m10.89891,2.50043c-0.49827,-0.24134 -1.09841,-0.03411 -1.34129,0.46514c-0.24185,0.49928 -0.03311,1.09942 0.46517,1.34128c1.56306,0.76071 2.64193,2.36094 2.64092,4.21555c-0.00501,2.58626 -2.09749,4.6787 -4.68322,4.68321c-2.58623,-0.005 -4.67869,-2.09746 -4.68371,-4.68321c-0.001,-1.85561 1.07834,-3.45731 2.64294,-4.21654c0.49928,-0.24185 0.7065,-0.84201 0.46514,-1.34129c-0.24185,-0.49825 -0.84098,-0.70697 -1.34029,-0.46513c-2.23396,1.08135 -3.77446,3.37351 -3.77545,6.02296c0.00099,3.69518 2.99518,6.68989 6.69138,6.69088c3.6957,-0.00099 6.69037,-2.9957 6.69089,-6.69088c-0.00102,-2.64846 -1.53948,-4.9391 -3.77247,-6.02197zm-2.91842,4.9346c0.55398,0 1.00309,-0.44861 1.00309,-1.00357l0,-4.68373c0,-0.55446 -0.44911,-1.00309 -1.00309,-1.00309c-0.555,0 -1.00358,0.44911 -1.00358,1.00309l0,4.68321c0,0.55499 0.44858,1.00409 1.00358,1.00409z"/>

Does removing stroke="#edf0f1" stroke-width="0" affect the appearance of the file ?

stroke-width=0 means that there is no stroke, which is the default behaviour.

::: browser/themes/shared/devtools/images/tool-network.svg
@@ +27,5 @@
> +    <g opacity=".85">
> +      <path fill="#edf0f1" d="m6.4,12.1c.4,0 .7,.3 .7,.7 0,.4-.3,.7-.7,.7h-2.1c-.4,0-.7-.3-.7-.7 0-.4 .3-.7 .7-.7h2.1m0-.4h-2.1c-.6,0-1.1,.5-1.1,1.1 0,.6 .5,1.1 1.1,1.1h2.1c.6,0 1.1-.5 1.1-1.1-.1-.6-.5-1.1-1.1-1.1z"/>
> +    </g>
> +  </g>
> +  <g opacity="5.000000e-02" fill="#edf0f1" fill-rule="evenodd">

5.000000e-02 is equivalent to 0.05

::: browser/themes/shared/heartbeat-star-lit.svg
@@ +46,5 @@
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#00a3f2" d="M2,115h7c0.6,0,1-0.4,1-1v-4c0-0.6-0.4-1-1-1H2c-0.6,0-1,0.4-1,1    v4C1,114.6,1.4,115,2,115z M14,109h-2c-0.6,0-1,0.4-1,1v4c0,0.6,0.4,1,1,1h2c0.6,0,1-0.4,1-1v-4C15,109.4,14.6,109,14,109z     M14,116H8c-0.6,0-1,0.4-1,1v5c0,0.6,0.4,1,1,1h6c0.6,0,1-0.4,1-1v-5C15,116.4,14.6,116,14,116z M5,116H2c-0.6,0-1,0.4-1,1v5    c0,0.6,0.4,1,1,1h3c0.6,0,1-0.4,1-1v-5C6,116.4,5.6,116,5,116z"/>
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#00a3f2" d="M45.8,116c0,0-0.6,0.8-1.8,0.8c-1.2,0-1.8-0.8-1.8-0.8l-6-5.1  c0.3-0.5,0.9-0.9,1.6-0.9h12.4c0.7,0,1.3,0.4,1.6,0.9L45.8,116z M42.2,117.7c0,0,0.6,0.8,1.8,0.8c1.2,0,1.8-0.8,1.8-0.8l6.2-5.4v8  c0,0.9-0.8,1.7-1.8,1.7H37.8c-1,0-1.8-0.8-1.8-1.7v-8L42.2,117.7z"/>
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#00a3f2" d="M-237.5,145h-13c-0.8,0-1.5,0.7-1.5,1.5v11    c0,0.8,0.7,1.5,1.5,1.5h13c0.8,0,1.5-0.7,1.5-1.5v-11C-236,145.7-236.7,145-237.5,145z M-245.5,146c0.3,0,0.5,0.2,0.5,0.5    c0,0.3-0.2,0.5-0.5,0.5c-0.3,0-0.5-0.2-0.5-0.5C-246,146.2-245.8,146-245.5,146z M-247.6,146c0.3,0,0.5,0.2,0.5,0.5    c0,0.3-0.2,0.5-0.5,0.5s-0.5-0.2-0.5-0.5C-248.1,146.2-247.8,146-247.6,146z M-249.5,146c0.3,0,0.5,0.2,0.5,0.5    c0,0.3-0.2,0.5-0.5,0.5s-0.5-0.2-0.5-0.5C-250,146.2-249.8,146-249.5,146z M-250,156v-6c0-0.6,0.4-1,1-1h7v8h-7    C-249.6,157-250,156.6-250,156z M-238,156c0,0.6-0.4,1-1,1h-1v-8h1c0.6,0,1,0.4,1,1V156z"/>
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#00a3f2" d="M-213.2,156c-1.3-1.2-2.2-2.8-2.2-4.6c0-3.6,3.3-6.5,7.4-6.5  c4.1,0,7.4,2.9,7.4,6.5c0,3.6-3.3,6.5-7.4,6.5c-0.8,0-1.6-0.1-2.4-0.3c-1.8,0.7-4.3,1.7-4.5,1.4C-213.9,157.9-213.5,156.8-213.2,156  z"/>
> +  <rect fill="none" width="16" height="16"/>

This rect is invisible (fill=none), so it can be removed.

::: browser/themes/shared/heartbeat-star-off.svg
@@ +46,5 @@
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#231f20" d="M2,115h7c0.6,0,1-0.4,1-1v-4c0-0.6-0.4-1-1-1H2c-0.6,0-1,0.4-1,1    v4C1,114.6,1.4,115,2,115z M14,109h-2c-0.6,0-1,0.4-1,1v4c0,0.6,0.4,1,1,1h2c0.6,0,1-0.4,1-1v-4C15,109.4,14.6,109,14,109z     M14,116H8c-0.6,0-1,0.4-1,1v5c0,0.6,0.4,1,1,1h6c0.6,0,1-0.4,1-1v-5C15,116.4,14.6,116,14,116z M5,116H2c-0.6,0-1,0.4-1,1v5    c0,0.6,0.4,1,1,1h3c0.6,0,1-0.4,1-1v-5C6,116.4,5.6,116,5,116z"/>
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#231f20" d="M45.8,116c0,0-0.6,0.8-1.8,0.8c-1.2,0-1.8-0.8-1.8-0.8l-6-5.1  c0.3-0.5,0.9-0.9,1.6-0.9h12.4c0.7,0,1.3,0.4,1.6,0.9L45.8,116z M42.2,117.7c0,0,0.6,0.8,1.8,0.8c1.2,0,1.8-0.8,1.8-0.8l6.2-5.4v8  c0,0.9-0.8,1.7-1.8,1.7H37.8c-1,0-1.8-0.8-1.8-1.7v-8L42.2,117.7z"/>
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#231f20" d="M-237.5,145h-13c-0.8,0-1.5,0.7-1.5,1.5v11    c0,0.8,0.7,1.5,1.5,1.5h13c0.8,0,1.5-0.7,1.5-1.5v-11C-236,145.7-236.7,145-237.5,145z M-245.5,146c0.3,0,0.5,0.2,0.5,0.5    c0,0.3-0.2,0.5-0.5,0.5c-0.3,0-0.5-0.2-0.5-0.5C-246,146.2-245.8,146-245.5,146z M-247.6,146c0.3,0,0.5,0.2,0.5,0.5    c0,0.3-0.2,0.5-0.5,0.5s-0.5-0.2-0.5-0.5C-248.1,146.2-247.8,146-247.6,146z M-249.5,146c0.3,0,0.5,0.2,0.5,0.5    c0,0.3-0.2,0.5-0.5,0.5s-0.5-0.2-0.5-0.5C-250,146.2-249.8,146-249.5,146z M-250,156v-6c0-0.6,0.4-1,1-1h7v8h-7    C-249.6,157-250,156.6-250,156z M-238,156c0,0.6-0.4,1-1,1h-1v-8h1c0.6,0,1,0.4,1,1V156z"/>
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#231f20" d="M-213.2,156c-1.3-1.2-2.2-2.8-2.2-4.6c0-3.6,3.3-6.5,7.4-6.5  c4.1,0,7.4,2.9,7.4,6.5c0,3.6-3.3,6.5-7.4,6.5c-0.8,0-1.6-0.1-2.4-0.3c-1.8,0.7-4.3,1.7-4.5,1.4C-213.9,157.9-213.5,156.8-213.2,156  z"/>
> +  <rect fill="none" width="16" height="16"/>

invisible rect here too

::: browser/themes/shared/tab-selected.svg
@@ +5,2 @@
>    <defs>
>      <style><![CDATA[

You can remove the <![CDATA[

@@ +26,5 @@
>            background-color: @customToolbarColor@;
>          }
>        }
>  %endif
>      ]]></style>

...and the end tag that comes with <![CDATA[

::: toolkit/themes/linux/global/icons/close.svg
@@ +6,2 @@
>    <defs>
>      <style type="text/css"><![CDATA[

<![CDATA[ can be removed

@@ +42,4 @@
>        .icon-background-hover-active {
>          fill: #b32c12;
>        }
>        ]]></style>

]]> too

::: toolkit/themes/shared/reader/RM-Minus-24x24.svg
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 24 24">
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#808080" d="M0,13.5v-3h24v3H0z"/>
> +  <g enable-background="new">

Can you remove this g tag (but keep the contents of course) ? it is fairly useless (enable-background has no effect).

::: toolkit/themes/shared/reader/RM-Plus-24x24.svg
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 24 24">
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#808080" d="M24,13.5H13.5V24h-3V13.5H0v-3h10.5V0h3v10.5H24V13.5z"/>
> +  <g enable-background="new">

Same comment about the g tag

::: toolkit/themes/shared/reader/RM-Reading-List-24x24.svg
@@ +8,5 @@
> +  <rect x="8" y="4" fill-rule="evenodd" clip-rule="evenodd" fill="#808080" width="14" height="4"/>
> +  <circle fill-rule="evenodd" clip-rule="evenodd" fill="#808080" cx="4" cy="6" r="2"/>
> +  <circle fill-rule="evenodd" clip-rule="evenodd" fill="#808080" cx="4" cy="12" r="2"/>
> +  <circle fill-rule="evenodd" clip-rule="evenodd" fill="#808080" cx="4" cy="18" r="2"/>
> +  <g enable-background="new">

Same comment about the g tag

::: toolkit/themes/shared/reader/RM-Type-Controls-Arrow.svg
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 24 24">
> +  <polygon opacity="0.15" fill-rule="evenodd" clip-rule="evenodd" points="16.583,0.015 16.569,0 4.583,12 16.569,24 16.583,23.985"/>
> +  <g enable-background="new">

Same comment about the g tag

::: toolkit/themes/windows/global/media/videoClickToPlayButton.svg
@@ +1,4 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" id="videoPlayButtonOverlay" preserveAspectRatio="xMinYMin meet" viewBox="0 0 64 64">

The id in the svg tag can be removed

Comment 10

[Tim Nguyen :ntim]

(In reply to Miles Richardson from comment #8)
> Created attachment 8606525 [details] [diff] [review]
> Patch v1
> 
> Created Patch v1
> 
> This patch tidies up all of the SVG files in browser/ and toolkit/ except
> for the PDF.JS files.
> 
> How do I go about getting this patch reviewed?
You can set the review flag on the patch to me, or to a peer.

Comment 11

[Miles Richardson]

Attachment: Patch v2

I incorporated all of the feedback from comment #9.

Tim Nguyen, apparently you have review requests disabled, so I'm just setting the needinfo flag. Feel free to delegate the review as you see fit.

Comment 12

[Robert Longson [:longsonr]]

Some general observations.

Do not set display:none on <defs> elements they are naturally not displayed.

Try to avoid <use> as much as possible. If there's only on user of a <use> element then just replace the <use> with the element itself.

There are a number of clip-path elements that don't seem to be used e.g. clippath4. Removing clippath4 may well allow you to remove clippath3 etc.

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8606684 [details] [diff] [review]
Patch v2

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

Nice job ! Here are a couple of comments.

--

These files have type attributes on their style element(s) which can be removed :
browser/themes/shared/devtools/images/search-clear-*.svg
browser/themes/shared/search/history-icon.svg
toolkit/themes/shared/in-content/check*.svg
browser/themes/shared/newtab/controls.svg
browser/themes/shared/reader/readerMode.svg
browser/themes/shared/readinglist/icons.svg
toolkit/themes/linux/global/icons/close.svg
toolkit/themes/shared/reader/RM-Close-24x24.svg
--

These files have display: none set on their defs element which is not needed as defs is already hidden by default :
browser/components/loop/content/shared/img/
browser/themes/shared/devedition/search.svg
browser/themes/*/content-contextmenu.svg
browser/themes/shared/devedition/urlbar-history-dropmarker.svg
browser/themes/shared/devtools/images/timeline-filter.svg
toolkit/themes/linux/global/icons/autocomplete-search.svg
toolkit/themes/shared/reader/pocket.svg
browser/themes/shared/incontentprefs/icons.svg
browser/themes/shared/search/history-icon.svg

::: browser/components/loop/content/shared/img/audio-call-avatar.svg
@@ +2,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xl="http://www.w3.org/1999/xlink" viewBox="129 5 252 253" width="21pc" height="253pt">
> +  <defs>

This defs tag, and the contents in it are useless.

::: browser/components/loop/content/shared/img/beta-ribbon.svg
@@ +18,3 @@
>      </g>
> +  </defs>
> +  <use id="beta-ribbon" xlink:href="#beta-ribbon"/>

Since this file only uses 1 use tag. You can remove this use tag, and unwrap the shapes from the defs tag (and from g#beta-ribbon too).

::: browser/components/loop/content/shared/img/icons-16x16.svg
@@ +44,5 @@
> +    <path id="settings-shape" fill-rule="evenodd" clip-rule="evenodd" d="M14.77,8c0,0.804,0.262,1.548,0.634,1.678L16,9.887 c-0.205,0.874-0.553,1.692-1.011,2.434l-0.567-0.272c-0.355-0.171-1.066,0.17-1.635,0.738c-0.569,0.569-0.909,1.279-0.738,1.635 l0.273,0.568c-0.741,0.46-1.566,0.79-2.438,0.998l-0.205-0.584c-0.13-0.372-0.874-0.634-1.678-0.634s-1.548,0.262-1.678,0.634 l-0.209,0.596c-0.874-0.205-1.692-0.553-2.434-1.011l0.272-0.567c0.171-0.355-0.17-1.066-0.739-1.635 c-0.568-0.568-1.279-0.909-1.635-0.738l-0.568,0.273c-0.46-0.741-0.79-1.566-0.998-2.439l0.584-0.205 C0.969,9.547,1.231,8.804,1.231,8c0-0.804-0.262-1.548-0.634-1.678L0,6.112c0.206-0.874,0.565-1.685,1.025-2.427l0.554,0.266 c0.355,0.171,1.066-0.17,1.635-0.738c0.569-0.568,0.909-1.28,0.739-1.635L3.686,1.025c0.742-0.46,1.553-0.818,2.427-1.024 l0.209,0.596C6.453,0.969,7.197,1.23,8.001,1.23s1.548-0.262,1.678-0.634l0.209-0.596c0.874,0.205,1.692,0.553,2.434,1.011 l-0.272,0.567c-0.171,0.355,0.17,1.066,0.738,1.635c0.569,0.568,1.279,0.909,1.635,0.738l0.568-0.273 c0.46,0.741,0.79,1.566,0.998,2.438l-0.584,0.205C15.032,6.452,14.77,7.196,14.77,8z M8.001,3.661C5.604,3.661,3.661,5.603,3.661,8 c0,2.397,1.943,4.34,4.339,4.34c2.397,0,4.339-1.943,4.339-4.34C12.34,5.603,10.397,3.661,8.001,3.661z"/>
> +    <g id="share-shape">
> +      <path fill="transparent" d="M11.704,12.375H7.556c-0.183,0-0.353-0.071-0.48-0.199c-0.124-0.125-0.191-0.29-0.19-0.464V10.09h-2.59 c-0.37,0-0.671-0.296-0.671-0.661V4.286c0-0.365,0.301-0.661,0.671-0.661h3.384L7.817,3.73l1.299,1.254v0.927h1.851 l1.408,1.359v4.444C12.375,12.079,12.074,12.375,11.704,12.375z M7.635,11.625h3.989V7.588l-0.961-0.927H7.635V11.625z M4.375,9.34h2.5V6.561c0-0.365,0.301-0.661,0.671-0.661h0.82V5.302L7.405,4.375h-3.03V9.34z"/>
> +      <polygon fill="transparent" points="10.222,8 10.222,6.857 11.407,8"/>
> +      <polygon fill="transparent" points="6.963,5.714 6.963,4.571 8.148,5.714"/>

There are some invisible polygons that can be removed

::: browser/themes/linux/content-contextmenu.svg
@@ +17,5 @@
> +      fill: graytext;
> +    }
> +  </style>
> +  <defs style="display:none">
> +    <path id="back-shape" fill-rule="evenodd" clip-rule="evenodd" d="M1.192,8.893L2.21,9.964c0.064,0.065,0.136,0.117,0.214,0.159 l5.199,5.301c0.607,0.63,1.465,0.764,1.915,0.297l1.02-1.082c0.449-0.467,0.32-1.357-0.288-1.99l-2.116-2.158h5.705 c0.671,0,1.215-0.544,1.215-1.215v-2.43c0-0.671-0.544-1.215-1.215-1.215H8.094l2.271-2.309c0.609-0.626,0.737-1.512,0.288-1.974 L9.635,0.278C9.184-0.188,8.327-0.055,7.718,0.575L2.479,5.901C2.38,5.946,2.289,6.008,2.21,6.089L1.192,7.171 c-0.21,0.219-0.293,0.53-0.26,0.864C0.899,8.367,0.981,8.676,1.192,8.893z"/> 

nit : trailing whitespace

@@ +18,5 @@
> +    }
> +  </style>
> +  <defs style="display:none">
> +    <path id="back-shape" fill-rule="evenodd" clip-rule="evenodd" d="M1.192,8.893L2.21,9.964c0.064,0.065,0.136,0.117,0.214,0.159 l5.199,5.301c0.607,0.63,1.465,0.764,1.915,0.297l1.02-1.082c0.449-0.467,0.32-1.357-0.288-1.99l-2.116-2.158h5.705 c0.671,0,1.215-0.544,1.215-1.215v-2.43c0-0.671-0.544-1.215-1.215-1.215H8.094l2.271-2.309c0.609-0.626,0.737-1.512,0.288-1.974 L9.635,0.278C9.184-0.188,8.327-0.055,7.718,0.575L2.479,5.901C2.38,5.946,2.289,6.008,2.21,6.089L1.192,7.171 c-0.21,0.219-0.293,0.53-0.26,0.864C0.899,8.367,0.981,8.676,1.192,8.893z"/> 
> +    <path id="forward-shape" fill-rule="evenodd" clip-rule="evenodd" d="M14.808,7.107L13.79,6.036c-0.064-0.065-0.136-0.117-0.214-0.159 L8.377,0.576C7.77-0.054,6.912-0.189,6.461,0.278L5.441,1.36c-0.449,0.467-0.32,1.357,0.288,1.99l2.116,2.158H2.14 c-0.671,0-1.215,0.544-1.215,1.215v2.43c0,0.671,0.544,1.215,1.215,1.215h5.765l-2.271,2.309c-0.609,0.626-0.737,1.512-0.288,1.974 l1.019,1.072c0.451,0.465,1.308,0.332,1.917-0.297l5.238-5.326c0.1-0.045,0.191-0.107,0.269-0.188l1.019-1.082 c0.21-0.219,0.293-0.53,0.26-0.864C15.101,7.633,15.019,7.324,14.808,7.107z"/> 

nit : trailing whitespace

::: browser/themes/shared/aboutNetError_alert.svg
@@ +7,5 @@
> +    <stop offset="0" style="stop-color:#e63b2e"/>
> +    <stop offset="1" style="stop-color:#c33931"/>
> +  </linearGradient>
> +  <path fill="url(#gradient1)" d="M13.373,4L4,13.372v13.256L13.373,36h13.255L36,26.628V13.372L26.627,4H13.373z M22.176,8.704 l-0.48,14.304h-3.424L17.76,8.704H22.176z M20,31.296c-1.44,0-2.592-1.184-2.592-2.592c0-1.44,1.152-2.592,2.592-2.592 c1.472,0,2.592,1.152,2.592,2.592C22.592,30.112,21.472,31.296,20,31.296z"/>
> +  <linearGradient id="gradient2" gradientUnits="userSpaceOnUse" x1="20" y1="-3.728928e-10" x2="20" y2="40">

The value of y1 here is basically 0

::: browser/themes/shared/devtools/images/filetypes/dir-open.svg
@@ +5,1 @@
>  <svg viewBox="0 0 512 512" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" fill="#000000" width="16" height="16">

You can use the short hex for the fill.

::: browser/themes/shared/heartbeat-icon.svg
@@ +6,5 @@
> +  <defs>
> +    <path id="path-1" d="M144,246.857143 C141.214272,246.857143 138.857152,245.892867 136.928571,243.964286 L36.6428571,147.214286 C35.5714232,146.357139 34.0982237,144.964295 32.2232143,143.035714 C30.3482049,141.107133 27.3750204,137.59824 23.3035714,132.508929 C19.2321225,127.419617 15.5893018,122.196455 12.375,116.839286 C9.16069821,111.482116 6.29465545,105.000038 3.77678571,97.3928571 C1.25891598,89.7856763 0,82.392893 0,75.2142857 C0,51.6427393 6.80350339,33.2143521 20.4107143,19.9285714 C34.0179252,6.64279071 52.8213086,0 76.8214286,0 C83.4643189,0 90.2410369,1.1517742 97.1517857,3.45535714 C104.062535,5.75894009 110.491042,8.86605187 116.4375,12.7767857 C122.383958,16.6875196 127.499979,20.3571257 131.785714,23.7857143 C136.07145,27.2143029 140.142838,30.8571236 144,34.7142857 C147.857162,30.8571236 151.92855,27.2143029 156.214286,23.7857143 C160.500021,20.3571257 165.616042,16.6875196 171.5625,12.7767857 C177.508958,8.86605187 183.937465,5.75894009 190.848214,3.45535714 C197.758963,1.1517742 204.535681,0 211.178571,0 C235.178691,0 253.982075,6.64279071 267.589286,19.9285714 C281.196497,33.2143521 288,51.6427393 288,75.2142857 C288,98.8929755 275.732266,122.999877 251.196429,147.535714 L151.071429,243.964286 C149.142847,245.892867 146.785728,246.857143 144,246.857143 L144,246.857143 Z"/>
> +  </defs>
> +  <g stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">
> +    <g transform="translate(0.000000, -1.000000)">

Those numbers don't need excessive precision, translate(0,-1) should be enough

@@ +8,5 @@
> +  </defs>
> +  <g stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">
> +    <g transform="translate(0.000000, -1.000000)">
> +      <path fill="#d74345" d="M144,248.571429 C141.214272,248.571429 138.857152,247.607152 136.928571,245.678571 L36.6428571,148.928571 C35.5714232,148.071424 34.0982237,146.678581 32.2232143,144.75 C30.3482049,142.821419 27.3750204,139.312525 23.3035714,134.223214 C19.2321225,129.133903 15.5893018,123.910741 12.375,118.553571 C9.16069821,113.196402 6.29465545,106.714324 3.77678571,99.1071429 C1.25891598,91.499962 0,84.1071788 0,76.9285714 C0,53.357025 6.80350339,34.9286379 20.4107143,21.6428571 C34.0179252,8.35707643 52.8213086,1.71428571 76.8214286,1.71428571 C83.4643189,1.71428571 90.2410369,2.86605991 97.1517857,5.16964286 C104.062535,7.4732258 110.491042,10.5803376 116.4375,14.4910714 C122.383958,18.4018053 127.499979,22.0714114 131.785714,25.5 C136.07145,28.9285886 140.142838,32.5714093 144,36.4285714 C147.857162,32.5714093 151.92855,28.9285886 156.214286,25.5 C160.500021,22.0714114 165.616042,18.4018053 171.5625,14.4910714 C177.508958,10.5803376 183.937465,7.4732258 190.848214,5.16964286 C197.758963,2.86605991 204.535681,1.71428571 211.178571,1.71428571 C235.178691,1.71428571 253.982075,8.35707643 267.589286,21.6428571 C281.196497,34.9286379 288,53.357025 288,76.9285714 C288,100.607261 275.732266,124.714163 251.196429,149.25 L151.071429,245.678571 C149.142847,247.607152 146.785728,248.571429 144,248.571429 L144,248.571429 Z"/>
> +      <g transform="translate(0.000000, 0.714286)">

Same comment about the number. translate(0, 0.71) should be enough.

::: toolkit/themes/linux/global/icons/close.svg
@@ +1,5 @@
>  <?xml version="1.0" encoding="utf-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg id="icon-close" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="96px" height="16px" viewBox="0 0 96 16">

The id here is useless

::: toolkit/themes/osx/global/media/videoClickToPlayButton.svg
@@ +5,3 @@
>    <defs>
>      <linearGradient id="whiteGradientStops">
> +      <stop id="whiteGradientStop01" style="stop-color:#fff;stop-opacity:.95" offset="0"/>

ids on those gradient stops aren't useful (although the ID on the linearGradient is)

@@ +13,3 @@
>      <linearGradient id="arrowGradientStops">
> +      <stop id="arrowGradientStop01" style="stop-color:#333;stop-opacity:.5" offset="0"/>
> +      <stop id="arrowGradientStop02" style="stop-color:#666;stop-opacity:.5" offset="1"/>

ids on those gradient stops aren't useful (although the ID on the linearGradient is)

::: toolkit/themes/shared/reader/RM-Minus-24x24.svg
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 24 24">
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#808080" d="M0,13.5v-3h24v3H0z"/>
> +  <defs>

Now that I have taken a deeper look, everything below this <defs> tag (including this <defs> tag) is useless (excluding the closing </svg> tag).

::: toolkit/themes/shared/reader/RM-Plus-24x24.svg
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 24 24">
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#808080" d="M24,13.5H13.5V24h-3V13.5H0v-3h10.5V0h3v10.5H24V13.5z"/>
> +  <defs>

Same comment about everything below this defs tag

::: toolkit/themes/shared/reader/RM-Reading-List-24x24.svg
@@ +8,5 @@
> +  <rect x="8" y="4" fill-rule="evenodd" clip-rule="evenodd" fill="#808080" width="14" height="4"/>
> +  <circle fill-rule="evenodd" clip-rule="evenodd" fill="#808080" cx="4" cy="6" r="2"/>
> +  <circle fill-rule="evenodd" clip-rule="evenodd" fill="#808080" cx="4" cy="12" r="2"/>
> +  <circle fill-rule="evenodd" clip-rule="evenodd" fill="#808080" cx="4" cy="18" r="2"/>
> +  <defs>

Same comment about all things below the defs tag.

::: toolkit/themes/shared/reader/RM-Type-Controls-Arrow.svg
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 24 24">
> +  <polygon opacity="0.15" fill-rule="evenodd" clip-rule="evenodd" points="16.583,0.015 16.569,0 4.583,12 16.569,24 16.583,23.985"/>
> +  <defs>

Same comment about everything below this defs tag. (except the polygon tag at the end of this file, which needs to be kept)

::: toolkit/themes/windows/global/media/videoClickToPlayButton.svg
@@ +6,5 @@
>      <linearGradient id="whiteGradientStops">
> +      <stop id="whiteGradientStop01" style="stop-color:#fff;stop-opacity:.95" offset="0"/>
> +      <stop id="whiteGradientStop02" style="stop-color:#fff;stop-opacity:.75" offset=".45"/>
> +      <stop id="whiteGradientStop03" style="stop-color:#fff;stop-opacity:.72" offset=".55"/>
> +      <stop id="whiteGradientStop04" style="stop-color:#fff;stop-opacity:.65" offset="1"/>

ids on those gradient stops aren't useful (although the ID on the linearGradient is)

@@ +13,3 @@
>      <linearGradient id="arrowGradientStops">
> +      <stop id="arrowGradientStop01" style="stop-color:#333;stop-opacity:.5" offset="0"/>
> +      <stop id="arrowGradientStop02" style="stop-color:#666;stop-opacity:.5" offset="1"/>

same comment about the gradient stops

@@ +25,5 @@
> +  <path mask="url(#dropShadowMask)" id="playButtonShadow" style="filter:url(#dropShadow);" d="M32,4C16.536,4,4,16.536,4,32s12.536,28,28,28s28-12.536,28-28S47.464,4,32,4z M47.285,33.014 L23.75,46.762C22.797,47.319,22,46.863,22,45.751v-27.5c0-0.697,0.32-1.137,0.781-1.232c0.277-0.058,0.612,0.012,0.969,0.221 l23.535,13.751C48.238,31.546,48.238,32.458,47.285,33.014z"/>
> +  <path id="playButtonArrow" style="fill:url(#arrowGradient);" d="M22.781,17.019C22.32,17.114,22,17.555,22,18.251v27.5c0,1.112,0.797,1.568,1.75,1.011 l23.535-13.748c0.953-0.556,0.953-1.467,0-2.023L23.75,17.24C23.393,17.031,23.058,16.961,22.781,17.019z"/>
> +  <path id="playButton" style="fill:url(#whiteGradient);" d="M32,4C16.536,4,4,16.536,4,32s12.536,28,28,28s28-12.536,28-28S47.464,4,32,4z M47.285,33.014 L23.75,46.762C22.797,47.319,22,46.863,22,45.751v-27.5c0-0.697,0.32-1.137,0.781-1.232c0.277-0.058,0.612,0.012,0.969,0.221 l23.535,13.751C48.238,31.546,48.238,32.458,47.285,33.014z"/>
> +  <path id="playButtonEdgeHighlights" style="fill:white;fill-opacity:.3;" d="M32,4C16.536,4,4,16.536,4,32s12.536,28,28,28s28-12.536,28-28S47.464,4,32,4z M32,59C17.112,59,5,46.888,5,32S17.112,5,32,5s27,12.112,27,27S46.888,59,32,59z M47.789,30.127l-23.534-13.75 C23.826,16.126,23.396,16,22.976,16c-0.135,0-0.27,0.014-0.398,0.041C21.62,16.238,21,17.106,21,18.251v27.5 C21,47.075,21.812,48,22.977,48c0.423,0,0.854-0.126,1.279-0.375L47.79,33.877c0.769-0.449,1.21-1.132,1.21-1.875 S48.559,30.576,47.789,30.127z M47.285,33.014L23.75,46.762C23.474,46.924,23.211,47,22.977,47C22.402,47,22,46.541,22,45.751v-27.5 c0-0.697,0.32-1.137,0.781-1.232L22.976,17c0.233,0,0.498,0.079,0.775,0.24l23.535,13.751 C48.238,31.546,48.238,32.458,47.285,33.014z"/>
> +  <path id="playButtonTopEdgeHighlights" style="fill:white;fill-opacity:.8;" d="M32,4C16.536,4,4,16.536,4,32c0,0.167,0.01,0.333,0.013,0.5 C4.28,17.268,16.704,5,32,5c15.296,0,27.72,12.268,27.987,27.5C59.99,32.333,60,32.167,60,32C60,16.536,47.464,4,32,4z M47.285,33.014L23.75,46.762C22.797,47.319,22,46.863,22,45.751v1c0,1.112,0.797,1.568,1.75,1.011l23.535-13.748 c0.697-0.406,0.879-1.003,0.556-1.512C47.723,32.688,47.541,32.864,47.285,33.014z"/>

The ids on the paths are not useful

Comment 14

[Tim Nguyen :ntim]

(In reply to Miles Richardson from comment #11)
> Created attachment 8606684 [details] [diff] [review]
> Patch v2
> 
> I incorporated all of the feedback from comment #9.
> 
> Tim Nguyen, apparently you have review requests disabled, so I'm just
> setting the needinfo flag. Feel free to delegate the review as you see fit.
Just needinfo me in this case :)

Comment 15

[Miles Richardson]

Attachment: Patch v3

I incorporated feedback from comment #13. I also went through all of the files again looking for anything I missed the first time I went through everything. I found a few more values that were excessively precise and I found a few <use> tags that could be replaced (comment #12).

Let me know if I've overlooked anything or if you have any other suggestions for cleanup.

Comment 16

[Robert Longson [:longsonr]]

+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 24 24">
+  <polygon opacity="0.15" fill-rule="evenodd" clip-rule="evenodd" points="16.583,0.015 16.569,0 4.583,12 16.569,24 16.583,23.985"/>
+  <polygon fill-rule="evenodd" clip-rule="evenodd" fill="#fbfbfb" points="16.575,1.021 16.561,1.008 5.583,12 16.577,23.008 16.591,22.994 "/>
 </svg>

Only path elements should have clip-rule or fill-rule properties really and no SVG element should have both a fill-rule and a clip-rule, it's one or the other. If the <path> element is a descendant of a clipPath then it's clip-rule if not, it's fill-rule. In the above case it's neither.

+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 16 16">
+	<path fill-rule="evenodd" clip-rule="evenodd" fill="#231f20" d="M9.921,8.415c0.105-0.11,0.146-0.265,0.13-0.432 c0.016-0.166-0.025-0.321-0.13-0.429L9.305,6.938l-2.6-2.65C6.402,3.973,5.973,3.906,5.748,4.139L5.238,4.68 c-0.225,0.233-0.16,0.679,0.144,0.995L6.44,6.754H0.608C0.272,6.754,0,7.026,0,7.361l0,1.215c0,0.335,0.272,0.607,0.608,0.607H6.47 l-1.136,1.155c-0.305,0.313-0.369,0.756-0.144,0.987L5.7,11.861c0.225,0.233,0.654,0.166,0.959-0.149l2.619-2.663L9.921,8.415z"/>
+  <path fill-rule="evenodd" clip-rule="evenodd" fill="#231f20" d="M14,0H5.558c-0.331,0-0.6,0.269-0.6,0.6v0.8 c0,0.331,0.269,0.6,0.6,0.6H12.5C13.328,2,14,2.672,14,3.5v9c0,0.828-0.672,1.5-1.5,1.5H5.558c-0.331,0-0.6,0.269-0.6,0.6v0.8 c0,0.331,0.269,0.6,0.6,0.6H14c1.105,0,2-0.895,2-2V2C16,0.895,15.105,0,14,0z"/>
 </svg>

nit: consistent indenting

+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 40 40">
+  <linearGradient id="gradient1" gradientUnits="userSpaceOnUse" x1="20" y1="4" x2="20" y2="36">
+    <stop offset="0" style="stop-color:#e63b2e"/>
+    <stop offset="1" style="stop-color:#c33931"/>
+  </linearGradient>
+  <path fill="url(#gradient1)" d="M13.373,4L4,13.372v13.256L13.373,36h13.255L36,26.628V13.372L26.627,4H13.373z M22.176,8.704 l-0.48,14.304h-3.424L17.76,8.704H22.176z M20,31.296c-1.44,0-2.592-1.184-2.592-2.592c0-1.44,1.152-2.592,2.592-2.592 c1.472,0,2.592,1.152,2.592,2.592C22.592,30.112,21.472,31.296,20,31.296z"/>
+  <linearGradient id="gradient2" gradientUnits="userSpaceOnUse" x1="20" y1="0" x2="20" y2="40">
+    <stop offset="0" style="stop-color:#e63b2e"/>
+    <stop offset="1" style="stop-color:#c33931"/>
+  </linearGradient>
+  <path fill="url(#gradient2)" d="M28.284,0H11.716L0,11.716v16.569L11.716,40h16.569L40,28.284V11.716L28.284,0z M38,27.456 L27.456,38H12.544L2,27.456V12.544L12.544,2h14.911L38,12.544V27.456z"/>
 </svg>

gradients should be (but don't have to be) children of <defs> elements.

+    <rect x="0px" y="3px" width="16px" height="2px" rx="1" ry="1"/>

x=0 in any units you like is the default so this attribute can be omitted.

+    <g opacity=".3">

every time you use opacity consider whether you could get away with fill-opacity or stroke-opacity or both instead of opacity. opacity is much less efficient than the fill/stroke variants. This might be best addressed as a follow up.

Comment 17

[Tim Nguyen :ntim]

(In reply to Robert Longson from comment #16)
> +<svg xmlns="http://www.w3.org/2000/svg"
> xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 24 24">
> +  <polygon opacity="0.15" fill-rule="evenodd" clip-rule="evenodd"
> points="16.583,0.015 16.569,0 4.583,12 16.569,24 16.583,23.985"/>
> +  <polygon fill-rule="evenodd" clip-rule="evenodd" fill="#fbfbfb"
> points="16.575,1.021 16.561,1.008 5.583,12 16.577,23.008 16.591,22.994 "/>
>  </svg>
> 
> Only path elements should have clip-rule or fill-rule properties really and
> no SVG element should have both a fill-rule and a clip-rule, it's one or the
> other. If the <path> element is a descendant of a clipPath then it's
> clip-rule if not, it's fill-rule. In the above case it's neither.
It'd be better to remove this in a followup, since there are a lot of occurrences of those attributes.
Let's keep this bug simple.

> +    <g opacity=".3">
> 
> every time you use opacity consider whether you could get away with
> fill-opacity or stroke-opacity or both instead of opacity. opacity is much
> less efficient than the fill/stroke variants. This might be best addressed
> as a follow up.
Yes, this is best addressed in a followup, since they are a lot of occurences too, and there are lots of nested opacity styles too (tool-network.svg).

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8606748 [details] [diff] [review]
Patch v3

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

I haven't got a chance to look at the interdiff yet (it seems slow on bugzilla), but here are some remarks meanwhile (mainly Robert's remarks).

::: browser/branding/aurora/content/about-wordmark.svg
@@ +2,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 132 62" width="132" height="62">
> +  <path fill="#fff" d="M5.3,45.2H2.7L0.1,57.3h3.2c1.8,0,3-0.4,4.2-1.5c1.4-1.2,2.6-4.1,2.6-6.2c0-1.7-0.4-2.8-1.2-3.5C8,45.4,7.1,45.2,5.3,45.2z

nit : This is tab-indented (should be 2 spaces)

::: browser/branding/aurora/content/silhouette-40.svg
@@ -21,5 @@
> -    c0,0.1,0,0.1,0,0.2c-0.3,0.4-0.5,0.7-0.6,0.9c-0.4,0.7-0.7,1.8-1,3.5c0,0,0.2-0.6,0.6-1.3l0,0c-0.3,0.9-0.5,2.3-0.4,4.4
> -    c0-0.1,0.1-0.6,0.2-1.3c0.1,1.4,0.5,3.1,1.5,5c0.8,1.4,1.7,2.4,2.7,3.2c0.2,0.2,0.4,0.3,0.6,0.5c1.3,1,3.3,2.1,5,2.4
> -    c-0.6-0.2-1-0.5-1-0.5s2,0.7,3.5,0.6c-0.5-0.1-0.6-0.3-0.6-0.3s4.2,0.2,6.4-1.5c0.5-0.4,0.8-0.8,0.9-1.2c0.6-0.4,1.3-0.8,2-1.6
> -    c1.2-1.2,1.3-2.1,1.4-3v0.1C-14,55.2-14,54.9-14.1,54.7z"/>
> -  </svg>

nit : This shouldn't be indented

::: browser/components/loop/content/shared/img/svg/glyph-signin-16x16.svg
@@ +2,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 16 16">
> +	<path fill-rule="evenodd" clip-rule="evenodd" fill="#231f20" d="M9.921,8.415c0.105-0.11,0.146-0.265,0.13-0.432 c0.016-0.166-0.025-0.321-0.13-0.429L9.305,6.938l-2.6-2.65C6.402,3.973,5.973,3.906,5.748,4.139L5.238,4.68 c-0.225,0.233-0.16,0.679,0.144,0.995L6.44,6.754H0.608C0.272,6.754,0,7.026,0,7.361l0,1.215c0,0.335,0.272,0.607,0.608,0.607H6.47 l-1.136,1.155c-0.305,0.313-0.369,0.756-0.144,0.987L5.7,11.861c0.225,0.233,0.654,0.166,0.959-0.149l2.619-2.663L9.921,8.415z"/>

nit : this should be 2 space indented

::: browser/themes/shared/devtools/images/performance-icons.svg
@@ +10,5 @@
> +      display: none;
> +    }
> +  </style>
> +  <g id="overview-markers">
> +    <rect x="0px" y="3px" width="5px" height="2.5px" rx="1" ry="1"/>

This file (and only this file) has some x=0 attributes that can be ommited, as Robert pointed out

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8606748 [details] [diff] [review]
Patch v3

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

I have just taken a look at the interdiff, looks good to me (except the previous remarks).
I'll let :dao review this patch as I am not a peer.

Also, have you built Firefox with the patch applied yet ? In case you haven't, can you just take a quick look at the context menu, the preferences and the hello panel just to make sure nothing breaks ? Nothing should break, but it's still better to be sure.

Comment 20

[Miles Richardson]

Attachment: Patch v4

I incorporated Robert's feedback from comment #16 and Tim's feedback from comment #18. I actually already started incorporating Robert's feedback before I saw Tim's suggestion to leave the clip-rule/fill-rule for a followup in comment #17 so the clip-rule/fill-rule issue has already been taken care of I believe. I didn't bother messing with the opacity attributes yet so that will still make a good followup (which I'd be happy to work on). I also removed xmlns:xlink attributes from <svg> elements when there were no xlink's in the file.

Regarding building Firefox with this patch, I just did so and had a look around and all of the icons look correct to me (though keep in mind I've only just started contributing and could have easily missed something).

If there's anything else that needs done with this bug please let me know.

Comment 21

[Miles Richardson]

Now that Dão has approved the patch (attachment 8606793 [details] [diff] [review]) what's the next step? Do I need to get this patch tested on the try server (I do not have try server access), or can I mark this patch as ready for check-in?

Comment 22

[Tim Nguyen :ntim]

(In reply to Miles Richardson from comment #21)
> Now that Dão has approved the patch (attachment 8606793 [details] [diff] [review]
> [review]) what's the next step? Do I need to get this patch tested on the
> try server (I do not have try server access), or can I mark this patch as
> ready for check-in?

It's very likely you'll need to rebase to the patch (fix patch conflicts) before checkin. So, please do that first using those commands : hg qpop -a && hg pull -u && hg qpush patchname --move . Once you've done those commands, you'll see the patch conflicts you need to fix. Once you've fixed the conflicts, you can do hg qref and then re-upload the patch here.

Once you've re-uploaded the patch, you can set the checkin-needed flag, and then the sheriffs will check this in for you. (You don't need to push to try in this case as this is a patch that is unlikely going to affect tests).

Comment 23

[Miles Richardson]

Attachment: Patch v4.1

Rebased patch v4 on current tip (cb33535b93f2) and resolved all conflicts.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2d38fecce226

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2d38fecce226

Comment 26

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

Removing the <g> from WebIDE's throbber.svg altered the image (see bug 1169109).  Not sure if other files were affected.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This was backed out in bug 1169109.
https://hg.mozilla.org/mozilla-central/rev/9e0263028c04

Comment 28

[Dão Gottwald [::dao]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> This was backed out in bug 1169109.
> https://hg.mozilla.org/mozilla-central/rev/9e0263028c04

No, it wasn't, but maybe it should because it's hard to tell if there were other mistakes like the one in throbber.svg.