MD Settings: Improve rendering performance of fonts page.

MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large <settings-dropdown-menu> instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <button>. This change alone reduces
    total rendering time from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896, 608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9a07dc6b71462795d776490acb1e51142c9afd74
Cr-Commit-Position: refs/heads/master@{#402981}

[dpapad, 2016-06-29 01:41:39]

Description was changed from

==========
MD Settings: Improve rendering performance of fonts page.

BUG=602896,608535
==========

to

==========
MD Settings: Improve rendering performance of fonts page.

BUG=602896,608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-06-29 01:44:44]

Description was changed from

==========
MD Settings: Improve rendering performance of fonts page.

BUG=602896,608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large settings-dropdown-menu instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This alone reduces 1.3s
    to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-06-29 01:48:24]

Description was changed from

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large settings-dropdown-menu instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This alone reduces 1.3s
    to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large settings-dropdown-menu instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This change alone reduces
    from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-06-29 01:58:38]

Description was changed from

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large settings-dropdown-menu instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This change alone reduces
    from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large settings-dropdown-menu instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This change alone reduces
    from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-06-29 02:02:35]

dpapad@chromium.org changed reviewers:
+ michaelpg@chromium.org

[dpapad, 2016-06-29 02:02:57]

Description was changed from

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large settings-dropdown-menu instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This change alone reduces
    from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large <settings-dropdown-menu> instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This change alone reduces
    from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-06-29 02:06:35]

Description was changed from

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large <settings-dropdown-menu> instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This change alone reduces
    from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large <settings-dropdown-menu> instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This change alone reduces
    total rendering time from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-06-29 15:12:16]

https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:42:
<template is="dom-repeat" items="[[menuOptions]]" item-count="5">
why not 1?

https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:43: <div
class="item" data-value$="[[item.value]]">[[item.name]]</div>
Can you try with a <button class="paper-item" role="option"> rather than a div,
to retain the styling and a11y of a paper-item for free?

You would need to pull in https://github.com/PolymerElements/paper-item/pull/86
first. The goal is to land that PR, roll it up into Chrome and then convert
*all* of our paper-items to native buttons for a modest performance gain.

<div> may well save us even more than <button> but I haven't gotten numbers on
that (and it's not the direction Polymer went).

[dpapad, 2016-06-29 17:16:26]

https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:42:
<template is="dom-repeat" items="[[menuOptions]]" item-count="5">
On 2016/06/29 at 15:12:15, michaelpg wrote:
> why not 1?

1 seems a bit extreme in the sense that it would trigger more yielding than
necessary. If we assume that we want to yield at least within 16ms, then
rendering a single div when we get the execution control means that we yield
even though there was plenty of time to render a few more divs within our time
window. (I am assuming that rendering a single <div> takes much less than 16ms).

Regardless of theory above, in practice I tried both 1 and 5 and could not
notice a difference. I am fine with 1 if you think it is not overly
conservative.

https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:43: <div
class="item" data-value$="[[item.value]]">[[item.name]]</div>
On 2016/06/29 at 15:12:15, michaelpg wrote:
> Can you try with a <button class="paper-item" role="option"> rather than a
div, to retain the styling and a11y of a paper-item for free?
> 
> You would need to pull in
https://github.com/PolymerElements/paper-item/pull/86 first. The goal is to land
that PR, roll it up into Chrome and then convert *all* of our paper-items to
native buttons for a modest performance gain.
> 
> <div> may well save us even more than <button> but I haven't gotten numbers on
that (and it's not the direction Polymer went).

Tried your suggestion. I am seeing two problems with it
1) When I click on any menu option, nothing happens (the option is not selected
and the menu does not close).
2) When the menu is collapsed, no value is being displayed.

I did not go further into comparing <div> VS <button> performance.

[michaelpg, 2016-06-29 18:18:37]

https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:43: <div
class="item" data-value$="[[item.value]]">[[item.name]]</div>
On 2016/06/29 17:16:26, dpapad wrote:
> On 2016/06/29 at 15:12:15, michaelpg wrote:
> > Can you try with a <button class="paper-item" role="option"> rather than a
> div, to retain the styling and a11y of a paper-item for free?
> > 
> > You would need to pull in
> https://github.com/PolymerElements/paper-item/pull/86 first. The goal is to
land
> that PR, roll it up into Chrome and then convert *all* of our paper-items to
> native buttons for a modest performance gain.
> > 
> > <div> may well save us even more than <button> but I haven't gotten numbers
on
> that (and it's not the direction Polymer went).
> 
> Tried your suggestion. I am seeing two problems with it
> 1) When I click on any menu option, nothing happens (the option is not
selected
> and the menu does not close).
> 2) When the menu is collapsed, no value is being displayed.
> 
> I did not go further into comparing <div> VS <button> performance.

Could you upload what you have? It works fine for me, even replicating the
attr-for-selected and item-count stuff:

https://jsfiddle.net/yecdn72d/2/

If there's a bug in paper-item-shared-styles blocking us from using it we should
open an issue on PolymerElements/paper-item.

[dpapad, 2016-06-29 18:28:38]

On 2016/06/29 at 18:18:37, michaelpg wrote:
>
https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):
> 
>
https://codereview.chromium.org/2106723003/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/controls/settings_dropdown_menu.html:43:
<div class="item" data-value$="[[item.value]]">[[item.name]]</div>
> On 2016/06/29 17:16:26, dpapad wrote:
> > On 2016/06/29 at 15:12:15, michaelpg wrote:
> > > Can you try with a <button class="paper-item" role="option"> rather than a
> > div, to retain the styling and a11y of a paper-item for free?
> > > 
> > > You would need to pull in
> > https://github.com/PolymerElements/paper-item/pull/86 first. The goal is to
land
> > that PR, roll it up into Chrome and then convert *all* of our paper-items to
> > native buttons for a modest performance gain.
> > > 
> > > <div> may well save us even more than <button> but I haven't gotten
numbers on
> > that (and it's not the direction Polymer went).
> > 
> > Tried your suggestion. I am seeing two problems with it
> > 1) When I click on any menu option, nothing happens (the option is not
selected
> > and the menu does not close).
> > 2) When the menu is collapsed, no value is being displayed.
> > 
> > I did not go further into comparing <div> VS <button> performance.
> 
> Could you upload what you have? It works fine for me, even replicating the
attr-for-selected and item-count stuff:
> 
> https://jsfiddle.net/yecdn72d/2/
> 
> If there's a bug in paper-item-shared-styles blocking us from using it we
should open an issue on PolymerElements/paper-item.

I had to redo this since I had deleted my trial branch, and it works now, so
obviously I had done something wrong, here is the working
diff (based on this CL), http://pastebin.com/C59EJTgQ. Now I can compare
<button> and <div> performance, will do shortly.

[dpapad, 2016-06-29 19:18:33]

Patchset #4 (id:60001) has been deleted

[dpapad, 2016-06-29 19:20:20]

Per discussion, updated this CL to use the uncommitted
https://github.com/PolymerElements/paper-item/pull/86. PTAL

[michaelpg, 2016-06-29 19:49:14]

thanks for updating!

update the CL description for <button>

https://codereview.chromium.org/2106723003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2106723003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:4: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-item/paper-item-shared-styles.html">
alphabetize

https://codereview.chromium.org/2106723003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:35:
<button class="paper-item"
the docs suggest including role="option". i assume that's an a11y thing but
haven't looked into it... is there a reason you dropped it?

https://codereview.chromium.org/2106723003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:36:
data-value$="[[item.value]]">[[item.name]]</button>
4-space indent
nit: line break after > and before </button> like lines 38-41

https://codereview.chromium.org/2106723003/diff/80001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/paper-item/paper-item-shared-styles.html
(right):

https://codereview.chromium.org/2106723003/diff/80001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/paper-item/paper-item-shared-styles.html:58:
:host(paper-item:focus):before, :host(paper-icon-item:focus):before,
.paper-item:focus:before {
let's re-vulcanize downloads so it doesn't get out of sync

[dpapad, 2016-06-29 20:50:08]

https://codereview.chromium.org/2106723003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2106723003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:4: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-item/paper-item-shared-styles.html">
On 2016/06/29 at 19:49:14, michaelpg wrote:
> alphabetize

Done.

https://codereview.chromium.org/2106723003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:35:
<button class="paper-item"
On 2016/06/29 at 19:49:14, michaelpg wrote:
> the docs suggest including role="option". i assume that's an a11y thing but
haven't looked into it... is there a reason you dropped it?

Done, just forgot about it.

https://codereview.chromium.org/2106723003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:36:
data-value$="[[item.value]]">[[item.name]]</button>
On 2016/06/29 at 19:49:14, michaelpg wrote:
> 4-space indent
> nit: line break after > and before </button> like lines 38-41

Fixed.

https://codereview.chromium.org/2106723003/diff/80001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/paper-item/paper-item-shared-styles.html
(right):

https://codereview.chromium.org/2106723003/diff/80001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/paper-item/paper-item-shared-styles.html:58:
:host(paper-item:focus):before, :host(paper-icon-item:focus):before,
.paper-item:focus:before {
On 2016/06/29 at 19:49:14, michaelpg wrote:
> let's re-vulcanize downloads so it doesn't get out of sync

Done.

[dpapad, 2016-06-29 20:51:44]

Description was changed from

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large <settings-dropdown-menu> instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <div>. This change alone reduces
    total rendering time from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large <settings-dropdown-menu> instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <button>. This change alone
reduces
    total rendering time from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-06-29 20:53:17]

lgtm

[dpapad, 2016-06-29 21:03:06]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-06-29 21:03:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 23:09:58]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-29 23:10:23]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 23:11:27]

Description was changed from

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large <settings-dropdown-menu> instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <button>. This change alone
reduces
    total rendering time from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Improve rendering performance of fonts page.

The fonts page includes a few very large <settings-dropdown-menu> instances.
While implementing force-rendering for the purposes of "search within settings"
it was revealed that it occupied about 1.3s of the main thread to render itself.
There are two separate improvements bundled in this CL.
 1) Replace <paper-item> with a simple styled <button>. This change alone
reduces
    total rendering time from 1.3s to 0.25s.
 2) Use the new itemCount feature for dom-repeat to yield, instead of rendering
    the entire template all at once.

BUG=602896,608535
TEST=Click on "Customize fonts", observe how much faster the subpage opens.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9a07dc6b71462795d776490acb1e51142c9afd74
Cr-Commit-Position: refs/heads/master@{#402981}
==========

[commit-bot: I haz the power, 2016-06-29 23:11:28]

Patchset 5 (id:??) landed as
https://crrev.com/9a07dc6b71462795d776490acb1e51142c9afd74
Cr-Commit-Position: refs/heads/master@{#402981}