1297806 - 15% linux cart regressions from bug 1022573

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P1)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

from bug 1295502, we have two patches in the same push and each patch has a unique set of regressions- the patch in bug 1295502 will fix everything but the linux cart regressions- lets fix that here.

for the backstory, you can see backing out the patch yields the win:
https://bugzilla.mozilla.org/show_bug.cgi?id=1295502#c10

Comment 1

[Dão Gottwald [:dao]]

To fix this I think we need to avoid the fill filter and replace menuPanel.png with an SVG instead. Stephen, can you provide the SVG or delegate to someone to do so?

Comment 2

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #1)
> To fix this I think we need to avoid the fill filter and replace
> menuPanel.png with an SVG instead. Stephen, can you provide the SVG or
> delegate to someone to do so?

We need menuPanel.png in the current sprite layout but as an SVG instead?

Comment 3

[Dão Gottwald [:dao]]

A sprite would be easiest to replace the PNGs with, but I think long term we'll want to get rid of the coordinates and use ids instead. We might as well make that change here.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #3)
> A sprite would be easiest to replace the PNGs with, but I think long term
> we'll want to get rid of the coordinates and use ids instead. We might as
> well make that change here.

Though considering that these might have different performance characteristics (i.e. I think ids might be slower due to Gecko bugs), maybe we should start with the sprite for the purpose of this bug :/

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:dao what are the next steps here?  We will be merging to Aurora next week.

Comment 6

[Dão Gottwald [:dao]]

Stephen is still preparing the SVGs.

Comment 7

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #6)
> Stephen is still preparing the SVGs.

I will get that to you today.

Comment 8

[Stephen Horlander [:shorlander]]

Attachment: menuPanel.svg

Updated menuPanel.png sprite to menuPanel.svg sprite. Should be mostly the same, I had to make a few tweaks and updated the webIDE icon (not even sure if we use that anymore).

Comment 9

[Dão Gottwald [:dao]]

Stephen, could I please get the SVG version of menuPanel-small.png too? Thanks!

Comment 10

[Dão Gottwald [:dao]]

prototype patch appears to undo the regression:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=dfa90003a104&newProject=try&newRevision=f5e2ce922bdf&framework=1

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

great stuff!

Comment 12

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #9)
> Stephen, could I please get the SVG version of menuPanel-small.png too?
> Thanks!

Yes. I will do that today.

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: menuPanel-small.svg

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch

Thanks Stephen!

Comment 15

[Dão Gottwald [:dao]]

Attachment: hg diff -w browser/themes/shared/menupanel.inc.css

for easier review

Comment 16

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

Comment on attachment 8789900 [details] [diff] [review]
hg diff -w browser/themes/shared/menupanel.inc.css

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

::: browser/themes/shared/menupanel.inc.css
@@ +147,5 @@
>    #edit-controls@inAnyPanel@ > toolbarbutton,
>    #zoom-controls@inAnyPanel@ > toolbarbutton,
>    toolbarpaletteitem[place="palette"] > #edit-controls > toolbarbutton,
>    toolbarpaletteitem[place="palette"] > #zoom-controls > toolbarbutton {
> +  list-style-image: url(chrome://browser/skin/menuPanel-small.svg);

something is funky with the indentation changes in this patch. it looks like all lines added should have two extra spaces in front of them.

Comment 17

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

disregard that, it's because of the diff -w

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for the work here!  As a note, we are merging Firefox 51 to Aurora this week- which means that we should consider uplifting this patch to Aurora once we land it on central.

Comment 19

[Dão Gottwald [:dao]]

I intend to land this before 51 moves to aurora.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8789899 [details] [diff] [review]
patch

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

Looks good - thanks Dao!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe381245f03
Replace menuPanel.png / menuPanel-small.png and the fill filter with menuPanel.svg / menuPanel-small.svg to fix customize mode animation regression on Linux. r=mconley

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

and we see improvements:
https://treeherder.mozilla.org/perf.html#/alerts?id=3116

Comment 24

[William Lachance (:wlach)]

Incidentally this gives us a small installer size win:

== Change summary for alert #3115 (as of September 13 2016 10:21 UTC) ==

Improvements:

  0%  installer size summary windowsxp opt         53574766 -> 53373114.17
  0%  installer size summary windows8-64 opt       57569298.83 -> 57370336.25
  0%  installer size summary windowsxp debug       66902027.83 -> 66702231
  0%  installer size summary windows8-64 debug     79852025.08 -> 79651911.5

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=3115