Settings People Revamp: Simplify Change Picture browsertests.

Settings People Revamp: Simplify Change Picture browsertests.

The old version used the real objects, was slow, and a bit flaky.

This new version uses the TestBrowserProxy pattern established by
dpapad, so hopefully this solves the flakiness and unblocks the Polymer
roll.

BUG=563721
Committed: https://crrev.com/0b46662789c37358cf4f9d8fb7f4e03506dfec81
Cr-Commit-Position: refs/heads/master@{#386546}

[tommycli, 2016-04-08 23:56:55]

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

[commit-bot: I haz the power, 2016-04-08 23:57:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1871073004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1871073004/20001

[tommycli, 2016-04-08 23:57:51]

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

[tommycli, 2016-04-08 23:57:52]

dpapad: PTAL. This change is to simplify these tests and unblock your Polymer
roll.

There's no urgency / this can easily wait for next week. Cheers.

[commit-bot: I haz the power, 2016-04-08 23:59:29]

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

[commit-bot: I haz the power, 2016-04-08 23:59:30]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[dpapad, 2016-04-09 00:16:35]

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:50:
selectedItem_: {
Shouldn't this be

 /**
  * @private {settings.ChangePictureImageElement}
  */
 selectedItem_: Object,


I don't think you can use the typedef name as the "type" value in a Polymer
property, only Boolean, Date, Number, String, Array or Object is accepted.

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:91: type:
settings.ChangePictureImageElement,
Same here.

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:355: return
cameraPresent && selectedItem != null &&
Nit (optional): Use "!==" instead of "!=".

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

https://codereview.chromium.org/1871073004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_change_picture_test.js:104:
assertTrue(!!discardControlBar);
Could we move the following which is repeated at the beginning of every test
here?

return browserProxy.whenCalled('initialize').then(function() {
Polymer.dom.flush(); });

https://codereview.chromium.org/1871073004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_change_picture_test.js:134:
expectEquals('camera', changePicture.selectedItem_.dataset.type);
Is there a way we can achieve the same assertion without referencing a private
var?

If not could you at least add a type annotation for "changePicture" at line 83?

[tommycli, 2016-04-11 19:09:58]

dpapad: Done, thanks!

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:50:
selectedItem_: {
On 2016/04/09 00:16:35, dpapad wrote:
> Shouldn't this be
> 
>  /**
>   * @private {settings.ChangePictureImageElement}
>   */
>  selectedItem_: Object,
> 
> 
> I don't think you can use the typedef name as the "type" value in a Polymer
> property, only Boolean, Date, Number, String, Array or Object is accepted.

Done. Yes I think you are correct. I wonder what is actually happening when I do
this.

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:91: type:
settings.ChangePictureImageElement,
On 2016/04/09 00:16:35, dpapad wrote:
> Same here.

Done.

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:355: return
cameraPresent && selectedItem != null &&
On 2016/04/09 00:16:35, dpapad wrote:
> Nit (optional): Use "!==" instead of "!=".

This didn't work for me because even with the value: null, the value is
momentarily undefined when the page is first loaded. Not sure why.

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

https://codereview.chromium.org/1871073004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_change_picture_test.js:104:
assertTrue(!!discardControlBar);
On 2016/04/09 00:16:35, dpapad wrote:
> Could we move the following which is repeated at the beginning of every test
> here?
> 
> return browserProxy.whenCalled('initialize').then(function() {
> Polymer.dom.flush(); });

Done. I did not know that was even an option. The diff looks messy but i Just
unindented everything and added the getSelectedItem() method.

https://codereview.chromium.org/1871073004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_change_picture_test.js:134:
expectEquals('camera', changePicture.selectedItem_.dataset.type);
On 2016/04/09 00:16:35, dpapad wrote:
> Is there a way we can achieve the same assertion without referencing a private
> var?
> 
> If not could you at least add a type annotation for "changePicture" at line
83?

Done.

[dpapad, 2016-04-11 19:36:39]

LGTM

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:355: return
cameraPresent && selectedItem != null &&
On 2016/04/11 at 19:09:57, tommycli wrote:
> On 2016/04/09 00:16:35, dpapad wrote:
> > Nit (optional): Use "!==" instead of "!=".
> 
> This didn't work for me because even with the value: null, the value is
momentarily undefined when the page is first loaded. Not sure why.

That is odd. Polymer data bindings guarantee that this function will not be
called until all dependencies (cameraPresent and selectedItem) are set to a
non-undefined value, pasting below from the docs:

"The computing function is not called until all dependent properties are defined
(!=undefined)."

I am fine with leaving as is, but it would be nice to understand why does the
behavior not match the Polymer docs. I have definitely seen the advertized
behavior working in other cases.

https://codereview.chromium.org/1871073004/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_change_picture_test.js (right):

https://codereview.chromium.org/1871073004/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_change_picture_test.js:170:
expectEquals('old', getSelectedItem().dataset.type);
Nit: Could we create an enum to hold the possible values of dataset.type ('old',
'profile', 'default' etc). Those string literals are repeated in various places
in the code (in the proxy, in change_picture.js and here).

In a separate CL is fine, I just think it would make the code more readable,
since reader will have an easier time figuring out where those literals come
from.

[tommycli, 2016-04-11 21:20:52]

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:355: return
cameraPresent && selectedItem != null &&
On 2016/04/11 19:36:38, dpapad wrote:
> On 2016/04/11 at 19:09:57, tommycli wrote:
> > On 2016/04/09 00:16:35, dpapad wrote:
> > > Nit (optional): Use "!==" instead of "!=".
> > 
> > This didn't work for me because even with the value: null, the value is
> momentarily undefined when the page is first loaded. Not sure why.
> 
> That is odd. Polymer data bindings guarantee that this function will not be
> called until all dependencies (cameraPresent and selectedItem) are set to a
> non-undefined value, pasting below from the docs:
> 
> "The computing function is not called until all dependent properties are
defined
> (!=undefined)."
> 
> I am fine with leaving as is, but it would be nice to understand why does the
> behavior not match the Polymer docs. I have definitely seen the advertized
> behavior working in other cases.

I investigated this in detail. Basically: when iron-selector selects a different
item, the selectedItem momentarily becomes undefined, and then it becomes
defined again (to the new item).

I filed an issue: https://github.com/PolymerElements/iron-selector/issues/118

In the meantime i will leave these computed bindings as 'falsy'.

https://codereview.chromium.org/1871073004/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_change_picture_test.js (right):

https://codereview.chromium.org/1871073004/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_change_picture_test.js:170:
expectEquals('old', getSelectedItem().dataset.type);
On 2016/04/11 19:36:38, dpapad wrote:
> Nit: Could we create an enum to hold the possible values of dataset.type
('old',
> 'profile', 'default' etc). Those string literals are repeated in various
places
> in the code (in the proxy, in change_picture.js and here).
> 
> In a separate CL is fine, I just think it would make the code more readable,
> since reader will have an easier time figuring out where those literals come
> from.

Will do (in separate CL)

[tommycli, 2016-04-11 21:21:05]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-04-11 21:21:06]

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/1871073004/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-04-11 21:22:01]

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

[dpapad, 2016-04-11 21:29:34]

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1871073004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:355: return
cameraPresent && selectedItem != null &&
On 2016/04/11 at 21:20:52, tommycli wrote:
> On 2016/04/11 19:36:38, dpapad wrote:
> > On 2016/04/11 at 19:09:57, tommycli wrote:
> > > On 2016/04/09 00:16:35, dpapad wrote:
> > > > Nit (optional): Use "!==" instead of "!=".
> > > 
> > > This didn't work for me because even with the value: null, the value is
> > momentarily undefined when the page is first loaded. Not sure why.
> > 
> > That is odd. Polymer data bindings guarantee that this function will not be
> > called until all dependencies (cameraPresent and selectedItem) are set to a
> > non-undefined value, pasting below from the docs:
> > 
> > "The computing function is not called until all dependent properties are
defined
> > (!=undefined)."
> > 
> > I am fine with leaving as is, but it would be nice to understand why does
the
> > behavior not match the Polymer docs. I have definitely seen the advertized
> > behavior working in other cases.
> 
> I investigated this in detail. Basically: when iron-selector selects a
different item, the selectedItem momentarily becomes undefined, and then it
becomes defined again (to the new item).
> 
> I filed an issue: https://github.com/PolymerElements/iron-selector/issues/118
> 
> In the meantime i will leave these computed bindings as 'falsy'.

Thanks for investigating!

[commit-bot: I haz the power, 2016-04-11 22:28:53]

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

[commit-bot: I haz the power, 2016-04-11 22:28:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-04-12 00:02:10]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-04-12 00:02:10]

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/1871073004/#ps80001
(title: "fix hitting the check (and merge origin/master)")

[commit-bot: I haz the power, 2016-04-12 00:03:02]

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

[commit-bot: I haz the power, 2016-04-12 01:47:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-12 01:48:50]

Description was changed from

==========
Settings People Revamp: Simplify Change Picture browsertests.

The old version used the real objects, was slow, and a bit flaky.

This new version uses the TestBrowserProxy pattern established by
dpapad, so hopefully this solves the flakiness and unblocks the Polymer
roll.

BUG=563721
==========

to

==========
Settings People Revamp: Simplify Change Picture browsertests.

The old version used the real objects, was slow, and a bit flaky.

This new version uses the TestBrowserProxy pattern established by
dpapad, so hopefully this solves the flakiness and unblocks the Polymer
roll.

BUG=563721

Committed: https://crrev.com/0b46662789c37358cf4f9d8fb7f4e03506dfec81
Cr-Commit-Position: refs/heads/master@{#386546}
==========

[commit-bot: I haz the power, 2016-04-12 01:48:52]

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