Move settings-section animations into setting-section, make better

Move settings-section animations into setting-section, make better

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition to manage
animation lifetime. Most of the section animation/styling logic is
now in settings_section.js where it belongs.

This patch:

* Removes most animation logic from MainPageBehavior and does away
  with TransitionBehavior entirely.

* Makes the collapse animation position: absolute (instead of fixed).
  Anchoring it this way makes it MUCH easier to end up in the right
  place regardless of scroll events, height blips, etc. Should reduce
  jarring jumps at the end of transitions.

* Moves most style changes into Web Animations instead of inline.
  This makes the animations fire-and-forget. Transition clean up is
  largely automatic, there are no styles to (forget to) remove (at the
  wrong time). If a transition is buggy, we're still likely to end up
  in the right place.

* As a result of the above, simplifies the cancel/finish logic. We only
  have one animation going at a time. This is much easier to debug and
  should lead to fewer broken page states.

This also fixes a LOT of bugs (and introduces a few less severe ones).

It's annoying to test animations programmatically. I have CLs with tests,
but they're still flaky.

BUG=589681, 621245, 622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

[michaelpg, 2016-06-28 23:27:33]

Description was changed from

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition
and ExpandCardTransition/CollapseCardTransition, which take over
the animations for showing sections from the MainPageBehavior. This fixes
a bunch of bugs in the process (and probably introduces new ones).

There is a proof-of-concept test; I'll work on test cases for all the
bugs this addresses. It's annoying to test animations programmatically,
but the amount of breakages we've had makes it more painful not to.

BUG=589681,621245,622172
==========

to

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition
and ExpandCardTransition/CollapseCardTransition, which take over
the animations for showing sections from the MainPageBehavior. This fixes
a bunch of bugs in the process (and probably introduces new ones).

There is a proof-of-concept test; I'll work on test cases for all the
bugs this addresses. It's annoying to test animations programmatically,
but the amount of breakages we've had makes it more painful not to.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-06-28 23:28:39]

Description was changed from

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition
and ExpandCardTransition/CollapseCardTransition, which take over
the animations for showing sections from the MainPageBehavior. This fixes
a bunch of bugs in the process (and probably introduces new ones).

There is a proof-of-concept test; I'll work on test cases for all the
bugs this addresses. It's annoying to test animations programmatically,
but the amount of breakages we've had makes it more painful not to.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition
and ExpandCardTransition/CollapseCardTransition.

This removes most animation logic from MainPageBehavior and
does away with TransitionBehavior entirely.

This also fixes a bunch of bugs (and probably introduces new ones).

There is a proof-of-concept test; I'll work on test cases for all the
bugs this addresses. It's annoying to test animations programmatically,
but the amount of breakages we've had makes it more painful not to.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-06-28 23:28:39]

michaelpg@chromium.org changed reviewers:
+ dbeam@chromium.org, dschuyler@chromium.org

[michaelpg, 2016-06-28 23:45:16]

the big one. I'm happy with where this is. PTAL and let me know your thoughts.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.html (right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.html:41:
on-subpage-expand="subpageExpanded_"
dschuyler, please take a look at these changes. We can't have the advanced page
and the toggle button disappear when a section is clicked, because that (a)
looks weird and (b) shortens the page which can change the scroll height.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/expand_card_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:150:
this.finished.catch(function() {}).then(function() {
Is this idiom OK? The alternative is to pass two functions to then() which both
do the same thing. IMO this is closer to the familiar "try, catch, finally"
structure.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(left):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:86:
this.startExpandSection_(section);
This whole mess is why making everything Promise-aware is nicer.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:99: //
scrollbar we don't want to account for.
Oops, this might be old logic, I'll check if it can be removed.

[dschuyler, 2016-06-30 01:24:05]

I'm going to need to look this over again.
In the meantime, I've noted some minor/easy
things.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/expand_card_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:6: 
It kinda looks like most of this file
should be indented one more indent. Is
it intentionally not indented?

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:130:
parseInt(headerStyle.marginTop, 10);
I believe these may be floats. Consider
parseFloat(foo)

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:64: //
Nothing to see here....
Funny comment. Though maybe strike it. :)
(or at least s/..../.../).

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:141:
this.async(function() {
Can this be done through a positive signal
of readiness, rather than an async? (maybe
a promise or event).

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/open_section_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/open_section_transition.js:6: 
Please indent code within braces.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/open_section_transition.js:184:
}.bind(this), function() {
since these are separate parameters, let's line
break after the comma.

https://codereview.chromium.org/2106013002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/open_section_transition_browsertest.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/open_section_transition_browsertest.js:84: 
extra blank line.

[michaelpg, 2016-07-07 05:22:29]

Patchset #6 (id:100001) has been deleted

[michaelpg, 2016-07-07 05:27:44]

Forgot to send this out; mind taking another look?

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/expand_card_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:6: 
On 2016/06/30 01:24:04, dschuyler wrote:
> It kinda looks like most of this file
> should be indented one more indent. Is
> it intentionally not indented?

Done.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:130:
parseInt(headerStyle.marginTop, 10);
On 2016/06/30 01:24:04, dschuyler wrote:
> I believe these may be floats. Consider
> parseFloat(foo)

Done.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:64: //
Nothing to see here....
On 2016/06/30 01:24:04, dschuyler wrote:
> Funny comment. Though maybe strike it. :)
> (or at least s/..../.../).

Omit the sentence-ending period after an ellipsis...? Never!

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:99: //
scrollbar we don't want to account for.
On 2016/06/28 23:45:16, michaelpg wrote:
> Oops, this might be old logic, I'll check if it can be removed.

Done.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:141:
this.async(function() {
On 2016/06/30 01:24:04, dschuyler wrote:
> Can this be done through a positive signal
> of readiness, rather than an async? (maybe
> a promise or event).

I'd prefer to do that through the router rather than make this even more tightly
coupled to settings-main. Added a TODO, LMK if that's ok.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/open_section_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/open_section_transition.js:6: 
On 2016/06/30 01:24:04, dschuyler wrote:
> Please indent code within braces.

Done.

https://codereview.chromium.org/2106013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/open_section_transition.js:184:
}.bind(this), function() {
On 2016/06/30 01:24:04, dschuyler wrote:
> since these are separate parameters, let's line
> break after the comma.

Done.

https://codereview.chromium.org/2106013002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/open_section_transition_browsertest.js
(right):

https://codereview.chromium.org/2106013002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/open_section_transition_browsertest.js:84: 
On 2016/06/30 01:24:04, dschuyler wrote:
> extra blank line.

Done.

[Dan Beam, 2016-07-07 18:15:39]

https://codereview.chromium.org/2106013002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:118:
newRoute.page == 'advanced';
should all of these be computed separtely with their own dependencies?  i.e.

showAdvancedPage_: {
  computed: 'computeShowAdvancedPage_(isAdvancedMenuOpen_, '
                                     'showAdvancedToggle_, ' +
                                     'currentRoute)',
},

i think it'd be a little less error prone if these were computed separately at
this point

https://codereview.chromium.org/2106013002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:120: 
/** @private */

[Dan Beam, 2016-07-07 18:27:54]

did you meant to remove transition_behavior.*?

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/expand_card_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:20:
this.animation_ = null;
why not just put this @type when initializing?

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:65: 
as in here:

/** @type {?settings.animation.Animation} */

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:89:
finished: null,
why is this necessary?

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:101:
this.card = section.$['card'];
this.card = assert(section.$['card']);

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:101:
this.card = section.$['card'];
should card be @private (i.e. card_)?

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:172:
},
why can't we make a @protected |this.animation| in the base class and put both
finish and cancel there (if they're the same between both)?

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/transition_behavior.js
(left):

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/transition_behavior.js:57:
reject();
we probably shouldn't be rejecting for this.  i think we should just be
resolving with an argument of whether the animation succeeded

[dschuyler, 2016-07-07 18:46:21]

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:113: var
isSubpage = !!newRoute.subpage.length;
Is isSubpage unused?

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:96:
.then(function() {
This looks like a different style than is
commonly done: where the .then is on the 
following line rather than the line above.
Is this a whoops or an intentional deviation?
(Not saying this must change, just asking
in case it's a whoops).

[michaelpg, 2016-07-08 00:19:02]

On 2016/07/07 18:27:54, Dan Beam wrote:
> did you meant to remove transition_behavior.*?

Yes. The logic is moved to Animation and MainPageBehavior's use of individual
AnimationGroups with promises.

https://codereview.chromium.org/2106013002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:118:
newRoute.page == 'advanced';
On 2016/07/07 18:15:38, Dan Beam wrote:
> should all of these be computed separtely with their own dependencies?  i.e.
> 
> showAdvancedPage_: {
>   computed: 'computeShowAdvancedPage_(isAdvancedMenuOpen_, '
>                                      'showAdvancedToggle_, ' +
>                                      'currentRoute)',
> },
> 
> i think it'd be a little less error prone if these were computed separately at
> this point

Done, great suggestion!

https://codereview.chromium.org/2106013002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:120: 
On 2016/07/07 18:15:38, Dan Beam wrote:
> /** @private */

Done.

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:113: var
isSubpage = !!newRoute.subpage.length;
On 2016/07/07 18:46:21, dschuyler wrote:
> Is isSubpage unused?

Done.

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/expand_card_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:20:
this.animation_ = null;
On 2016/07/07 18:27:54, Dan Beam wrote:
> why not just put this @type when initializing?

Done.

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:65: 
On 2016/07/07 18:27:54, Dan Beam wrote:
> as in here:
> 
> /** @type {?settings.animation.Animation} */

Done.

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:89:
finished: null,
On 2016/07/07 18:27:54, Dan Beam wrote:
> why is this necessary?

it's required. otherwise:

##
/foo/src/chrome/browser/resources/settings/settings_page/expand_card_transition.js:15:
ERROR - property finished on interface settings.animation.AnimationGroup is not
implemented by type settings.ExpandCardTransition

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:101:
this.card = section.$['card'];
On 2016/07/07 18:27:54, Dan Beam wrote:
> this.card = assert(section.$['card']);

Done.

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:101:
this.card = section.$['card'];
On 2016/07/07 18:27:54, Dan Beam wrote:
> should card be @private (i.e. card_)?

Done.

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:172:
},
On 2016/07/07 18:27:54, Dan Beam wrote:
> why can't we make a @protected |this.animation| in the base class and put both
> finish and cancel there (if they're the same between both)?

the base interface is AnimationGroup; it doesn't require that all
implementations have some single web animation (case in point,
{Open,Close}SectionTransition).

I thought about making an intermediate class just for
{Expand,Collapse}CardTransition to extend, e.g. "SingleAnimation", but figured
I'd wait until there are more instances.

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:96:
.then(function() {
On 2016/07/07 18:46:21, dschuyler wrote:
> This looks like a different style than is
> commonly done: where the .then is on the 
> following line rather than the line above.
> Is this a whoops or an intentional deviation?
> (Not saying this must change, just asking
> in case it's a whoops).

Thanks for asking, but yes, this was very intentional; my impression was that
this was easier to read ("then, catch, then"). I'm open to suggestions, though!

What I have: http://pastebin.com/raw/74znLD6h

What we normally do: http://pastebin.com/raw/TcUCAASm

(remove "/raw" for syntax highlighting, but it sucks)

Another alt: http://pastebin.com/raw/7KvPa8kK

[Dan Beam, 2016-07-12 01:36:48]

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/transition_behavior.js
(left):

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/transition_behavior.js:57:
reject();
On 2016/07/07 18:27:54, Dan Beam wrote:
> we probably shouldn't be rejecting for this.  i think we should just be
> resolving with an argument of whether the animation succeeded

ping

[Dan Beam, 2016-07-12 02:01:55]

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:31: /**
@private */
it might be worth making a separate CL for these computed properties changes

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:151: page ==
'advanced' && !inSubpage;
please add ()s to clarify this

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:160: page ==
'basic' && advancedToggleExpanded && !inSubpage;
same

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/expand_card_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:101:
this.card_ = assert(section.$['card']);
can you make this not poke into $.idName?  can we just pass section.$.card from
the calling code if it's within SettingsSectionElement?

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:111:
/** Sets up the transition before other page content has been unhidden. */
can you note how this should be used?  i.e. call before play() or something?

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:127:
var headerStyle = getComputedStyle(this.section_.$['header']);
same here: can we expose header rather than use the local DOM ID collection?

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:79:
.then(this.expandSection.bind(this, section));
i don't see lines like this as a big improvement wrt indentation

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.html:11: /**
Margin so the box-shadow isn't clipped during animations. */
nit: /*

https://codereview.chromium.org/2106013002/diff/160001/chrome/test/data/webui...
File chrome/test/data/webui/settings/open_section_transition_browsertest.js
(right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/settings/open_section_transition_browsertest.js:82:
expectLE(settings.animation.Timing.DURATION, totalTime);
nit: I would reverse this, i.e.

expectGE(totalTime, settings.animation.Timing.DURATION);

[michaelpg, 2016-07-19 23:35:31]

Description was changed from

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition
and ExpandCardTransition/CollapseCardTransition.

This removes most animation logic from MainPageBehavior and
does away with TransitionBehavior entirely.

This also fixes a bunch of bugs (and probably introduces new ones).

There is a proof-of-concept test; I'll work on test cases for all the
bugs this addresses. It's annoying to test animations programmatically,
but the amount of breakages we've had makes it more painful not to.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition
and ExpandCardTransition/CollapseCardTransition.

This removes most animation logic from MainPageBehavior and
does away with TransitionBehavior entirely.

This also fixes a bunch of bugs (and probably introduces new ones).

There is a proof-of-concept test; I'll work on test cases for all the
bugs this addresses. It's annoying to test animations programmatically,
but the amount of breakages we've had makes it more painful not to.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-07-20 22:10:58]

PTAL. I think I've addressed or obsoleted all comments.

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/transition_behavior.js
(left):

https://codereview.chromium.org/2106013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/transition_behavior.js:57:
reject();
On 2016/07/12 01:36:48, Dan Beam wrote:
> On 2016/07/07 18:27:54, Dan Beam wrote:
> > we probably shouldn't be rejecting for this.  i think we should just be
> > resolving with an argument of whether the animation succeeded
> 
> ping

?? this whole file was deleted. you had a similar comment about
c/b/r/settings/animation/animation.js, but I pointed out that's the behavior
prescribed by the Animation interface.

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:31: /**
@private */
On 2016/07/12 02:01:54, Dan Beam wrote:
> it might be worth making a separate CL for these computed properties changes

Done, see depends-on CL

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:152: },
Removed.

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:160: page ==
'basic' && advancedToggleExpanded && !inSubpage;
On 2016/07/12 02:01:54, Dan Beam wrote:
> same

Acknowledged.

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/expand_card_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:101:
this.card_ = assert(section.$['card']);
On 2016/07/12 02:01:54, Dan Beam wrote:
> can you make this not poke into $.idName?  can we just pass section.$.card
from
> the calling code if it's within SettingsSectionElement?

Done.

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:111:
/** Sets up the transition before other page content has been unhidden. */
On 2016/07/12 02:01:54, Dan Beam wrote:
> can you note how this should be used?  i.e. call before play() or something?

Done.

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:127:
var headerStyle = getComputedStyle(this.section_.$['header']);
On 2016/07/12 02:01:54, Dan Beam wrote:
> same here: can we expose header rather than use the local DOM ID collection?

Done.

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:79:
.then(this.expandSection.bind(this, section));
On 2016/07/12 02:01:54, Dan Beam wrote:
> i don't see lines like this as a big improvement wrt indentation

Acknowledged. Breaking here after .then( instead.

But in the above block (this.openSectionTransition_.finished), and below
(this.openSectionTransition_.play()), I think it adds clarity.

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.html:11: /**
Margin so the box-shadow isn't clipped during animations. */
On 2016/07/12 02:01:54, Dan Beam wrote:
> nit: /*

Done.

https://codereview.chromium.org/2106013002/diff/160001/chrome/test/data/webui...
File chrome/test/data/webui/settings/open_section_transition_browsertest.js
(right):

https://codereview.chromium.org/2106013002/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/settings/open_section_transition_browsertest.js:82:
expectLE(settings.animation.Timing.DURATION, totalTime);
On 2016/07/12 02:01:55, Dan Beam wrote:
> nit: I would reverse this, i.e.
> 
> expectGE(totalTime, settings.animation.Timing.DURATION);

agreed, Done

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/expand_card_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/expand_card_transition.js:113: *
Prepare for the transition before other page content has been unhidden.
"Prepares"

[dschuyler, 2016-07-21 03:26:09]

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/animation/animation_group.js (right):

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/animation/animation_group.js:6: 'use strict';
Adding 'use strict' is cool, but why is it only added here
and not in the other new js files in this CL?

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:72:
}).catch(function() {})
Does this .catch hide something specific?
Maybe add a comment about what that is?

[Dan Beam, 2016-07-21 03:55:11]

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/animation/animation_group.js (right):

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/animation/animation_group.js:39: * Resolved or
rejected when the the AnimationGroup finishes or is canceled.
I think for no other reason than not having to copy pasta a bunch of empty
catch(function() {}) we should make this a Promise<boolean> and pass whether the
animation completed successfully (and not reject() if it's cancelled early)

[michaelpg, 2016-07-22 15:23:35]

Patchset #12 (id:240001) has been deleted

[michaelpg, 2016-07-22 15:47:39]

Patchset #12 (id:260001) has been deleted

[michaelpg, 2016-07-22 16:05:50]

PTAL.

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/animation/animation_group.js (right):

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/animation/animation_group.js:6: 'use strict';
On 2016/07/21 03:26:09, dschuyler wrote:
> Adding 'use strict' is cool, but why is it only added here
> and not in the other new js files in this CL?

Dunno, added it now. Thanks!

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/animation/animation_group.js:39: * Resolved or
rejected when the the AnimationGroup finishes or is canceled.
On 2016/07/21 03:55:10, Dan Beam wrote:
> I think for no other reason than not having to copy pasta a bunch of empty
> catch(function() {}) we should make this a Promise<boolean> and pass whether
the
> animation completed successfully (and not reject() if it's cancelled early)

Done.

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:72:
}).catch(function() {})
On 2016/07/21 03:26:09, dschuyler wrote:
> Does this .catch hide something specific?
> Maybe add a comment about what that is?

Removed now that AnimationGroup promises resolve on cancel.

[dschuyler, 2016-07-22 22:34:10]

lgtm

[Dan Beam, 2016-07-23 01:13:45]

i really think that moving the freezing logic into sections and/or cards would
clarify this code a lot.  is that possible?

https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:98:
this.inSubpage_ = this.currentRoute.subpage.length > 0;
so we don't need to call updatePagesShown_() here? because we're doing it from
currentRouteChanged_?

https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:122:
assert(this.closeSectionTransition_.section == section);
i don't really understand why this assert() must be true, but sure!

https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/open_section_transition.js
(right):

https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/open_section_transition.js:14:
var card = section.$['card'];
can we make this not poke into otherObject.$.*?

even better, make a .frozen on section and/or card and let them do the freezing
internally?

https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/open_section_transition.js:68:
this.container = container;
why do we need this.container?  where is it used?

https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/open_section_transition.js:140:
this.container = container;
why do we need this.container?

[michaelpg, 2016-07-29 17:30:10]

On 2016/07/23 01:13:45, Dan Beam wrote:
> i really think that moving the freezing logic into sections and/or cards would
> clarify this code a lot.  is that possible?

status update: yes, it helps a lot. I've made the change but it makes this CL
even larger and more of a rebase landmine. I'm going to split up what we already
have into a few CLs and give each their own tests. Should take <1 day and be
easier for you to review.

> 
>
https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
> File chrome/browser/resources/settings/settings_main/settings_main.js (right):
> 
>
https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
> chrome/browser/resources/settings/settings_main/settings_main.js:98:
> this.inSubpage_ = this.currentRoute.subpage.length > 0;
> so we don't need to call updatePagesShown_() here? because we're doing it from
> currentRouteChanged_?
> 
>
https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
> File chrome/browser/resources/settings/settings_page/main_page_behavior.js
> (right):
> 
>
https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
> chrome/browser/resources/settings/settings_page/main_page_behavior.js:122:
> assert(this.closeSectionTransition_.section == section);
> i don't really understand why this assert() must be true, but sure!
> 
>
https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
> File
chrome/browser/resources/settings/settings_page/open_section_transition.js
> (right):
> 
>
https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
> chrome/browser/resources/settings/settings_page/open_section_transition.js:14:
> var card = section.$['card'];
> can we make this not poke into otherObject.$.*?
> 
> even better, make a .frozen on section and/or card and let them do the
freezing
> internally?
> 
>
https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
> chrome/browser/resources/settings/settings_page/open_section_transition.js:68:
> this.container = container;
> why do we need this.container?  where is it used?
> 
>
https://codereview.chromium.org/2106013002/diff/300001/chrome/browser/resourc...
>
chrome/browser/resources/settings/settings_page/open_section_transition.js:140:
> this.container = container;
> why do we need this.container?

[michaelpg, 2016-08-04 06:56:51]

Description was changed from

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition
and ExpandCardTransition/CollapseCardTransition.

This removes most animation logic from MainPageBehavior and
does away with TransitionBehavior entirely.

This also fixes a bunch of bugs (and probably introduces new ones).

There is a proof-of-concept test; I'll work on test cases for all the
bugs this addresses. It's annoying to test animations programmatically,
but the amount of breakages we've had makes it more painful not to.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition to manage
animation lifetime. Most of the section animation/styling logic is
now in settings_section.js where it belongs.

This patch:

* Removes most animation logic from MainPageBehavior and does away
  with TransitionBehavior entirely.

* Makes the collapse animation position: absolute (instead of fixed).
  Anchoring it this way makes it MUCH easier to end up in the right
  place regardless of scroll events, height blips, etc. Should reduce
  jarring jumps at the end of transitions.

* Moves most style changes into Web Animations instead of inline.
  This makes the animations fire-and-forget. Transition clean up is
  largely automatic, there are no styles to (forget to) remove (at the
  wrong time). If a transition is buggy, we're still likely to end up
  in the right place.

This also fixes a LOT of bugs (and introduces a few less severe ones).

It's annoying to test animations programmatically. I have CLs with tests,
but they're still flaky.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-08-04 06:58:51]

Description was changed from

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition to manage
animation lifetime. Most of the section animation/styling logic is
now in settings_section.js where it belongs.

This patch:

* Removes most animation logic from MainPageBehavior and does away
  with TransitionBehavior entirely.

* Makes the collapse animation position: absolute (instead of fixed).
  Anchoring it this way makes it MUCH easier to end up in the right
  place regardless of scroll events, height blips, etc. Should reduce
  jarring jumps at the end of transitions.

* Moves most style changes into Web Animations instead of inline.
  This makes the animations fire-and-forget. Transition clean up is
  largely automatic, there are no styles to (forget to) remove (at the
  wrong time). If a transition is buggy, we're still likely to end up
  in the right place.

This also fixes a LOT of bugs (and introduces a few less severe ones).

It's annoying to test animations programmatically. I have CLs with tests,
but they're still flaky.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition to manage
animation lifetime. Most of the section animation/styling logic is
now in settings_section.js where it belongs.

This patch:

* Removes most animation logic from MainPageBehavior and does away
  with TransitionBehavior entirely.

* Makes the collapse animation position: absolute (instead of fixed).
  Anchoring it this way makes it MUCH easier to end up in the right
  place regardless of scroll events, height blips, etc. Should reduce
  jarring jumps at the end of transitions.

* Moves most style changes into Web Animations instead of inline.
  This makes the animations fire-and-forget. Transition clean up is
  largely automatic, there are no styles to (forget to) remove (at the
  wrong time). If a transition is buggy, we're still likely to end up
  in the right place.

* As a result of the above, simplifies the cancel/finish logic. We only
  have one animation going at a time. This is much easier to debug and
  should lead to fewer broken page states.

This also fixes a LOT of bugs (and introduces a few less severe ones).

It's annoying to test animations programmatically. I have CLs with tests,
but they're still flaky.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-08-04 07:07:36]

PTAL. I don't recommend trying to diff with previous patches, just look through
the new changes.

This CL is big (+570 lines, -428 lines).
But the previous patch was bigger (+680 lines, -363 lines).

Dan's suggestion to move the settings-section manipulation really helped. I also
broke off a couple tiny pieces which t his depends on, but this is largely
impossible to change in small chunks. The animations are MUCH smoother and MUCH
less likely to break now, and it will be much easier to fix the remaining
jank/blips.

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:79: // Used to
control overscroll behavior while animating sections.
Dave, these events let transitions actually start at the right place. Otherwise,
an animation would start, the page would immediately change height (and scroll),
and the animation would be offset.

Do you have any better ideas for handling this scenario?

[michaelpg, 2016-08-04 07:08:00]

Description was changed from

==========
Wrap Settings animations in modular classes and begin tests

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition to manage
animation lifetime. Most of the section animation/styling logic is
now in settings_section.js where it belongs.

This patch:

* Removes most animation logic from MainPageBehavior and does away
  with TransitionBehavior entirely.

* Makes the collapse animation position: absolute (instead of fixed).
  Anchoring it this way makes it MUCH easier to end up in the right
  place regardless of scroll events, height blips, etc. Should reduce
  jarring jumps at the end of transitions.

* Moves most style changes into Web Animations instead of inline.
  This makes the animations fire-and-forget. Transition clean up is
  largely automatic, there are no styles to (forget to) remove (at the
  wrong time). If a transition is buggy, we're still likely to end up
  in the right place.

* As a result of the above, simplifies the cancel/finish logic. We only
  have one animation going at a time. This is much easier to debug and
  should lead to fewer broken page states.

This also fixes a LOT of bugs (and introduces a few less severe ones).

It's annoying to test animations programmatically. I have CLs with tests,
but they're still flaky.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Move settings-section animations into setting-section, make better

This follows https://codereview.chromium.org/2072643002/
by creating OpenSectionTransition/CloseSectionTransition to manage
animation lifetime. Most of the section animation/styling logic is
now in settings_section.js where it belongs.

This patch:

* Removes most animation logic from MainPageBehavior and does away
  with TransitionBehavior entirely.

* Makes the collapse animation position: absolute (instead of fixed).
  Anchoring it this way makes it MUCH easier to end up in the right
  place regardless of scroll events, height blips, etc. Should reduce
  jarring jumps at the end of transitions.

* Moves most style changes into Web Animations instead of inline.
  This makes the animations fire-and-forget. Transition clean up is
  largely automatic, there are no styles to (forget to) remove (at the
  wrong time). If a transition is buggy, we're still likely to end up
  in the right place.

* As a result of the above, simplifies the cancel/finish logic. We only
  have one animation going at a time. This is much easier to debug and
  should lead to fewer broken page states.

This also fixes a LOT of bugs (and introduces a few less severe ones).

It's annoying to test animations programmatically. I have CLs with tests,
but they're still flaky.

BUG=589681,621245,622172
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-08-04 18:36:51]

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.html (right):

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.html:65: 
I'm fine with a blank line or not having
a blank line. I'm pointing it out in case
it's a whoops.

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:99:
this.$.overscroll.style.paddingBottom = this.overscrollHeight_() + 'px';
The overscroll calculation changed in a CL yesterday.
Consider pulling new code. It's now using a property
to set the overscroll height.

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_section.js:9: 
Maybe remove blank line or add an '*'?

[Dan Beam, 2016-08-05 00:18:06]

i really think it'd be a better idea to split this up

i think it's bound to take forever to review like it is

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:32: /**
@type {!HTMLElement} */
indent off

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:33:
this.scroller = (this.domHost && this.domHost.parentNode.tagName ==
what's the functional difference here?  why are you making a ternary?

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:35:
this.domHost.parentNode.$.mainContainer :
yeppppppp this'll trigger that presubmit

[michaelpg, 2016-08-05 00:20:27]

Dan has pointed out that this is harder, not easier, to review that the previous
patch. I will try to split it up more aggressively and open separate CLs then.

[Dan Beam, 2016-08-05 00:47:07]

this is looking better but it's still fairly hard to grok (and i think we'll all
miss things while it's big and hard to keep in sync)

i don't think you HAVE to split, but i think it'll result in landing this faster

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:99:
this.$.overscroll.style.paddingBottom = this.overscrollHeight_() + 'px';
On 2016/08/04 18:36:51, dschuyler wrote:
> The overscroll calculation changed in a CL yesterday.
> Consider pulling new code. It's now using a property
> to set the overscroll height.

yep, this it out of date

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:53: *
Transistions to the current route if the |deferredTransition_| flag is set.
Transitions

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:56:
runDeferredTransition_: function() {
this makes it seem as if "deferredTransition_" is a @type {Transition} that's
been set previously.  but it's actually a boolean.  can you name this
wasDeferred_ or something less ambiguous?

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:78: //
settings-animated-pages controls visibility of subpages in sections.
can you describe this in a different, more high-level way. possibly with
concrete examples.  as in: this happens when a dialog or subpage is shown.

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:92: if
(this.currentRoute.section.length == 0)
nit: can you just use truthiness for section or other strings?  makes it easier
to tell they're not arrays like subpage is

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:105: 
can you put a comment saying which cases actually make it here?

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:206:
this.scroller.style.width = '';
marginally useful: make a lockScrolling_() and unlockScrolling_() that sets
overflow to 'hidden' or '' and possibly dabbles with width in some cases?

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:273:
newRoute.subpage.length == 0)) {
can you use some temporary variables for this?

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page_css.html (right):

https://codereview.chromium.org/2106013002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page_css.html:40:
settings-section.collapsing {
why are you adding this?  i had to remove a tag names recently because they
weren't helpful.  for specificity?  if so: which rule?

[michaelpg, 2016-08-05 02:15:58]

On 2016/08/05 00:47:07, Dan Beam wrote:
> this is looking better but it's still fairly hard to grok (and i think we'll
all
> miss things while it's big and hard to keep in sync)
> 
> i don't think you HAVE to split, but i think it'll result in landing this
faster

first part: https://codereview.chromium.org/2215213002/