MD Settings: Prevent <settings-languages> running init code after is detached.

MD Settings: Prevent <settings-languages> running init code after is detached.

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

Review-Url: https://codereview.chromium.org/2779873003
Cr-Commit-Position: refs/heads/master@{#460453}
Committed: https://chromium.googlesource.com/chromium/src/+/1696c7fdb2d074501acc0b234874b5c2fab32082

[dpapad, 2017-03-28 18:31:03]

Description was changed from

==========
MD Settings: Prevent <settings-languages> running init code after is detached.

BUG=706078
==========

to

==========
MD Settings: Prevent <settings-languages> running init code after is detached.

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

[dpapad, 2017-03-28 18:31:17]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:32:00]

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

[dpapad, 2017-03-28 18:42:46]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:43:50]

Dry run: 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, 2017-03-28 20:13:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 20:13:23]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dpapad, 2017-03-28 20:28:11]

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

[michaelpg, 2017-03-28 20:30:10]

https://codereview.chromium.org/2779873003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/2779873003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:152: attached:
function() {
Is there a way to do this without waiting for |attached|? I don't have data to
back this up, but I would expect this to have some (possibly miniscule)
performance impact since we have to wait for these round trips from Chrome
before displaying any language settings.

Maybe that means moving only the Promise.all().then() to attached().

[dpapad, 2017-03-28 20:42:25]

On 2017/03/28 at 20:30:10, michaelpg wrote:
>
https://codereview.chromium.org/2779873003/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/languages_page/languages.js (right):
> 
>
https://codereview.chromium.org/2779873003/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/languages_page/languages.js:152: attached:
function() {
> Is there a way to do this without waiting for |attached|? I don't have data to
back this up, but I would expect this to have some (possibly miniscule)
performance impact since we have to wait for these round trips from Chrome
before displaying any language settings.
> 
> Maybe that means moving only the Promise.all().then() to attached().

Tried adding a performance.now() call in 'created' and one in 'attached'. It
shows about 10ms diff between the two calls, so yes that would be the impact in
performance you are referring to. But on the other hand, one could argue that
doing less in created() and more in attached() can benefit the rest of the page,
since it is more "lazy". WDYT?

[michaelpg, 2017-03-28 20:53:01]

lgtm

On 2017/03/28 20:42:25, dpapad wrote:
> On 2017/03/28 at 20:30:10, michaelpg wrote:
> >
>
https://codereview.chromium.org/2779873003/diff/20001/chrome/browser/resource...
> > File chrome/browser/resources/settings/languages_page/languages.js (right):
> > 
> >
>
https://codereview.chromium.org/2779873003/diff/20001/chrome/browser/resource...
> > chrome/browser/resources/settings/languages_page/languages.js:152: attached:
> function() {
> > Is there a way to do this without waiting for |attached|? I don't have data
to
> back this up, but I would expect this to have some (possibly miniscule)
> performance impact since we have to wait for these round trips from Chrome
> before displaying any language settings.
> > 
> > Maybe that means moving only the Promise.all().then() to attached().
> 
> Tried adding a performance.now() call in 'created' and one in 'attached'. It
> shows about 10ms diff between the two calls, so yes that would be the impact
in
> performance you are referring to. But on the other hand, one could argue that
> doing less in created() and more in attached() can benefit the rest of the
page,
> since it is more "lazy". WDYT?

I'll leave that decision to you as you're more familiar with how the page is
loaded and whether that 10ms would even be in a noticeable spot.

FWIW, the languages page UI won't do much work until |languages| is defined,
since a lot of its bindings will be ignored. So that 10ms delay *might* cause
extra Polymer bindings updates in <settings-languages-page> but the effect of
that is unlikely to propagate/compound.

[dpapad, 2017-03-28 20:58:08]

On 2017/03/28 at 20:53:01, michaelpg wrote:
> lgtm
> 
> On 2017/03/28 20:42:25, dpapad wrote:
> > On 2017/03/28 at 20:30:10, michaelpg wrote:
> > >
> >
https://codereview.chromium.org/2779873003/diff/20001/chrome/browser/resource...
> > > File chrome/browser/resources/settings/languages_page/languages.js
(right):
> > > 
> > >
> >
https://codereview.chromium.org/2779873003/diff/20001/chrome/browser/resource...
> > > chrome/browser/resources/settings/languages_page/languages.js:152:
attached:
> > function() {
> > > Is there a way to do this without waiting for |attached|? I don't have
data to
> > back this up, but I would expect this to have some (possibly miniscule)
> > performance impact since we have to wait for these round trips from Chrome
> > before displaying any language settings.
> > > 
> > > Maybe that means moving only the Promise.all().then() to attached().
> > 
> > Tried adding a performance.now() call in 'created' and one in 'attached'. It
> > shows about 10ms diff between the two calls, so yes that would be the impact
in
> > performance you are referring to. But on the other hand, one could argue
that
> > doing less in created() and more in attached() can benefit the rest of the
page,
> > since it is more "lazy". WDYT?
> 
> I'll leave that decision to you as you're more familiar with how the page is
loaded and whether that 10ms would even be in a noticeable spot.
> 
> FWIW, the languages page UI won't do much work until |languages| is defined,
since a lot of its bindings will be ignored. So that 10ms delay *might* cause
extra Polymer bindings updates in <settings-languages-page> but the effect of
that is unlikely to propagate/compound.

Ok thanks for the context. Given that
- the languages page is not showing in the landing page chrome://md-settings,
and that
- the "advanced" page is force rendered after the initial load is done, inside a
requestIdleCallback,

the potential perf impact would only affect direct navigation to
chrome://md-settings/languages, so I don't think it is likely to be a problem.

[dpapad, 2017-03-28 21:41:13]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-03-28 21:41:28]

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, 2017-03-29 04:15:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 04:15:13]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dpapad, 2017-03-29 17:14:58]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-03-29 17:15:23]

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, 2017-03-29 18:01:44]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490807698669950,
"parent_rev": "3e2ce51e88f19f0cc5196284f580583ddd4dbd9a", "commit_rev":
"1696c7fdb2d074501acc0b234874b5c2fab32082"}

[commit-bot: I haz the power, 2017-03-29 18:18:37]

Description was changed from

==========
MD Settings: Prevent <settings-languages> running init code after is detached.

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

to

==========
MD Settings: Prevent <settings-languages> running init code after is detached.

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

Review-Url: https://codereview.chromium.org/2779873003
Cr-Commit-Position: refs/heads/master@{#460453}
Committed:
https://chromium.googlesource.com/chromium/src/+/1696c7fdb2d074501acc0b234874...
==========

[commit-bot: I haz the power, 2017-03-29 18:18:38]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/1696c7fdb2d074501acc0b234874...