MD Settings: Custom expand/collapse section animations.

MD Settings: Custom expand/collapse section animations.

Adds a behavior to expand and collapse a list of sections. Co-exists with the
sliding animations in settings-animated-pages for switching between main pages
and sub-pages, so there is a bit of hackery and several TODOs involved.

This fixes a couple problems:
* Width changing drastically while collapsing/expanding
* Duplicate scrollbar showing on top of original scrollbar when
  expanding/collapsing

Known issues to be addressed in follow-up patches:
* pressing forward after closing a section resizes to the new subpage before
  expanding
* starting an animation while another is in progress resizes to the new
  subpage dimensions before expanding

BUG=589681
Committed: https://crrev.com/e74d6b39b3e7b31ebdb451341ab923b3c8faed07
Cr-Commit-Position: refs/heads/master@{#381390}

[michaelpg, 2016-02-25 03:22:58]

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

[michaelpg, 2016-02-25 03:22:59]

PTAL. if this is too big, I can land the two behaviors first and follow up with
the functionality, but this way everything actually works together.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/compiled_resources.gyp:29:
'target_name': 'main_page_behavior',
Closure does not recognize the w3c web animations API, and
web-animations-next-lite.min.js is pretty far from being acceptable to closure.
Thoughts?

[Dan Beam, 2016-02-26 19:18:24]

lookin' pretty good!

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/compiled_resources.gyp:29:
'target_name': 'main_page_behavior',
On 2016/02/25 03:22:59, michaelpg wrote:
> Closure does not recognize the w3c web animations API, and
> web-animations-next-lite.min.js is pretty far from being acceptable to
closure.
> Thoughts?

@externs for the web animations API, maybe?

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:15: var
MainPageBehavior = [TransitionBehavior, {
up to you, but Polymer typically does:

var MainPageBehaviorImpl = {...};

var MainPageBehavior = [
  MainPageBehaviorImpl,
  TransitionBehavior
];

instead of what you're doing inlining the contents.  i'm fine wither either

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:16: /**
@type {string} Selector to get the sections. Elements must override. */
nit: s/Elements/Consumers/?  Downstream users?  Derived?  Behavior followers?

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:19: /**
@type {Element} The scrolling container. Elements must set this. */
nit: ?Element

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:46: if
(this.animations['section']) {
why ['section'] instead of .section?

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:180:
section.classList.add('frozen-section');
nit: why not just frozen?  expanding and expanded don't have a -section

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:232: var
promise = Promise.resolve();
nit: s/ var//

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:240:
}.bind(this), function() {
why is this second function here?  isn't it optional?

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:251: *
Collapses the cand in |section| back to its normal position.
cand -> card

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:269:
section.classList.remove('expanded');
nit: .remove('expanding', 'expanded')

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:274: if
(!card.origHeight_)
i think it might be safer to set this to an invalid number and verify it's that
invalid number (i.e. -1).  i could see how we might accidentally be using the
wrong object or setting the wrong property and this code would have a hard time
finding that out.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:328: var
section = this.$$('*[section=' + newRoute.section + ']');
why do you need the * before *[section?

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:342: var
section = this.$$('*[section=' + newRoute.section + ']');
maybe make this into a getSection_ method?  both of these query selector
construction calls

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_section.html
(left):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_section.html:14: <div
id="placeholder"></div>
huzzah

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/transition_behavior.js
(right):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/transition_behavior.js:13: *
@type {!Array<!Animation>}
wat, Object<!Animation>?

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/transition_behavior.js:41:
return new Promise(function(resolve, reject) {
return this.promises_[name] = new Promise(...);

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/transition_behavior.js:60:
finishAnimation: function(name, opt_callback) {
this.promises_[name].then(opt_callback);

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/transition_behavior.js:78: if
(opt_callback) {
this.promise_[name].catch(opt_callback);

[michaelpg, 2016-03-01 00:15:10]

Patchset #2 (id:20001) has been deleted

[michaelpg, 2016-03-01 00:15:17]

Patchset #2 (id:40001) has been deleted

[michaelpg, 2016-03-01 00:18:29]

ptal. lemme know about externs, but everything else *seems* to compile OK except
for the *Animation* lines

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/compiled_resources.gyp:29:
'target_name': 'main_page_behavior',
On 2016/02/26 19:18:23, Dan Beam wrote:
> On 2016/02/25 03:22:59, michaelpg wrote:
> > Closure does not recognize the w3c web animations API, and
> > web-animations-next-lite.min.js is pretty far from being acceptable to
> closure.
> > Thoughts?
> 
> @externs for the web animations API, maybe?

is there a tool for that?

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:15: var
MainPageBehavior = [TransitionBehavior, {
On 2016/02/26 19:18:23, Dan Beam wrote:
> up to you, but Polymer typically does:
> 
> var MainPageBehaviorImpl = {...};
> 
> var MainPageBehavior = [
>   MainPageBehaviorImpl,
>   TransitionBehavior
> ];
> 
> instead of what you're doing inlining the contents.  i'm fine wither either

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:16: /**
@type {string} Selector to get the sections. Elements must override. */
On 2016/02/26 19:18:23, Dan Beam wrote:
> nit: s/Elements/Consumers/?  Downstream users?  Derived?  Behavior followers?

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:19: /**
@type {Element} The scrolling container. Elements must set this. */
On 2016/02/26 19:18:23, Dan Beam wrote:
> nit: ?Element

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:46: if
(this.animations['section']) {
On 2016/02/26 19:18:23, Dan Beam wrote:
> why ['section'] instead of .section?

it's the name/key for the animation, not a property of this.animations

would using Map.{set, get, has} make more sense?

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:180:
section.classList.add('frozen-section');
On 2016/02/26 19:18:23, Dan Beam wrote:
> nit: why not just frozen?  expanding and expanded don't have a -section

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:232: var
promise = Promise.resolve();
On 2016/02/26 19:18:23, Dan Beam wrote:
> nit: s/ var//

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:240:
}.bind(this), function() {
On 2016/02/26 19:18:23, Dan Beam wrote:
> why is this second function here?  isn't it optional?

if the animation is cancelled, the promise is rejected (like it would be in the
spec: https://w3c.github.io/web-animations/#canceling-an-animation-section)

a rejected promise that isn't dealt with in a then(__, fn) or a catch(fn) throws
a console error

but canceling the animation is normal behavior, so we eat the rejected promise.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:251: *
Collapses the cand in |section| back to its normal position.
On 2016/02/26 19:18:23, Dan Beam wrote:
> cand -> card

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:269:
section.classList.remove('expanded');
On 2016/02/26 19:18:23, Dan Beam wrote:
> nit: .remove('expanding', 'expanded')

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:274: if
(!card.origHeight_)
On 2016/02/26 19:18:23, Dan Beam wrote:
> i think it might be safer to set this to an invalid number and verify it's
that
> invalid number (i.e. -1).  i could see how we might accidentally be using the
> wrong object or setting the wrong property and this code would have a hard
time
> finding that out.

What about... and I don't think I've ever used this before... NaN?

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:328: var
section = this.$$('*[section=' + newRoute.section + ']');
On 2016/02/26 19:18:23, Dan Beam wrote:
> why do you need the * before *[section?

Removed.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:342: var
section = this.$$('*[section=' + newRoute.section + ']');
On 2016/02/26 19:18:23, Dan Beam wrote:
> maybe make this into a getSection_ method?  both of these query selector
> construction calls

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_section.html
(left):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_section.html:14: <div
id="placeholder"></div>
On 2016/02/26 19:18:23, Dan Beam wrote:
> huzzah

Acknowledged.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/transition_behavior.js
(right):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/transition_behavior.js:13: *
@type {!Array<!Animation>}
On 2016/02/26 19:18:23, Dan Beam wrote:
> wat, Object<!Animation>?

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/transition_behavior.js:41:
return new Promise(function(resolve, reject) {
On 2016/02/26 19:18:23, Dan Beam wrote:
> return this.promises_[name] = new Promise(...);

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/transition_behavior.js:60:
finishAnimation: function(name, opt_callback) {
On 2016/02/26 19:18:24, Dan Beam wrote:
> this.promises_[name].then(opt_callback);

Done.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/transition_behavior.js:78: if
(opt_callback) {
On 2016/02/26 19:18:23, Dan Beam wrote:
> this.promise_[name].catch(opt_callback);

Done.

[Dan Beam, 2016-03-01 02:10:15]

will get to this tomororw

[Dan Beam, 2016-03-04 19:24:24]

lgtm, let's land this puppy and fix anything we find afterward

also +tommycli@ as FYI (speak now or... just fix it later :P)

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:46: if
(this.animations['section']) {
On 2016/03/01 00:18:28, michaelpg wrote:
> On 2016/02/26 19:18:23, Dan Beam wrote:
> > why ['section'] instead of .section?
> 
> it's the name/key for the animation, not a property of this.animations
> 
> would using Map.{set, get, has} make more sense?

Acknowledged.

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/transition_behavior.js
(right):

https://codereview.chromium.org/1736733003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/transition_behavior.js:41:
return new Promise(function(resolve, reject) {
On 2016/03/01 00:18:28, michaelpg wrote:
> On 2016/02/26 19:18:23, Dan Beam wrote:
> > return this.promises_[name] = new Promise(...);
> 
> Done.

thank you, i think the new code is much simpler

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/compiled_resources.gyp:29:
'target_name': 'main_page_behavior',
nit: alphabetize by target name (maybe?)

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

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:280: if
(Number.isNaN(card.origHeight_))
fwiw: isNaN is also on window, so this could just be isNaN() I think (unless
maybe it's required by the compiler?)

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/transition_behavior.js
(right):

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/transition_behavior.js:48:
return this.promises_[name] = new Promise(function(resolve, reject) {
this does work, I was just trying to be brief

doing assignment while returning the result is rather uncommon, i'd say.  this
is probably more common:

var promise = new Promise(...);
this.promises_[name] = promise;
return promise;

or

this.promises_[name] = new Promise(...);
return this.promises_[name];

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/transition_behavior.js:50:
this.animations[name] = undefined;
note: be care about using `in` with animations as these names are still
enumerable

[tommycli, 2016-03-04 19:34:16]

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

[tommycli, 2016-03-04 19:34:17]

No issues on my end. (Moar compiled_resources.gyp changes... nooo plz no)

[michaelpg, 2016-03-14 18:55:39]

Patchset #6 (id:140001) has been deleted

[michaelpg, 2016-03-14 18:57:44]

dbeam: ptal at externs, then we'll commit

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/compiled_resources.gyp:29:
'target_name': 'main_page_behavior',
On 2016/03/04 19:24:24, Dan Beam wrote:
> nit: alphabetize by target name (maybe?)

Done (in compiled_resources2.gyp)

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

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:280: if
(Number.isNaN(card.origHeight_))
On 2016/03/04 19:24:24, Dan Beam wrote:
> fwiw: isNaN is also on window, so this could just be isNaN() I think (unless
> maybe it's required by the compiler?)

Done.

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/transition_behavior.js
(right):

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/transition_behavior.js:48:
return this.promises_[name] = new Promise(function(resolve, reject) {
On 2016/03/04 19:24:24, Dan Beam wrote:
> this does work, I was just trying to be brief
> 
> doing assignment while returning the result is rather uncommon, i'd say.  this
> is probably more common:
> 
> var promise = new Promise(...);
> this.promises_[name] = promise;
> return promise;
> 
> or
> 
> this.promises_[name] = new Promise(...);
> return this.promises_[name];

Done.

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/transition_behavior.js:50:
this.animations[name] = undefined;
On 2016/03/04 19:24:24, Dan Beam wrote:
> note: be care about using `in` with animations as these names are still
> enumerable

Noted. There shouldn't be a need to.

[michaelpg, 2016-03-14 18:57:44]

dbeam: ptal at externs, then we'll commit

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/compiled_resources.gyp:29:
'target_name': 'main_page_behavior',
On 2016/03/04 19:24:24, Dan Beam wrote:
> nit: alphabetize by target name (maybe?)

Done (in compiled_resources2.gyp)

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

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:280: if
(Number.isNaN(card.origHeight_))
On 2016/03/04 19:24:24, Dan Beam wrote:
> fwiw: isNaN is also on window, so this could just be isNaN() I think (unless
> maybe it's required by the compiler?)

Done.

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/transition_behavior.js
(right):

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/transition_behavior.js:48:
return this.promises_[name] = new Promise(function(resolve, reject) {
On 2016/03/04 19:24:24, Dan Beam wrote:
> this does work, I was just trying to be brief
> 
> doing assignment while returning the result is rather uncommon, i'd say.  this
> is probably more common:
> 
> var promise = new Promise(...);
> this.promises_[name] = promise;
> return promise;
> 
> or
> 
> this.promises_[name] = new Promise(...);
> return this.promises_[name];

Done.

https://codereview.chromium.org/1736733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/transition_behavior.js:50:
this.animations[name] = undefined;
On 2016/03/04 19:24:24, Dan Beam wrote:
> note: be care about using `in` with animations as these names are still
> enumerable

Noted. There shouldn't be a need to.

[Dan Beam, 2016-03-14 21:27:53]

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
File third_party/closure_compiler/externs/web_animations.js (right):

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:9: * web-animations-js
polyfill for the time being.
can you add @see <link> for this file and any part that has an easily-accessible
link?

an example of this for the whole file would be

+ * @see https://w3c.github.io/web-animations/#programming-interface

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:14: */
nit: almost all of these short jsdoc comments can be /** one-liners */

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:57:
KeyframeEffectOptions.prototype.delay = 0;
one advantage of the .prototype.* route is that you don't have to initialize
each property (and probably shouldn't be):

  KeyframeEffectOptions.prototype.delay = 0;

would become just:

  KeyframeEffectOptions.prototype.delay;

with @type.

note: I have been asked by ewa@ and other externs reviewers to explicitly NOT
use sane default values in externs, as well as I know that all of this JS still
needs to be parsed (even though it's moved to a different AST), and adding these
values may slightly slow that down.

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:160: function
Animatable() {};
no ;

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:162: Animatable.prototype
= /** @lends {Element.prototype} */({
til: @lends is a thing

[michaelpg, 2016-03-15 00:04:52]

Patchset #8 (id:200001) has been deleted

[michaelpg, 2016-03-15 00:06:50]

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
File third_party/closure_compiler/externs/web_animations.js (right):

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:9: * web-animations-js
polyfill for the time being.
On 2016/03/14 21:27:53, Dan Beam wrote:
> can you add @see <link> for this file and any part that has an
easily-accessible
> link?
> 
> an example of this for the whole file would be
> 
> + * @see https://w3c.github.io/web-animations/#programming-interface

Done.

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:14: */
On 2016/03/14 21:27:53, Dan Beam wrote:
> nit: almost all of these short jsdoc comments can be /** one-liners */

Done.

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:57:
KeyframeEffectOptions.prototype.delay = 0;
On 2016/03/14 21:27:53, Dan Beam wrote:
> one advantage of the .prototype.* route is that you don't have to initialize
> each property (and probably shouldn't be):
> 
>   KeyframeEffectOptions.prototype.delay = 0;
> 
> would become just:
> 
>   KeyframeEffectOptions.prototype.delay;
> 
> with @type.
> 
> note: I have been asked by ewa@ and other externs reviewers to explicitly NOT
> use sane default values in externs, as well as I know that all of this JS
still
> needs to be parsed (even though it's moved to a different AST), and adding
these
> values may slightly slow that down.

Ok, but then why not extend this logic to functions?

    /** @constructor **
    var KeyframeEffectOptions;

    /** @type {function()} */
    Animation.prototype.finish;

etc.

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:160: function
Animatable() {};
On 2016/03/14 21:27:53, Dan Beam wrote:
> no ;

Done.

[michaelpg, 2016-03-16 00:01:59]

dbeam: slgty?

[Dan Beam, 2016-03-16 00:23:15]

still lgtm as long as you remove the closure compile fixes

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
File third_party/closure_compiler/externs/web_animations.js (right):

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:57:
KeyframeEffectOptions.prototype.delay = 0;
On 2016/03/15 00:06:50, michaelpg wrote:
> On 2016/03/14 21:27:53, Dan Beam wrote:
> > one advantage of the .prototype.* route is that you don't have to initialize
> > each property (and probably shouldn't be):
> > 
> >   KeyframeEffectOptions.prototype.delay = 0;
> > 
> > would become just:
> > 
> >   KeyframeEffectOptions.prototype.delay;
> > 
> > with @type.
> > 
> > note: I have been asked by ewa@ and other externs reviewers to explicitly
NOT
> > use sane default values in externs, as well as I know that all of this JS
> still
> > needs to be parsed (even though it's moved to a different AST), and adding
> these
> > values may slightly slow that down.
> 
> Ok, but then why not extend this logic to functions?
> 
>     /** @constructor **
>     var KeyframeEffectOptions;
> 
>     /** @type {function()} */
>     Animation.prototype.finish;
> 
> etc.

suppose that's OK, too, as long as you can name and describe @param's (can you?)

https://codereview.chromium.org/1736733003/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1736733003/diff/220001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:75: timestamp:
2,//this.timestamp,
wat

https://codereview.chromium.org/1736733003/diff/220001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:136: return
'blah';//this.item.dateRelativeDay;
wut

https://codereview.chromium.org/1736733003/diff/220001/third_party/closure_co...
File third_party/closure_compiler/externs/web_animations.js (right):

https://codereview.chromium.org/1736733003/diff/220001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:133: function
Animatable() {}
out of curiosity, why not var Animatable = function() {}; like others?

[michaelpg, 2016-03-16 03:11:15]

thanks, submitting now

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
File third_party/closure_compiler/externs/web_animations.js (right):

https://codereview.chromium.org/1736733003/diff/160001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:57:
KeyframeEffectOptions.prototype.delay = 0;
On 2016/03/16 00:23:14, Dan Beam wrote:
> On 2016/03/15 00:06:50, michaelpg wrote:
> > On 2016/03/14 21:27:53, Dan Beam wrote:
> > > one advantage of the .prototype.* route is that you don't have to
initialize
> > > each property (and probably shouldn't be):
> > > 
> > >   KeyframeEffectOptions.prototype.delay = 0;
> > > 
> > > would become just:
> > > 
> > >   KeyframeEffectOptions.prototype.delay;
> > > 
> > > with @type.
> > > 
> > > note: I have been asked by ewa@ and other externs reviewers to explicitly
> NOT
> > > use sane default values in externs, as well as I know that all of this JS
> > still
> > > needs to be parsed (even though it's moved to a different AST), and adding
> > these
> > > values may slightly slow that down.
> > 
> > Ok, but then why not extend this logic to functions?
> > 
> >     /** @constructor **
> >     var KeyframeEffectOptions;
> > 
> >     /** @type {function()} */
> >     Animation.prototype.finish;
> > 
> > etc.
> 
> suppose that's OK, too, as long as you can name and describe @param's (can
you?)

Yeah it seems to work just the same. I'll keep it the "usual" way though.

https://codereview.chromium.org/1736733003/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1736733003/diff/220001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:75: timestamp:
2,//this.timestamp,
On 2016/03/16 00:23:14, Dan Beam wrote:
> wat

lol oops

https://codereview.chromium.org/1736733003/diff/220001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:136: return
'blah';//this.item.dateRelativeDay;
On 2016/03/16 00:23:14, Dan Beam wrote:
> wut

Done.

https://codereview.chromium.org/1736733003/diff/220001/third_party/closure_co...
File third_party/closure_compiler/externs/web_animations.js (right):

https://codereview.chromium.org/1736733003/diff/220001/third_party/closure_co...
third_party/closure_compiler/externs/web_animations.js:133: function
Animatable() {}
On 2016/03/16 00:23:14, Dan Beam wrote:
> out of curiosity, why not var Animatable = function() {}; like others?

dunno, switched it to that.

[michaelpg, 2016-03-16 03:12:06]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-03-16 03:12:06]

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/1736733003/#ps240001
(title: "function/rebase")

[commit-bot: I haz the power, 2016-03-16 03:12:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736733003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736733003/240001

[commit-bot: I haz the power, 2016-03-16 05:21:01]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2016-03-16 05:22:46]

Description was changed from

==========
MD Settings: Custom expand/collapse section animations.

Adds a behavior to expand and collapse a list of sections. Co-exists with the
sliding animations in settings-animated-pages for switching between main pages
and sub-pages, so there is a bit of hackery and several TODOs involved.

This fixes a couple problems:
* Width changing drastically while collapsing/expanding
* Duplicate scrollbar showing on top of original scrollbar when
  expanding/collapsing

Known issues to be addressed in follow-up patches:
* pressing forward after closing a section resizes to the new subpage before
  expanding
* starting an animation while another is in progress resizes to the new
  subpage dimensions before expanding

BUG=589681
==========

to

==========
MD Settings: Custom expand/collapse section animations.

Adds a behavior to expand and collapse a list of sections. Co-exists with the
sliding animations in settings-animated-pages for switching between main pages
and sub-pages, so there is a bit of hackery and several TODOs involved.

This fixes a couple problems:
* Width changing drastically while collapsing/expanding
* Duplicate scrollbar showing on top of original scrollbar when
  expanding/collapsing

Known issues to be addressed in follow-up patches:
* pressing forward after closing a section resizes to the new subpage before
  expanding
* starting an animation while another is in progress resizes to the new
  subpage dimensions before expanding

BUG=589681

Committed: https://crrev.com/e74d6b39b3e7b31ebdb451341ab923b3c8faed07
Cr-Commit-Position: refs/heads/master@{#381390}
==========

[commit-bot: I haz the power, 2016-03-16 05:22:47]

Patchset 9 (id:??) landed as
https://crrev.com/e74d6b39b3e7b31ebdb451341ab923b3c8faed07
Cr-Commit-Position: refs/heads/master@{#381390}