MD Settings: fix auto-focus on sync page.

MD Settings: fix auto-focus on sync page.

BUG=730954
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2941433002
Cr-Commit-Position: refs/heads/master@{#479480}
Committed: https://chromium.googlesource.com/chromium/src/+/690256f7a5cfb0fe8e61cf19c8e026f3ac11af88

[scottchen, 2017-06-13 01:21:13]

Description was changed from

==========
MD Settings: fix auto-focus on sync page.

BUG=730954
==========

to

==========
MD Settings: fix auto-focus on sync page.

BUG=730954
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[scottchen, 2017-06-13 01:21:56]

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

[dpapad, 2017-06-13 01:27:34]

Is there an easy way to test this? Either way, LGTM with nits (I expect testing
focus() related issues to not be trivial).

https://codereview.chromium.org/2941433002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/sync_page.js (right):

https://codereview.chromium.org/2941433002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/sync_page.js:219: // wait for the
dom-if templates to render and subpage to become visible.
s/wait/Wait

https://codereview.chromium.org/2941433002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/sync_page.js:228: 
Remove new blank lines?

[scottchen, 2017-06-14 00:51:54]

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

[scottchen, 2017-06-14 00:52:13]

Added test, surprisingly simple. PTAL

https://codereview.chromium.org/2941433002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/sync_page.js (right):

https://codereview.chromium.org/2941433002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/sync_page.js:219: // wait for the
dom-if templates to render and subpage to become visible.
On 2017/06/13 01:27:34, dpapad wrote:
> s/wait/Wait

Done.

https://codereview.chromium.org/2941433002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/sync_page.js:228: 
On 2017/06/13 01:27:34, dpapad wrote:
> Remove new blank lines?

Done.

[commit-bot: I haz the power, 2017-06-14 00:52:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-06-14 01:02:00]

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

https://codereview.chromium.org/2941433002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_interactive_ui_tests.js:77:
__proto__: CrSettingsInteractiveUITest.prototype,
Does the new test have to be an interactive UI test? Asking because we already
have tests for sync_page.html, at
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/cr_setti...

It is currently a bit confusing that we have two files that test the same
element,

people_page_sync_page_test.js (existing)
settings_sync_page_test.js (new in this CL)

So I suggest to either
1) Run your new test as a normal test and move the test in the existing file.
2) If 1 not possible, maybe still move the test in the existing file, and use
mocha.grep() at line 80 to only run your new test case as an interactive ui
test.
3) If not 1 or 2, rename the files to be more consistent/descriptive?

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

https://codereview.chromium.org/2941433002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_sync_page_test.js:26:
testElement.fire('show-container');
Nit: Maybe add a short comment 

// Simulate event normally fired by ...

[commit-bot: I haz the power, 2017-06-14 02:21:22]

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

[commit-bot: I haz the power, 2017-06-14 02:21:23]

Dry run: This issue passed the CQ dry run.

[scottchen, 2017-06-14 18:14:58]

On 2017/06/14 01:02:00, dpapad wrote:
>
https://codereview.chromium.org/2941433002/diff/20001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/cr_settings_interactive_ui_tests.js
> (right):
> 
>
https://codereview.chromium.org/2941433002/diff/20001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/cr_settings_interactive_ui_tests.js:77:
> __proto__: CrSettingsInteractiveUITest.prototype,
> Does the new test have to be an interactive UI test? Asking because we already
> have tests for sync_page.html, at
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/cr_setti...
> 
> It is currently a bit confusing that we have two files that test the same
> element,
> 
> people_page_sync_page_test.js (existing)
> settings_sync_page_test.js (new in this CL)
> 
> So I suggest to either
> 1) Run your new test as a normal test and move the test in the existing file.
> 2) If 1 not possible, maybe still move the test in the existing file, and use
> mocha.grep() at line 80 to only run your new test case as an interactive ui
> test.
> 3) If not 1 or 2, rename the files to be more consistent/descriptive?
> 
>
https://codereview.chromium.org/2941433002/diff/20001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/settings_sync_page_test.js (right):
> 
>
https://codereview.chromium.org/2941433002/diff/20001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/settings_sync_page_test.js:26:
> testElement.fire('show-container');
> Nit: Maybe add a short comment 
> 
> // Simulate event normally fired by ...

Tried 1, doesn't work as a normal browser test. I thought 2 would be more
confusing because then parts of the file would be ran as normal browser test and
parts would be interactive test, so I'm gonna rename the file.

[scottchen, 2017-06-14 18:24:33]

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

[commit-bot: I haz the power, 2017-06-14 18:25:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-06-14 18:31:35]

Still LGTM

[scottchen, 2017-06-14 18:32:44]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-06-14 18:33:31]

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, 2017-06-14 20:05:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497465164568680,
"parent_rev": "e9c7c14f31fdbc695f94f2b574067cc5616cc476", "commit_rev":
"690256f7a5cfb0fe8e61cf19c8e026f3ac11af88"}

[commit-bot: I haz the power, 2017-06-14 20:05:34]

Description was changed from

==========
MD Settings: fix auto-focus on sync page.

BUG=730954
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: fix auto-focus on sync page.

BUG=730954
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2941433002
Cr-Commit-Position: refs/heads/master@{#479480}
Committed:
https://chromium.googlesource.com/chromium/src/+/690256f7a5cfb0fe8e61cf19c8e0...
==========

[commit-bot: I haz the power, 2017-06-14 20:05:38]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/690256f7a5cfb0fe8e61cf19c8e0...