Improve updateOriginsEliding_ performance.

Improve updateOriginsEliding_ performance.

This CL is part of the effort to improve performance of the password manager
settings. In particular, this CL fixes layout thrashing in the
updateOriginsEliding_ code. Prior to this change the code was first reading
style information from the DOM and then changing it in a loop.

This change creates a canvas element that is not part of the DOM.
It then uses the canvas to do the text measurements, thus avoiding the
repainting of the DOM. In the end, it writes the final obtained string in the
DOM, and thus does not change the display of the saved passwords.

Reference: https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/measureText

BUG=651049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/68f4faa15a37a66d07d374cd7df3dfd296ff957f
Cr-Commit-Position: refs/heads/master@{#427028}

[jdoerrie, 2016-10-21 12:30:56]

Description was changed from

==========
Improve updateOriginsEliding_ performance.

This CL is part of the effort to improve performance of the password manager
settings. In particular, this CL fixes layout thrashing in the
updateOriginsEliding_ code. Prior to this change the code was first reading
style information from the DOM and then changing it in a loop.

This change creates a canvas element that is not part of the DOM.
It then uses the canvas to do the text measurements, thus avoiding the
repainting of the DOM. In the end, it writes the final obtained string in the
DOM, and thus does not change the display of the saved passwords.

Reference:
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/mea...

BUG=651049
==========

to

==========
Improve updateOriginsEliding_ performance.

This CL is part of the effort to improve performance of the password manager
settings. In particular, this CL fixes layout thrashing in the
updateOriginsEliding_ code. Prior to this change the code was first reading
style information from the DOM and then changing it in a loop.

This change creates a canvas element that is not part of the DOM.
It then uses the canvas to do the text measurements, thus avoiding the
repainting of the DOM. In the end, it writes the final obtained string in the
DOM, and thus does not change the display of the saved passwords.

Reference:
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/mea...

BUG=651049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[jdoerrie, 2016-10-21 12:32:14]

jdoerrie@chromium.org changed reviewers:
+ stevenjb@chromium.org

[vabr (Chromium), 2016-10-21 13:15:31]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2016-10-21 13:15:31]

Drive-by optional comment below.
(The change LGTM, but I am no OWNER here.)

Thanks!
Vaclav

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:219: textContent = '…' +
textContent.substring(2);
optional, because not introduced by this CL:

It seems strange that on line 217 just one character is cut away, and here in
the loop it is 2 per iteration. Do we need them different? Why do we use the
value(s) we use? (Small number => maximizing of the original text being left,
big number => quicker processing, what is the right tradeoff?)

Feel free to tweak this according to what you think is best and add a comment
explaining your choice (unless you use 1 in both places, which is default enough
to need an explanation).

[jdoerrie, 2016-10-21 14:15:00]

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:219: textContent = '…' +
textContent.substring(2);
On 2016/10/21 13:15:31, vabr (Chromium) wrote:
> optional, because not introduced by this CL:
> 
> It seems strange that on line 217 just one character is cut away, and here in
> the loop it is 2 per iteration. Do we need them different? Why do we use the
> value(s) we use? (Small number => maximizing of the original text being left,
> big number => quicker processing, what is the right tradeoff?)
> 
> Feel free to tweak this according to what you think is best and add a comment
> explaining your choice (unless you use 1 in both places, which is default
enough
> to need an explanation).

We need different numbers because textContent[0] will always be '…' after line
217. If we used substring(1) in line 219 the whole operation would simply be
identity. With the current implementation we always take one character away per
iteration. I thought about optimizing this, but with variable length characters
it would be hard to know how many characters exactly need to be chopped off in
order to make the text fit. If this turns out to be a perf issue (which
currently I don't think it is), one could implement a form of binary search
here.

[vabr (Chromium), 2016-10-21 14:20:45]

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:219: textContent = '…' +
textContent.substring(2);
On 2016/10/21 14:15:00, jdoerrie wrote:
> On 2016/10/21 13:15:31, vabr (Chromium) wrote:
> > optional, because not introduced by this CL:
> > 
> > It seems strange that on line 217 just one character is cut away, and here
in
> > the loop it is 2 per iteration. Do we need them different? Why do we use the
> > value(s) we use? (Small number => maximizing of the original text being
left,
> > big number => quicker processing, what is the right tradeoff?)
> > 
> > Feel free to tweak this according to what you think is best and add a
comment
> > explaining your choice (unless you use 1 in both places, which is default
> enough
> > to need an explanation).
> 
> We need different numbers because textContent[0] will always be '…' after line
> 217. If we used substring(1) in line 219 the whole operation would simply be
> identity. With the current implementation we always take one character away
per
> iteration. I thought about optimizing this, but with variable length
characters
> it would be hard to know how many characters exactly need to be chopped off in
> order to make the text fit. If this turns out to be a perf issue (which
> currently I don't think it is), one could implement a form of binary search
> here.

Thanks for the explanation, Jan, and sorry for my dumb question, I should have
thought more about it. I also agree with you that the current way of finding the
maximal length does not seem to be the performance issue of this code.

[stevenjb, 2016-10-21 19:12:44]

stevenjb@chromium.org changed reviewers:
+ hcarmona@chromium.org

[stevenjb, 2016-10-21 19:12:44]

We're in the process of switching to chrome://md-settings, so I'm not sure we
want to introduce this change at this point (and I'm not sure whether it will be
relevant in the MD Settings UI).

+hcarmona@ who is working on this section.

[vabr (Chromium), 2016-10-21 19:23:12]

On 2016/10/21 19:12:44, stevenjb wrote:
> We're in the process of switching to chrome://md-settings, so I'm not sure we
> want to introduce this change at this point (and I'm not sure whether it will
be
> relevant in the MD Settings UI).
> 
> +hcarmona@ who is working on this section.

Please note that the current state is causing notable issues to the users.
Unless we are launching MD settings in the current stable, there seems to be
strong benefits to landing this change.

Cheers,
Vaclav

[hcarmona, 2016-10-21 19:44:41]

On 2016/10/21 19:12:44, stevenjb wrote:
> We're in the process of switching to chrome://md-settings, so I'm not sure we
> want to introduce this change at this point (and I'm not sure whether it will
be
> relevant in the MD Settings UI).
> 
> +hcarmona@ who is working on this section.

http://crbug.com/620007 is about bringing this functionality to the
MD-Settings version of passwords and it mentions that this code is slow
in comment #3.

I think making this code faster is good overall and changes here might
be helpful in the md-settings version of passwords. I can answer
questions about the new UI if you want to bring this code there.

[vabr (Chromium), 2016-10-21 19:56:43]

On 2016/10/21 19:44:41, Hector Carmona wrote:
> On 2016/10/21 19:12:44, stevenjb wrote:
> > We're in the process of switching to chrome://md-settings, so I'm not sure
we
> > want to introduce this change at this point (and I'm not sure whether it
will
> be
> > relevant in the MD Settings UI).
> > 
> > +hcarmona@ who is working on this section.
> 
> http://crbug.com/620007 is about bringing this functionality to the
> MD-Settings version of passwords and it mentions that this code is slow
> in comment #3.
> 
> I think making this code faster is good overall and changes here might
> be helpful in the md-settings version of passwords. I can answer
> questions about the new UI if you want to bring this code there.

Thanks, Hector, for the bug link and giving the context about the MD settings.

I would suggest for Jan, the author of this CL, to have a look at bringing this
solution to the MD settings (and consulting you as needed), but perhaps in a
separate CL.

Meanwhile, could we please get an OWNERS review to land this?

Thanks!
Vaclav

[stevenjb, 2016-10-21 19:58:40]

OK, thanks Hector. Owner lgtm.

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:215: }
nit: {} not needed

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:220: }
nit: {} also not needed here.

[hcarmona, 2016-10-21 20:48:42]

On 2016/10/21 19:56:43, vabr (Chromium) wrote:
> On 2016/10/21 19:44:41, Hector Carmona wrote:
> > On 2016/10/21 19:12:44, stevenjb wrote:
> > > We're in the process of switching to chrome://md-settings, so I'm not sure
> we
> > > want to introduce this change at this point (and I'm not sure whether it
> will
> > be
> > > relevant in the MD Settings UI).
> > > 
> > > +hcarmona@ who is working on this section.
> > 
> > http://crbug.com/620007 is about bringing this functionality to the
> > MD-Settings version of passwords and it mentions that this code is slow
> > in comment #3.
> > 
> > I think making this code faster is good overall and changes here might
> > be helpful in the md-settings version of passwords. I can answer
> > questions about the new UI if you want to bring this code there.
> 
> Thanks, Hector, for the bug link and giving the context about the MD settings.
> 
> I would suggest for Jan, the author of this CL, to have a look at bringing
this
> solution to the MD settings (and consulting you as needed), but perhaps in a
> separate CL.
> 
> Meanwhile, could we please get an OWNERS review to land this?
> 
> Thanks!
> Vaclav

I'm not owner here, but lgtm from me also :-)
Agreed that moving to md-settings is its own CL,
feel free to send reviews for that to me.

[jdoerrie, 2016-10-24 07:34:55]

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:215: }
On 2016/10/21 19:58:40, stevenjb wrote:
> nit: {} not needed

Done.

https://codereview.chromium.org/2439453005/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:220: }
On 2016/10/21 19:58:40, stevenjb wrote:
> nit: {} also not needed here.

Done.

[jdoerrie, 2016-10-24 07:35:20]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2016-10-24 07:35:20]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
stevenjb@chromium.org, hcarmona@chromium.org
Link to the patchset: https://codereview.chromium.org/2439453005/#ps20001
(title: "Adressed nits")

[commit-bot: I haz the power, 2016-10-24 07:35:27]

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-10-24 08:32:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-24 08:35:27]

Description was changed from

==========
Improve updateOriginsEliding_ performance.

This CL is part of the effort to improve performance of the password manager
settings. In particular, this CL fixes layout thrashing in the
updateOriginsEliding_ code. Prior to this change the code was first reading
style information from the DOM and then changing it in a loop.

This change creates a canvas element that is not part of the DOM.
It then uses the canvas to do the text measurements, thus avoiding the
repainting of the DOM. In the end, it writes the final obtained string in the
DOM, and thus does not change the display of the saved passwords.

Reference:
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/mea...

BUG=651049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Improve updateOriginsEliding_ performance.

This CL is part of the effort to improve performance of the password manager
settings. In particular, this CL fixes layout thrashing in the
updateOriginsEliding_ code. Prior to this change the code was first reading
style information from the DOM and then changing it in a loop.

This change creates a canvas element that is not part of the DOM.
It then uses the canvas to do the text measurements, thus avoiding the
repainting of the DOM. In the end, it writes the final obtained string in the
DOM, and thus does not change the display of the saved passwords.

Reference:
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/mea...

BUG=651049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/68f4faa15a37a66d07d374cd7df3dfd296ff957f
Cr-Commit-Position: refs/heads/master@{#427028}
==========

[commit-bot: I haz the power, 2016-10-24 08:35:28]

Patchset 2 (id:??) landed as
https://crrev.com/68f4faa15a37a66d07d374cd7df3dfd296ff957f
Cr-Commit-Position: refs/heads/master@{#427028}