Tests for WaitUntilObserver and focus/openining windows.

Tests for WaitUntilObserver and focus/openining windows.

Those are LayoutTests opening and focusing windows while simulating 
notificationclicks. These tests are only meant to test how WaitUntilObserver
handles the permission, they do not really test the feature.

The CL is also doing a slight fix to the implementation to allow focusing
or creating window on the notificationclick handler stack.

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

[mlamouri (slow - plz ping), 2015-02-05 18:49:14]

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

[dominicc (has gone to gerrit), 2015-02-05 18:58:13]

Some comments inline; I have not looked at the tests yet.

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/WaitUntilObserver.cpp (right):

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/WaitUntilObserver.cpp:29: fprintf(stderr,
"%s%d\n", "windowInteractionTimeout = ", windowInteractionTimeout);
Hmm, what happens if unit tests run in parallel?

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/WaitUntilObserver.cpp:173: fprintf(stderr, "%s\n",
"consumeWindowInteraction");
Remove the logging.

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/testing/InternalsServiceWorker.h (right):

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/testing/InternalsServiceWorker.h:16: static void
overrideWindowInteractionTimeout(Internals&, unsigned);
The second parameter probably needs a name because 'unsigned' could be pretty
much anything. And/or you could introduce a typedef to name the type.

https://codereview.chromium.org/896043004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/WaitUntilObserver.cpp (right):

https://codereview.chromium.org/896043004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/WaitUntilObserver.cpp:89:
executionContext()->allowWindowInteraction();
Does it make sense to assert that this has been reset by the time the object is
destructed (or at some other/more points?)

[mlamouri (slow - plz ping), 2015-02-05 20:23:13]

Review comments applied. PTAL.

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/WaitUntilObserver.cpp (right):

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/WaitUntilObserver.cpp:29: fprintf(stderr,
"%s%d\n", "windowInteractionTimeout = ", windowInteractionTimeout);
On 2015/02/05 at 18:58:13, dominicc wrote:
> Hmm, what happens if unit tests run in parallel?

Yeah... So I moved to another model. There is now another
kWindowInteractionTimeout (ForTest) in SWGlobalScope and WaitUntilObserver is
using it when running in LayoutTests.

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/WaitUntilObserver.cpp:173: fprintf(stderr, "%s\n",
"consumeWindowInteraction");
On 2015/02/05 at 18:58:13, dominicc wrote:
> Remove the logging.

Removed in patch #2.

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/testing/InternalsServiceWorker.h (right):

https://codereview.chromium.org/896043004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/testing/InternalsServiceWorker.h:16: static void
overrideWindowInteractionTimeout(Internals&, unsigned);
On 2015/02/05 at 18:58:13, dominicc wrote:
> The second parameter probably needs a name because 'unsigned' could be pretty
much anything. And/or you could introduce a typedef to name the type.

Done.

https://codereview.chromium.org/896043004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/WaitUntilObserver.cpp (right):

https://codereview.chromium.org/896043004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/WaitUntilObserver.cpp:89:
executionContext()->allowWindowInteraction();
On 2015/02/05 at 18:58:13, dominicc wrote:
> Does it make sense to assert that this has been reset by the time the object
is destructed (or at some other/more points?)

Not really. The callers are not aware of the number of tokens the
ExecutionContext currently have. There might be multiple WaitUntilObserver
running at the same time in which case after one dies, it will still be allowed
to open/focus a window. Because of that, there isn't much sanity check we can
do.

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

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

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

+michaeln@, if you can have a look at that, it would be great. dominicc@ will
unlikely be able to look at this CL until next week.

[dominicc (has gone to gerrit), 2015-02-09 07:00:58]

Some comments inline.

https://codereview.chromium.org/896043004/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/notificationclick-can-focus.html
(right):

https://codereview.chromium.org/896043004/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/notificationclick-can-focus.html:1:
<!DOCTYPE html>
Should these tests be in ServiceWorker? Push Notifications seems significant
enough to have its own folder. Chatted with falken@ and his gut feeling is
Push/Notifications tests should go somewhere else. Maybe
http/tests/push_messaging?

https://codereview.chromium.org/896043004/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/notificationclick-can-focus.html:7: if
(window.testRunner)
If these tests are in Service Worker, tests that use Chromium-specific hooks
like this should be in chromium/ so we don't try to upstream them to W3C.

[dominicc (has gone to gerrit), 2015-02-09 07:03:10]

https://codereview.chromium.org/896043004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/896043004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:98: static const
unsigned kWindowInteractionTimeout = 10;
These could move into the implementation file since the logic to flip between
them is only accessible there anyway.

https://codereview.chromium.org/896043004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/testing/InternalsServiceWorker.cpp (right):

https://codereview.chromium.org/896043004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/testing/InternalsServiceWorker.cpp:9: #include
"modules/serviceworkers/WaitUntilObserver.h"
Maybe this include is no longer needed?

[Michael van Ouwerkerk, 2015-02-09 10:44:24]

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

[Michael van Ouwerkerk, 2015-02-09 10:44:24]

https://codereview.chromium.org/896043004/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/notificationclick-can-focus.html
(right):

https://codereview.chromium.org/896043004/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/notificationclick-can-focus.html:1:
<!DOCTYPE html>
On 2015/02/09 07:00:58, dominicc wrote:
> Should these tests be in ServiceWorker? Push Notifications seems significant
> enough to have its own folder. Chatted with falken@ and his gut feeling is
> Push/Notifications tests should go somewhere else. Maybe
> http/tests/push_messaging?

These tests don't appear to exercise the push part of push notifications, so
just use third_party/WebKit/LayoutTests/http/tests/notifications/

[mlamouri (slow - plz ping), 2015-02-11 10:42:06]

Comments applied. PTAL.

https://codereview.chromium.org/896043004/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/notificationclick-can-focus.html
(right):

https://codereview.chromium.org/896043004/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/notificationclick-can-focus.html:1:
<!DOCTYPE html>
On 2015/02/09 at 10:44:24, Michael van Ouwerkerk wrote:
> On 2015/02/09 07:00:58, dominicc wrote:
> > Should these tests be in ServiceWorker? Push Notifications seems significant
> > enough to have its own folder. Chatted with falken@ and his gut feeling is
> > Push/Notifications tests should go somewhere else. Maybe
> > http/tests/push_messaging?
> 
> These tests don't appear to exercise the push part of push notifications, so
just use third_party/WebKit/LayoutTests/http/tests/notifications/

This test is testing WaitUntilObserver behaviour, not Notifications behaviour.
It uses Notification for the notificationclick event. I think it should stay
here.

https://codereview.chromium.org/896043004/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/notificationclick-can-focus.html:7: if
(window.testRunner)
On 2015/02/09 at 07:00:58, dominicc wrote:
> If these tests are in Service Worker, tests that use Chromium-specific hooks
like this should be in chromium/ so we don't try to upstream them to W3C.

I wasn't aware of that directory. Will do.

https://codereview.chromium.org/896043004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/896043004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:98: static const
unsigned kWindowInteractionTimeout = 10;
On 2015/02/09 at 07:03:09, dominicc wrote:
> These could move into the implementation file since the logic to flip between
them is only accessible there anyway.

I wanted to have it there for future use but yeah, sounds good.

https://codereview.chromium.org/896043004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/testing/InternalsServiceWorker.cpp (right):

https://codereview.chromium.org/896043004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/testing/InternalsServiceWorker.cpp:9: #include
"modules/serviceworkers/WaitUntilObserver.h"
On 2015/02/09 at 07:03:09, dominicc wrote:
> Maybe this include is no longer needed?

Done.

[mlamouri (slow - plz ping), 2015-02-12 23:04:31]

review ping? :)

[dominicc (has gone to gerrit), 2015-02-13 03:45:11]

Sorry--holiday in Japan Wednesday put me behind.

Couple of comments inline. This LGTM once those are done, or happy to do another
round of feedback if I missed something.

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

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-focus.js:10:
var promise = new Promise(function(resolver) {
resolver -> resolve?

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-focus.js:30:
var CURRENT_TEST = -1;
Why ALL_CAPS for this? It's just a variable!

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-focus.js:115:
switch(e.data) {
Maybe "if" is more succinct?

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-openwindow.js
(right):

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-openwindow.js:104:
function runNextTestOrQuit() {
You could put this, the thing that starts it, and initialize in a shared helper
script.

[mlamouri (slow - plz ping), 2015-02-13 09:10:29]

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

[mlamouri (slow - plz ping), 2015-02-13 09:10:48]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-02-13 09:11:08]

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

[mlamouri (slow - plz ping), 2015-02-13 10:18:09]

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

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-focus.js:10:
var promise = new Promise(function(resolver) {
On 2015/02/13 at 03:45:11, dominicc wrote:
> resolver -> resolve?

Done.

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-focus.js:30:
var CURRENT_TEST = -1;
On 2015/02/13 at 03:45:11, dominicc wrote:
> Why ALL_CAPS for this? It's just a variable!

This is discrimination! :o

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-focus.js:115:
switch(e.data) {
On 2015/02/13 at 03:45:11, dominicc wrote:
> Maybe "if" is more succinct?

Done. It's also sending an error message and a "quit" message if the message
isn't "start".

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-focus.js:115:
switch(e.data) {
On 2015/02/13 at 03:45:11, dominicc wrote:
> Maybe "if" is more succinct?

Done. It's also sending an error message and a "quit" message if the message
isn't "start".

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-openwindow.js
(right):

https://codereview.chromium.org/896043004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/notificationclick-can-openwindow.js:104:
function runNextTestOrQuit() {
On 2015/02/13 at 03:45:11, dominicc wrote:
> You could put this, the thing that starts it, and initialize in a shared
helper script.

I have two other CLs testing .focus() and .openWindow(). I will refactor that in
the next one.

(Reason is that this CL is also fixing a bug and I prefer to have the feature
bug-free as soon as possible and doing the refactoring would be better with
another feedback cycle.)

[commit-bot: I haz the power, 2015-02-13 11:45:59]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190137