MD Settings: add tests for mischievous default zoom handling

MD Settings: add tests for mischievous default zoom handling

BUG=663661
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/8da6512026adab2c303f0cfd0ea5d58b94a4fb12
Cr-Commit-Position: refs/heads/master@{#431187}

[Dan Beam, 2016-11-10 01:53:06]

Description was changed from

==========
MD Settings: add tests for mischevious default zoom handling

BUG=663661
R=dpapad@chromium.org
==========

to

==========
MD Settings: add tests for mischevious default zoom handling

BUG=663661
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-11-10 01:53:20]

Description was changed from

==========
MD Settings: add tests for mischevious default zoom handling

BUG=663661
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: add tests for mischievous default zoom handling

BUG=663661
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-11-10 01:55:03]

this depends on both:
https://codereview.chromium.org/2489723006/
https://codereview.chromium.org/2488113002/

which rietveld can't currently handle (AFAIK).  oh well.  when one lands i'll
add a dep on the remaining one (rietveld can handle 1 dep at a time).

[dpapad, 2016-11-10 02:38:34]

LGTM with nits. Thanks for adding tests!

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/appearance_page_test.js:25: defaultZoom_: 1,
Can me move this within the constructor instead? I know this also works because
we only have one TestAppearanceBrowserProxy at a time, but generally speaking
data members make more sense in the constructor.

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/appearance_page_test.js:82: appearancePage =
document.createElement('settings-appearance-page');
Nit(optional): Would "appearancePage.prefs = " also trigger Polymer observers?

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/appearance_page_test.js:208:
appearanceBrowserProxy.reset();
Perhaps move that call within createAppearancePage()? I can't see a scenario
where you would create a new page, but don't want to reset() the proxy.

[Dan Beam, 2016-11-10 02:52:38]

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/appearance_page_test.js:25: defaultZoom_: 1,
On 2016/11/10 02:38:34, dpapad wrote:
> Can me move this within the constructor instead? I know this also works
because
> we only have one TestAppearanceBrowserProxy at a time, but generally speaking
> data members make more sense in the constructor.

Done, but is this request based on a style rule or statistically motivated
precedent?

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/appearance_page_test.js:82: appearancePage =
document.createElement('settings-appearance-page');
On 2016/11/10 02:38:34, dpapad wrote:
> Nit(optional): Would "appearancePage.prefs = " also trigger Polymer observers?

there's a few things I could do to attempt to simulate the side effects we want
(set appearancePage.defaultZoom_ directly, for instance), but it felt like
recreating the page completely was closest to "refresh" that I knew of, and
that's the repro steps on the bug (and what i've been doing in local testing).

it also seems the least likely to need to update the tests if the implementation
changes.

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/appearance_page_test.js:208:
appearanceBrowserProxy.reset();
On 2016/11/10 02:38:33, dpapad wrote:
> Perhaps move that call within createAppearancePage()? I can't see a scenario
> where you would create a new page, but don't want to reset() the proxy.

Done.

[Dan Beam, 2016-11-10 03:54:09]

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

[commit-bot: I haz the power, 2016-11-10 03:54:27]

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

[commit-bot: I haz the power, 2016-11-10 04:09:44]

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

[commit-bot: I haz the power, 2016-11-10 04:09:44]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Dan Beam, 2016-11-10 04:27:07]

Patchset #3 (id:40001) has been deleted

[Dan Beam, 2016-11-10 04:27:28]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-11-10 04:27:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2487423002/#ps60001
(title: "closure")

[commit-bot: I haz the power, 2016-11-10 04:27:44]

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-11-10 05:16:49]

Description was changed from

==========
MD Settings: add tests for mischievous default zoom handling

BUG=663661
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: add tests for mischievous default zoom handling

BUG=663661
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-11-10 05:16:50]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-10 05:19:37]

Description was changed from

==========
MD Settings: add tests for mischievous default zoom handling

BUG=663661
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: add tests for mischievous default zoom handling

BUG=663661
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/8da6512026adab2c303f0cfd0ea5d58b94a4fb12
Cr-Commit-Position: refs/heads/master@{#431187}
==========

[commit-bot: I haz the power, 2016-11-10 05:19:38]

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

[dpapad, 2016-11-10 17:57:46]

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/appearance_page_test.js:25: defaultZoom_: 1,
On 2016/11/10 at 02:52:38, Dan Beam wrote:
> On 2016/11/10 02:38:34, dpapad wrote:
> > Can me move this within the constructor instead? I know this also works
because
> > we only have one TestAppearanceBrowserProxy at a time, but generally
speaking
> > data members make more sense in the constructor.
> 
> Done, but is this request based on a style rule or statistically motivated
precedent?

Both.
https://google.github.io/styleguide/javascriptguide.xml?showone=Method_and_pr...

[Dan Beam, 2016-11-10 18:54:36]

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2487423002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/appearance_page_test.js:25: defaultZoom_: 1,
On 2016/11/10 17:57:46, dpapad wrote:
> On 2016/11/10 at 02:52:38, Dan Beam wrote:
> > On 2016/11/10 02:38:34, dpapad wrote:
> > > Can me move this within the constructor instead? I know this also works
> because
> > > we only have one TestAppearanceBrowserProxy at a time, but generally
> speaking
> > > data members make more sense in the constructor.
> > 
> > Done, but is this request based on a style rule or statistically motivated
> precedent?
> 
> Both.
>
https://google.github.io/styleguide/javascriptguide.xml?showone=Method_and_pr...

interesting that the style guide calls this out explicitly.  i would say their
rationale doesn't actually apply in this case (neither method we're considering
changes shape) and is super implementation specific / has changed over the
years.

the most interesting bit, in my opinion, is how this actually works when you do:

  function Class() {}
  Class.prototype = {foo: 1};

  var c = new Class();  // c.foo == 1
  c.foo = 3;  // c.foo == 3

is actually fairly dirty, as if you do something like:

  // I'm done with |foo|, let's remove it!
  delete c.foo;  // delete returns true, c.foo == 1

as it's actually assigning a property on the instance, closer on the __proto__
chain.

tl;dr - I'm fine to do this.* = initialization in the ctor, but the statistics
question was a bit loaded: I know there's tons of webui that relies on
statically initializing prototype POD members :(.  I don't really want to change
all the existing code, but i'll keep it in mind.

thanks for the info!