[ServiceWorker] Tests for WindowClient.focus().

[ServiceWorker] Tests for WindowClient.focus().

This test has to live in the chromium/ directory because it uses testRunner.

BUG=447212
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190433

[mlamouri (slow - plz ping), 2015-02-06 12:04:34]

mlamouri@chromium.org changed reviewers:
+ michaeln@chromium.org

[mlamouri (slow - plz ping), 2015-02-16 21:17:11]

mlamouri@chromium.org changed reviewers:
+ dominicc@chromium.org
- michaeln@chromium.org

[mlamouri (slow - plz ping), 2015-02-16 21:17:12]

Dominic, PTAL?

[mlamouri (slow - plz ping), 2015-02-18 00:43:16]

mlamouri@chromium.org changed reviewers:
+ michaeln@chromium.org, nhiroki@chromium.org

[mlamouri (slow - plz ping), 2015-02-18 00:43:17]

+nhiroki@, +michaeln@

Could one of you have a look? :)

[Michael van Ouwerkerk, 2015-02-18 10:55:46]

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

[Michael van Ouwerkerk, 2015-02-18 10:55:48]

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.html
(right):

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.html:7:
<div>Foo</div>
Omit this?

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js
(right):

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:1:
// This helper will setup a small test framework that will use TESTS and run
Please rename this file to windowclient-focus-worker.js to make more clear this
is a service worker script. It would be consistent with the other code in this
directory.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:2:
// them iteratively and call self.postMessage('quit') when done.
Iteratively, sure. Sequentially though?

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:3:
// This helper also exposes |client|, |postMessage()|, |runNextTestOrQuit()|,
This helper does not expose runNextTestOrQuit, it is in sw-test-helpers.js. Same
for postMessage. This helper assigns but does not declare |client|.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:7:
var nestedClients = [];
Please document this global.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:9:
// Override self.initiaze() from sw-test-helpers.js
initialize

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:15:
// It's url ends with '.html' while the others ends with '.html?X'
Its

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:16:
if (c.url.search(/\.html\?[0-9]/) != -1)
I think this is more readable by avoiding a regex:
if (c.url.endsWith('.html') {
  client = c
} else {
  nestedClients.push(c);
}

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:19:
client = c;
Is this not declared as a global explicitly? Please do, and document it. Ugh
you're setting a global from an imported script. Ok. Please document that, it's
hard to read.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:22:
}
semicolon

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:27:
clients.forEach(function(c) {
Please avoid single character variable names unless it is a loop counter or an
error. This could be |client|. Same below.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:54:
// There should be 1 focused client at that point.
s/that/this/g

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:60:
function testFocusNestedFrames() {
Maybe rename to |testFocusNestedFrame| as you expect to focus only one, right?
Same for the next test.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:61:
synthesizeNotificationClick().then(function(e) {
What is e?

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:71:
// There should be 2 focused client at that point.
clients

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:88:
// There should be 2 focused client at that point.
Why? Presumably because focusing this frame blurred the other, but it would be
nice to make that explicit.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:117:
if (e.data == "start") {
single quotes for js string literals

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:125:
}
semicolon

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/chromium/windowclient-focus.html
(right):

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/windowclient-focus.html:34: ' url:
' + location.origin +
'/serviceworker/chromium/resources/windowclient-focus.html',
No need to indent this much, this line is over 100 now. The members of the
|expected| array can just be 4 spaces like this:
var expected = [
....'foo'
];

[mlamouri (slow - plz ping), 2015-02-18 11:34:56]

PTAL

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.html
(right):

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.html:7:
<div>Foo</div>
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Omit this?

It's good for local debugging. You don't get a blank iframe so you know you got
something.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.html:7:
<div>Foo</div>
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Omit this?

It's good for local debugging. You don't get a blank iframe so you know you got
something.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js
(right):

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:1:
// This helper will setup a small test framework that will use TESTS and run
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Please rename this file to windowclient-focus-worker.js to make more clear
this is a service worker script. It would be consistent with the other code in
this directory.

It would be inconsistent with:
- notificationclick-can-focus.js
- notificationclick-can-openwindow.js
- client-openwindow.js

Let's keep this consistent with those given that they are the same kind of
tests.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:2:
// them iteratively and call self.postMessage('quit') when done.
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Iteratively, sure. Sequentially though?

Done. Change the other files.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:3:
// This helper also exposes |client|, |postMessage()|, |runNextTestOrQuit()|,
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> This helper does not expose runNextTestOrQuit, it is in sw-test-helpers.js.
Same for postMessage. This helper assigns but does not declare |client|.

sw-test-helpers.js is the helper.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:7:
var nestedClients = [];
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Please document this global.

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:9:
// Override self.initiaze() from sw-test-helpers.js
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> initialize

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:15:
// It's url ends with '.html' while the others ends with '.html?X'
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Its

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:16:
if (c.url.search(/\.html\?[0-9]/) != -1)
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> I think this is more readable by avoiding a regex:
> if (c.url.endsWith('.html') {
>   client = c
> } else {
>   nestedClients.push(c);
> }

I would have bet I tried to write some code in Chrome Beta and endsWith wasn't
there yet.

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:19:
client = c;
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Is this not declared as a global explicitly? Please do, and document it. Ugh
you're setting a global from an imported script. Ok. Please document that, it's
hard to read.

It's documented in the first block of comment.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:22:
}
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> semicolon

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:27:
clients.forEach(function(c) {
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Please avoid single character variable names unless it is a loop counter or an
error. This could be |client|. Same below.

I do not use |client| on purpose to prevent confusion with the imported
|client|.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:54:
// There should be 1 focused client at that point.
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> s/that/this/g

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:60:
function testFocusNestedFrames() {
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Maybe rename to |testFocusNestedFrame| as you expect to focus only one, right?
Same for the next test.

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:61:
synthesizeNotificationClick().then(function(e) {
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> What is e?

event. Not used.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:71:
// There should be 2 focused client at that point.
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> clients

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:88:
// There should be 2 focused client at that point.
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> Why? Presumably because focusing this frame blurred the other, but it would be
nice to make that explicit.

Add a comment to explain.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:117:
if (e.data == "start") {
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> single quotes for js string literals

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.js:125:
}
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> semicolon

Done.

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/chromium/windowclient-focus.html
(right):

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/windowclient-focus.html:34: ' url:
' + location.origin +
'/serviceworker/chromium/resources/windowclient-focus.html',
On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> No need to indent this much, this line is over 100 now. The members of the
|expected| array can just be 4 spaces like this:
> var expected = [
> ....'foo'
> ];

Done.

[Michael van Ouwerkerk, 2015-02-18 12:39:03]

lgtm if you fix the remaining issue

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.html
(right):

https://codereview.chromium.org/866043005/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-focus.html:7:
<div>Foo</div>
On 2015/02/18 11:34:55, Mounir Lamouri wrote:
> On 2015/02/18 at 10:55:47, Michael van Ouwerkerk wrote:
> > Omit this?
> 
> It's good for local debugging. You don't get a blank iframe so you know you
got
> something.

Ok, so then make it say something meaningful. In its current state this only
makes sense to you, and you've already asked 4 people to look at this.

[mlamouri (slow - plz ping), 2015-02-18 12:43:00]

New patchsets have been uploaded after l-g-t-m from mvanouwerkerk@chromium.org

[mlamouri (slow - plz ping), 2015-02-18 12:43:05]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-02-18 12:43:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/866043005/100001

[commit-bot: I haz the power, 2015-02-18 15:15:26]

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

[commit-bot: I haz the power, 2015-02-18 15:15:28]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/4...)

[mlamouri (slow - plz ping), 2015-02-18 15:18:22]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-02-18 15:19:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/866043005/100001

[commit-bot: I haz the power, 2015-02-18 16:30:44]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190433