MD Settings: Cleanup, remove unnecessary <cr-settings> wrapper.

MD Settings: Cleanup, remove unnecessary <cr-settings> wrapper.

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

Committed: https://crrev.com/113dc343df448872e655b8d1a0dd78116a6f26b6
Cr-Commit-Position: refs/heads/master@{#421588}

[dpapad, 2016-09-23 01:48:36]

Description was changed from

==========
MD Settings: Cleanup, remove unnecessary cr-settings wrapper.

BUG=None
==========

to

==========
MD Settings: Cleanup, remove unnecessary cr-settings wrapper.

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

[dpapad, 2016-09-23 01:48:54]

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

[commit-bot: I haz the power, 2016-09-23 01:49:16]

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

[dpapad, 2016-09-23 01:52:50]

Description was changed from

==========
MD Settings: Cleanup, remove unnecessary cr-settings wrapper.

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

to

==========
MD Settings: Cleanup, remove unnecessary <cr-settings> wrapper.

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

[commit-bot: I haz the power, 2016-09-23 03:31:13]

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

[commit-bot: I haz the power, 2016-09-23 03:31:14]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-09-23 18:19:17]

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

[Dan Beam, 2016-09-23 18:29:59]

Description was changed from

==========
MD Settings: Cleanup, remove unnecessary <cr-settings> wrapper.

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

to

==========
MD Settings: Cleanup, remove unnecessary <cr-settings> wrapper.

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

[Dan Beam, 2016-09-23 18:30:00]

dbeam@chromium.org changed reviewers:
+ michaelpg@chromium.org
- dbeam@chromium.org

[Dan Beam, 2016-09-23 18:30:04]

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

[Dan Beam, 2016-09-23 18:30:05]

+michaelpg@

[michaelpg, 2016-09-24 00:45:30]

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

[dpapad, 2016-09-27 19:09:10]

@michaelpg: Friendly ping.

[michaelpg, 2016-09-27 20:15:23]

have you tested this exteeeeensively? I'm worried about order of operations.

With this CL, in a fresh profile, the People section flickers briefly while
opening Settings. This might suggest the people handler's callback is running
sooner. :-\

[dpapad, 2016-09-27 20:50:14]

On 2016/09/27 at 20:15:23, michaelpg wrote:
> have you tested this exteeeeensively? I'm worried about order of operations.
> 
> With this CL, in a fresh profile, the People section flickers briefly while
opening Settings. This might suggest the people handler's callback is running
sooner. :-\

I am noticing the flicker now too (yes had tested it, without noticing it
before). I think the entire page is shown sooner (which is good), but the
flicker is obviously not desired. Will investigate how this change affects the
order of operations causing the flicker. Do you know/remember the original
reason for the cr-settings and settings-ui existence as separate elements?

[Dan Beam, 2016-09-27 22:23:05]

On 2016/09/27 20:50:14, dpapad wrote:
> On 2016/09/27 at 20:15:23, michaelpg wrote:
> > have you tested this exteeeeensively? I'm worried about order of operations.
> > 
> > With this CL, in a fresh profile, the People section flickers briefly while
> opening Settings. This might suggest the people handler's callback is running
> sooner. :-\
> 
> I am noticing the flicker now too (yes had tested it, without noticing it
> before). I think the entire page is shown sooner (which is good), but the
> flicker is obviously not desired. Will investigate how this change affects the
> order of operations causing the flicker. Do you know/remember the original
> reason for the cr-settings and settings-ui existence as separate elements?

dpapad@ just showed me this locally.

fwiw: there's been a ton of times people have sent me CLs that hid the whole UI
(i.e. settings page) and revealed it when it was all ready.  but this means that
the slowest part of your UI determines when the whole thing shows.

additionally, the sync section is notoriously flickery in the old UI.  we
pre-gathered information in loadTimeData, which is essentially guaranteed to
exist on creation time, and made the sync section render correctly with earlier,
semi-static data.  this helped.  it's possible that we've regressed slightly in
this area.  this is a hard section, because it's highly visible (first section
on desktop) and incredibly dynamic (the section shifts a lot depending on
signedin-ness).

I ultimately don't think we should be implicitly or explicitly showing the whole
UI when "it's all loaded", and making discrete changes to address flicker is the
price we pay for doing dynamic, client-side logic (rather than like a more
typical server-side-y HTML generation).

tl;dr - I think added flicker is fine, and we can spot fix

[Dan Beam, 2016-09-27 23:08:40]

lgtm

[michaelpg, 2016-09-28 01:01:19]

On 2016/09/27 22:23:05, Dan Beam wrote:
> On 2016/09/27 20:50:14, dpapad wrote:
> > On 2016/09/27 at 20:15:23, michaelpg wrote:
> > > have you tested this exteeeeensively? I'm worried about order of
operations.
> > > 
> > > With this CL, in a fresh profile, the People section flickers briefly
while
> > opening Settings. This might suggest the people handler's callback is
running
> > sooner. :-\
> > 
> > I am noticing the flicker now too (yes had tested it, without noticing it
> > before). I think the entire page is shown sooner (which is good), but the
> > flicker is obviously not desired. Will investigate how this change affects
the
> > order of operations causing the flicker. Do you know/remember the original
> > reason for the cr-settings and settings-ui existence as separate elements?
> 
> dpapad@ just showed me this locally.
> 
> fwiw: there's been a ton of times people have sent me CLs that hid the whole
UI
> (i.e. settings page) and revealed it when it was all ready.  but this means
that
> the slowest part of your UI determines when the whole thing shows.
> 
> additionally, the sync section is notoriously flickery in the old UI.  we
> pre-gathered information in loadTimeData, which is essentially guaranteed to
> exist on creation time, and made the sync section render correctly with
earlier,
> semi-static data.  this helped.  it's possible that we've regressed slightly
in
> this area.  this is a hard section, because it's highly visible (first section
> on desktop) and incredibly dynamic (the section shifts a lot depending on
> signedin-ness).
> 
> I ultimately don't think we should be implicitly or explicitly showing the
whole
> UI when "it's all loaded", and making discrete changes to address flicker is
the
> price we pay for doing dynamic, client-side logic (rather than like a more
> typical server-side-y HTML generation).
> 
> tl;dr - I think added flicker is fine, and we can spot fix

Agree with tl;dr. But the flicker shows that something fundamental has changed
(it's not just loading faster because we got rid of 1 element, it's doing things
in
a different order) which we don't fully understand. That's why I'm asking how
extensively this was tested -- there are a ton of effects it could have beyond
just
how fast the page loads.

So, yeah -- if you haven't found anything broken, navigating between pages works
as
before, directly loading sub-page URLs works as before, then lgtm.

[dpapad, 2016-09-28 01:46:27]

On 2016/09/28 at 01:01:19, michaelpg wrote:
> On 2016/09/27 22:23:05, Dan Beam wrote:
> > On 2016/09/27 20:50:14, dpapad wrote:
> > > On 2016/09/27 at 20:15:23, michaelpg wrote:
> > > > have you tested this exteeeeensively? I'm worried about order of
operations.
> > > > 
> > > > With this CL, in a fresh profile, the People section flickers briefly
while
> > > opening Settings. This might suggest the people handler's callback is
running
> > > sooner. :-\
> > > 
> > > I am noticing the flicker now too (yes had tested it, without noticing it
> > > before). I think the entire page is shown sooner (which is good), but the
> > > flicker is obviously not desired. Will investigate how this change affects
the
> > > order of operations causing the flicker. Do you know/remember the original
> > > reason for the cr-settings and settings-ui existence as separate elements?
> > 
> > dpapad@ just showed me this locally.
> > 
> > fwiw: there's been a ton of times people have sent me CLs that hid the whole
UI
> > (i.e. settings page) and revealed it when it was all ready.  but this means
that
> > the slowest part of your UI determines when the whole thing shows.
> > 
> > additionally, the sync section is notoriously flickery in the old UI.  we
> > pre-gathered information in loadTimeData, which is essentially guaranteed to
> > exist on creation time, and made the sync section render correctly with
earlier,
> > semi-static data.  this helped.  it's possible that we've regressed slightly
in
> > this area.  this is a hard section, because it's highly visible (first
section
> > on desktop) and incredibly dynamic (the section shifts a lot depending on
> > signedin-ness).
> > 
> > I ultimately don't think we should be implicitly or explicitly showing the
whole
> > UI when "it's all loaded", and making discrete changes to address flicker is
the
> > price we pay for doing dynamic, client-side logic (rather than like a more
> > typical server-side-y HTML generation).
> > 
> > tl;dr - I think added flicker is fine, and we can spot fix
> 
> Agree with tl;dr. But the flicker shows that something fundamental has changed
> (it's not just loading faster because we got rid of 1 element, it's doing
things in
> a different order) which we don't fully understand. That's why I'm asking how
> extensively this was tested -- there are a ton of effects it could have beyond
just
> how fast the page loads.
> 
> So, yeah -- if you haven't found anything broken, navigating between pages
works as
> before, directly loading sub-page URLs works as before, then lgtm.

I did a fair amount of navigating around and clicking on things, as well as
loading sub-page URLs directly. Could not find anything obviously broken,
neither did I encounter any exceptions in the console. That said, I am pretty
sure that QA will file bugs about the flickering, but also I agree with Dan's
tl;dr; assessment.

[dpapad, 2016-09-28 17:24:54]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-09-28 17:25: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-09-28 18:37:33]

Description was changed from

==========
MD Settings: Cleanup, remove unnecessary <cr-settings> wrapper.

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

to

==========
MD Settings: Cleanup, remove unnecessary <cr-settings> wrapper.

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

[commit-bot: I haz the power, 2016-09-28 18:37:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-28 18:39:37]

Description was changed from

==========
MD Settings: Cleanup, remove unnecessary <cr-settings> wrapper.

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

to

==========
MD Settings: Cleanup, remove unnecessary <cr-settings> wrapper.

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

Committed: https://crrev.com/113dc343df448872e655b8d1a0dd78116a6f26b6
Cr-Commit-Position: refs/heads/master@{#421588}
==========

[commit-bot: I haz the power, 2016-09-28 18:39:38]

Patchset 3 (id:??) landed as
https://crrev.com/113dc343df448872e655b8d1a0dd78116a6f26b6
Cr-Commit-Position: refs/heads/master@{#421588}