Move settings-section expand/collapse animations to settings-section

Move settings-section expand/collapse animations to settings-section

Adds functions on settings-section for the expand/collapse animations. These
functions just copied the code from the current main_page_behavior.js, with a
few necessary changes to return values and parameters. There should be NO
functional changes.

main-page-behavior now defers to settings-section for most of the
section-related animation work.

This is a similar interface to the one in https://codereview.chromium.org/2106013002/.

BUG=589681
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/405f956549f7853d58bf010844997910955880a5
Cr-Commit-Position: refs/heads/master@{#410172}

[michaelpg, 2016-08-05 01:59:16]

Description was changed from

==========
Move settings-section expand/collapse animations to settings-section

Adds functions on settings-section for the expand/collapse animations. These
functions just copied the code from the current main_page_behavior.js, with a
few necessary changes to return values and parameters. There should be NO
functional changes.

main-page-behavior now defers to settings-section for most of the
section-related animation work.

This is a similar interface to the one in
https://codereview.chromium.org/2106013002/.

BUG=589681
R=dbeam@chromium.org
==========

to

==========
Move settings-section expand/collapse animations to settings-section

Adds functions on settings-section for the expand/collapse animations. These
functions just copied the code from the current main_page_behavior.js, with a
few necessary changes to return values and parameters. There should be NO
functional changes.

main-page-behavior now defers to settings-section for most of the
section-related animation work.

This is a similar interface to the one in
https://codereview.chromium.org/2106013002/.

BUG=589681
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-08-05 02:09:11]

PTAL. This is a mostly mechanical change.

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

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:48:
freezeDimensions: function() {
The contents of the following functions were moved from main_page_behavior.js.
The animation logic is overhauled in my next patch, so the existing code doesn't
need to be re-reviewed here.

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:62:
card.origHeight_ = cardHeight;
I could take care of this TODO now with a property on <settings-section>, but
I'm reluctant to make more changes to the original code. LMK if you'd like that
in this CL.

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:231: *
@typedef {{
This typedef is temporary, the animations will eventually be called by
settings-section instead of MainPageBehavior.

[Dan Beam, 2016-08-05 04:14:38]

lgtm w/nits

I review a lot of code, so sorry if I'm thinking of the wrong reviews regarding
the consistency nits (I swear I saw you use Number.isNaN() and (thing -
otherThing) + 'px' in a different review?)

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

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:35:
this.scroller = this.domHost.parentNode./* presubmit */$.mainContainer;
scroller

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:114:
section.freezeDimensions();
setFrozen(true/false)?

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

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:9: 
 *

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:48:
freezeDimensions: function() {
does this need a

if (this.classList.contains('frozen'))
  return;

or

assert(!this.classList.contains('frozen'));

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:62:
card.origHeight_ = cardHeight;
On 2016/08/05 02:09:11, michaelpg wrote:
> I could take care of this TODO now with a property on <settings-section>, but
> I'm reluctant to make more changes to the original code. LMK if you'd like
that
> in this CL.

Acknowledged.

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:166:
animateCollapse: function(container, prevScrollTop) {
both animateCollapse and animateExpand are weird to me.

they both have side effects, but also return animation configs.  it'd be clearer
to separate if possible.

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:181: if
(isNaN(card.origHeight_))
nit: use isNaN or Number.isNan() consistently (I vote for isNaN because it's
shorter)

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:185:
this.style.height = this.clientHeight + card.origHeight_ + 'px';
nit: use parens with differently typed args that end up as a string consistently

i.e.

<num> + <num> + <string>

or

(<num> - <num>) + <string>

everywhere

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:231: *
@typedef {{
On 2016/08/05 02:09:11, michaelpg wrote:
> This typedef is temporary, the animations will eventually be called by
> settings-section instead of MainPageBehavior.

Acknowledged.

[michaelpg, 2016-08-05 05:16:08]

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

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:35:
this.scroller = this.domHost.parentNode./* presubmit */$.mainContainer;
On 2016/08/05 04:14:38, Dan Beam wrote:
> scroller

Done (also in a standalone CL)

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:114:
section.freezeDimensions();
On 2016/08/05 04:14:38, Dan Beam wrote:
> setFrozen(true/false)?

Done.

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

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:9: 
On 2016/08/05 04:14:38, Dan Beam wrote:
>  *

Done.

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:48:
freezeDimensions: function() {
On 2016/08/05 04:14:38, Dan Beam wrote:
> does this need a
> 
> if (this.classList.contains('frozen'))
>   return;
> 
> or
> 
> assert(!this.classList.contains('frozen'));

Probably? I'm reluctant to make changes like that. I know there are ways of
breaking navigation currently, so there are already incorrect logic/assumptions
somewhere, so I don't want to make functional changes that might make things
even worse. Since this will be rewritten and all.

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:166:
animateCollapse: function(container, prevScrollTop) {
On 2016/08/05 04:14:38, Dan Beam wrote:
> both animateCollapse and animateExpand are weird to me.
> 
> they both have side effects, but also return animation configs.  it'd be
clearer
> to separate if possible.

Acknowledged. I'll work on that in a followup CL.

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:181: if
(isNaN(card.origHeight_))
On 2016/08/05 04:14:38, Dan Beam wrote:
> nit: use isNaN or Number.isNan() consistently (I vote for isNaN because it's
> shorter)

Done (cut and pasted code)

I prefer Number.isNaN because it does what it says on the tin.

https://codereview.chromium.org/2215213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:185:
this.style.height = this.clientHeight + card.origHeight_ + 'px';
On 2016/08/05 04:14:38, Dan Beam wrote:
> nit: use parens with differently typed args that end up as a string
consistently
> 
> i.e.
> 
> <num> + <num> + <string>
> 
> or
> 
> (<num> - <num>) + <string>
> 
> everywhere

Done (cut and pasted code)

[michaelpg, 2016-08-05 20:51:36]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-08-05 20:51:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2215213002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-05 20:51:57]

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-08-05 21:46:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-05 21:48:18]

Description was changed from

==========
Move settings-section expand/collapse animations to settings-section

Adds functions on settings-section for the expand/collapse animations. These
functions just copied the code from the current main_page_behavior.js, with a
few necessary changes to return values and parameters. There should be NO
functional changes.

main-page-behavior now defers to settings-section for most of the
section-related animation work.

This is a similar interface to the one in
https://codereview.chromium.org/2106013002/.

BUG=589681
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Move settings-section expand/collapse animations to settings-section

Adds functions on settings-section for the expand/collapse animations. These
functions just copied the code from the current main_page_behavior.js, with a
few necessary changes to return values and parameters. There should be NO
functional changes.

main-page-behavior now defers to settings-section for most of the
section-related animation work.

This is a similar interface to the one in
https://codereview.chromium.org/2106013002/.

BUG=589681
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/405f956549f7853d58bf010844997910955880a5
Cr-Commit-Position: refs/heads/master@{#410172}
==========

[commit-bot: I haz the power, 2016-08-05 21:48:19]

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