1397907 - Optimize some of the SVG animations a little more

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

We can trim down some of the SVG animation file sizes by being a little more aggressive with some of the optimizations. I can't see any noticeable visual difference between the before/after of these.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1397907 - Optimize some of the SVG animations a little more.

Review commit: https://reviewboard.mozilla.org/r/177460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177460/

Comment 2

[Robert Longson [:longsonr]]

Comment on attachment 8905671 [details]
Bug 1397907 - Optimize some of the SVG animations a little more.

https://reviewboard.mozilla.org/r/177460/#review182668

most of the comments apply throughout the file. They are all nits and can be ignore if the fix cost outweighs the benefit of fixing them, if they are an artifact of the tool used to create it then perhaps the tool can be improved somehow.

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:5
(Diff revision 1)
>  <!-- 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" width="1078" height="54">
>    <svg x="0">

x="0" is the default so it can be removed.

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:6
(Diff revision 1)
>  <!-- 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" width="1078" height="54">
>    <svg x="0">
> -    <path fill="context-fill" fill-opacity="1" d="M8 22a1 1 0 0 0 -1 1v10a1 1 0 0 0 2 0v-10a1 1 0 0 0 -1 -1zm3 -1a1 1 0 0 0 -1 1v11a1 1 0 0 0 2 0v-11a1 1 0 0 0 -1 -1zm7.939 11.658l-4 -11a1 1 0 1 0 -1.879 0.684l4 11a1 1 0 1 0 1.892 -0.648l-0.013 -0.036zm-13.939 -12.658a1 1 0 0 0 -1 1v12a1 1 0 0 0 2 0v-12a1 1 0 0 0 -1 -1z" opacity="1"/>
> +    <path fill="context-fill" fill-opacity="1" d="M8 22a1 1 0 0 0-1 1v10a1 1 0 0 0 2 0V23a1 1 0 0 0-1-1zm3-1a1 1 0 0 0-1 1v11a1 1 0 0 0 2 0V22a1 1 0 0 0-1-1zm8 11.7l-4-11a1 1 0 1 0-2 .6l4 11a1 1 0 1 0 2-.6zM5 20a1 1 0 0 0-1 1v12a1 1 0 0 0 2 0V21a1 1 0 0 0-1-1z" opacity="1"/>

can't fill-opacity="1" and opacity="1" be removed, t they are default values. They seem to be repeated throughout the file.

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:16
(Diff revision 1)
> +        <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.7.4L6-8.5l8.5-1.7 2.8-18.2 6-25.2-.8-22.3-51.6-1z" opacity="1" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +      </mask>
> +      <mask id="a" mask-type="alpha">
>          <g opacity=".055">
>            <g opacity="1">
> -            <path fill="#11DBEA" fill-opacity="1" d="M-29.125 -76.875l-15.5 48.625 21.375 12 5.5 -0.75 3 -0.25 2.25 3.75 5.5 1 0.75 -2.25 3.25 -0.125 6.229 0.505 4.259 11.371 7.012 -7.251 2.75 -18.125 6 -25.25 -0.75 -22.25 -51.625 -1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +            <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.2.4L7.5-3l7-7.2 2.8-18.2 6-25.2-.8-22.3-51.6-1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>

why have nested <g> elements, the opacity will be .055 here.

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:17
(Diff revision 1)
> +      </mask>
> +      <mask id="a" mask-type="alpha">
>          <g opacity=".055">
>            <g opacity="1">
> -            <path fill="#11DBEA" fill-opacity="1" d="M-29.125 -76.875l-15.5 48.625 21.375 12 5.5 -0.75 3 -0.25 2.25 3.75 5.5 1 0.75 -2.25 3.25 -0.125 6.229 0.505 4.259 11.371 7.012 -7.251 2.75 -18.125 6 -25.25 -0.75 -22.25 -51.625 -1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> -            <path fill-opacity="0" stroke="#871111" stroke-linecap="butt" stroke-linejoin="miter" stroke-miterlimit="4" stroke-opacity="1" stroke-width="0" d="M-1.063 -10.563l-7.75 24.313 10.687 6 2.75 -0.375 1.5 -0.125 1.125 1.875 2.75 0.5 0.375 -1.125 1.625 -0.063 3.115 0.253 2.129 5.685 3.506 -3.625 1.375 -9.062 3 -12.625 -0.375 -11.125 -25.812 -0.5z"/>
> +            <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.2.4L7.5-3l7-7.2 2.8-18.2 6-25.2-.8-22.3-51.6-1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +            <path fill-opacity="0" stroke="#871111" stroke-linecap="butt" stroke-linejoin="miter" stroke-miterlimit="4" stroke-opacity="1" stroke-width="0" d="M-1-10.6l-7.8 24.3 10.7 6 2.7-.3 1.5-.1L7.2 21l2.8.5.4-1.1H12l3.1.2 2.1 5.7 3.5-3.6 1.4-9.1 3-12.6-.4-11.2-25.8-.5z"/>

This path is invisible and can be removed, no? It has fill-opacity="0" and stroke-width="0"

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:227
(Diff revision 1)
>        <mask id="v" mask-type="alpha">
> -        <path fill="#11DBEA" fill-opacity="1" d="M-29.125 -76.875l-15.5 48.625 21.375 12 5.5 -0.75 3 -0.25 2.25 3.75 5.5 1 0.75 -2.25 3.25 -0.125 6.658 0.5 2.375 5.876 8.467 -1.751 2.75 -18.125 6 -25.25 -0.75 -22.25 -51.625 -1z" opacity="1" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +        <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.7.4L6-8.5l8.5-1.7 2.8-18.2 6-25.2-.8-22.3-51.6-1z" opacity="1" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
>        </mask>
>        <mask id="u" mask-type="alpha">
>          <g opacity="1">
> -          <path fill="#11DBEA" fill-opacity="1" d="M-29.125 -76.875l-15.5 48.625 21.375 12 5.5 -0.75 3 -0.25 2.25 3.75 5.5 1 0.75 -2.25 3.25 -0.125 6.229 0.505 4.259 11.371 7.012 -7.251 2.75 -18.125 6 -25.25 -0.75 -22.25 -51.625 -1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +          <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.2.4L7.5-3l7-7.2 2.8-18.2 6-25.2-.8-22.3-51.6-1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>

pointless <g> element

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:267
(Diff revision 1)
>        <mask id="z" mask-type="alpha">
> -        <path fill="#11DBEA" fill-opacity="1" d="M-29.125 -76.875l-15.5 48.625 21.375 12 5.5 -0.75 3 -0.25 2.25 3.75 5.5 1 0.75 -2.25 3.25 -0.125 6.658 0.5 2.375 5.876 8.467 -1.751 2.75 -18.125 6 -25.25 -0.75 -22.25 -51.625 -1z" opacity="1" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +        <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.7.4L6-8.5l8.5-1.7 2.8-18.2 6-25.2-.8-22.3-51.6-1z" opacity="1" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
>        </mask>
>        <mask id="y" mask-type="alpha">
>          <g opacity="1">
> -          <path fill="#11DBEA" fill-opacity="1" d="M-29.125 -76.875l-15.5 48.625 21.375 12 5.5 -0.75 3 -0.25 2.25 3.75 5.5 1 0.75 -2.25 3.25 -0.125 6.229 0.505 4.259 11.371 7.012 -7.251 2.75 -18.125 6 -25.25 -0.75 -22.25 -51.625 -1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +          <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.2.4L7.5-3l7-7.2 2.8-18.2 6-25.2-.8-22.3-51.6-1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>

pointless <g> element.

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:347
(Diff revision 1)
>        <mask id="H" mask-type="alpha">
> -        <path fill="#11DBEA" fill-opacity="1" d="M-29.125 -76.875l-15.5 48.625 21.375 12 5.5 -0.75 3 -0.25 2.25 3.75 5.5 1 0.75 -2.25 3.25 -0.125 6.658 0.5 2.375 5.876 8.467 -1.751 2.75 -18.125 6 -25.25 -0.75 -22.25 -51.625 -1z" opacity="1" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +        <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.7.4L6-8.5l8.5-1.7 2.8-18.2 6-25.2-.8-22.3-51.6-1z" opacity="1" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
>        </mask>
>        <mask id="G" mask-type="alpha">
>          <g opacity="1">
> -          <path fill="#11DBEA" fill-opacity="1" d="M-29.125 -76.875l-15.5 48.625 21.375 12 5.5 -0.75 3 -0.25 2.25 3.75 5.5 1 0.75 -2.25 3.25 -0.125 6.229 0.505 4.259 11.371 7.012 -7.251 2.75 -18.125 6 -25.25 -0.75 -22.25 -51.625 -1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +          <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.2.4L7.5-3l7-7.2 2.8-18.2 6-25.2-.8-22.3-51.6-1z" transform="matrix(.5 0 0 .5 13.499 27.875)"/>

pointless <g> element.

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:385
(Diff revision 1)
>    <svg x="418">
>      <defs>
>        <mask id="L" mask-type="alpha">
> -        <path fill="#11DBEA" fill-opacity="1" d="M-29.125 -76.875l-15.5 48.625 21.375 12 5.5 -0.75 3 -0.25 2.25 3.75 5.5 1 0.75 -2.25 3.25 -0.125 6.658 0.5 2.375 5.876 8.467 -1.751 2.75 -18.125 6 -25.25 -0.75 -22.25 -51.625 -1z" opacity="1" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
> +        <path fill="#11DBEA" fill-opacity="1" d="M-29.1-76.9l-15.5 48.7 21.4 12 5.4-.8 3-.3 2.3 3.8 5.5 1 .8-2.3H-3l6.7.4L6-8.5l8.5-1.7 2.8-18.2 6-25.2-.8-22.3-51.6-1z" opacity="1" transform="matrix(.5 0 0 .5 13.499 27.875)"/>
>        </mask>
>        <mask id="K" mask-type="alpha">

is every mask an alpha mask? If so set it on the <svg> element or as a style and it can be removed from every other place it occurs.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8905671 [details]
Bug 1397907 - Optimize some of the SVG animations a little more.

https://reviewboard.mozilla.org/r/177460/#review182516

I had started noting the same nits that Robert has. I'm not sure why the collapseGroups plugin didn't get rid of the redundant <g> elements - seems like it should have. Don't need to fix the tooling in this bug though. Removing possibly-pointless masks though seems worth a shot though to see if there's any visible difference without them.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8905671 [details]
Bug 1397907 - Optimize some of the SVG animations a little more.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177460/diff/1-2/

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: slimmer library-bookmark-animation.svg

Took a pass through this to eliminate some duplication and some unused masks

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: slimmer library-pocket-animation.svg

Took a pass through this to eliminate some duplication and unused masks

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8905671 [details]
Bug 1397907 - Optimize some of the SVG animations a little more.

https://reviewboard.mozilla.org/r/177460/#review185316

Check out the 2 files I attached to the bug. Probably this would have been a good-first-bug? Its easy to verify and update so this looks good to me with or without those changes.

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:4
(Diff revision 2)
>  <!-- 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" width="1078" height="54">

I attached some further trimming of this to the bug.

::: browser/themes/shared/icons/library-bookmark-animation.svg:4
(Diff revision 2)
>  <!-- 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" width="1078" height="54" fill="context-fill">

Also had a pass through this - result attached to the bug.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8908502 [details]
slimmer library-bookmark-animation.svg

Why does this use fill="cyan" ?

Comment 9

[Sam Foster [:sfoster] (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Comment on attachment 8908502 [details]
> slimmer library-bookmark-animation.svg
> 
> Why does this use fill="cyan" ?

cyan is the new black! Actually I just temporarily replaced context-stroke(? or was it fill) with cyan to preview the work. Sorry about that!

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Okay, that's what I thought happened :)

I'll fix it and land this.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8905671 [details]
Bug 1397907 - Optimize some of the SVG animations a little more.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177460/diff/2-3/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

This patch takes the combined SVGs from 237kb to 93kb, a savings of 60%.

Comment 13

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f9294b6e7ad3 -d 8699ae18ef13: rebasing 421361:f9294b6e7ad3 "Bug 1397907 - Optimize some of the SVG animations a little more. r=sfoster" (tip)
merging browser/themes/shared/icons/chevron-animation.svg
merging browser/themes/shared/icons/library-bookmark-animation.svg
warning: conflicts while merging browser/themes/shared/icons/library-bookmark-animation.svg! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8905671 [details]
Bug 1397907 - Optimize some of the SVG animations a little more.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177460/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edc888ae6c3e
Optimize some of the SVG animations a little more. r=sfoster

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/edc888ae6c3e