[DeviceLight] Add Layout tests

[DeviceLight] Add Layout tests

Add some Layout tests which need TestRunner support

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

[riju_, 2014-09-23 14:12:47]

Friendly ping. The DeviceLight feature has landed. PTAL.

[timvolodine, 2014-09-29 16:43:53]

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
File LayoutTests/fast/dom/DeviceLight/add-during-dispatch.html (right):

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
LayoutTests/fast/dom/DeviceLight/add-during-dispatch.html:6: description('Test
no fire listeners added during event dispatch.');
it's not clear to me what this is testing. from the code there is a check that
the secondListener should be invoked once...

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
LayoutTests/fast/dom/DeviceLight/add-during-dispatch.html:27:
setTimeout(function(){finish();}, 100);
why do we need timeout of 100ms? generally it's best to avoid such timeouts.

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
File LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html (right):

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html:6:
description('Tests that adding a new devicelight event listener from a callback
works as expected.');
what does "from a callback" mean? i don't see any addEventListeners in
callback.. maybe it is sufficient to say: "Tests that adding a new devicelight
event listener invokes a callback".

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
File LayoutTests/fast/dom/DeviceLight/page-visibility.html (right):

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:34: testPassed("Received a
devicelight event");
nit: a better place for this is inside "deviceLightListener".

[abarth-chromium, 2014-11-26 20:54:18]

abarth@chromium.org changed reviewers:
- abarth@chromium.org

[riju_, 2014-12-01 10:04:41]

Patchset #2 (id:20001) has been deleted

[riju_, 2015-01-21 10:00:53]

Patchset #3 (id:60001) has been deleted

[riju_, 2015-03-27 12:25:36]

Hi Tim, sorry for the late response.. now that the windows part is finally in,
we can have a final look on this CL.

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
File LayoutTests/fast/dom/DeviceLight/add-during-dispatch.html (right):

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
LayoutTests/fast/dom/DeviceLight/add-during-dispatch.html:6: description('Test
no fire listeners added during event dispatch.');
On 2014/09/29 16:43:53, timvolodine wrote:
> it's not clear to me what this is testing. from the code there is a check that
> the secondListener should be invoked once...

I am deleting this test case as of now.

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
File LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html (right):

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html:6:
description('Tests that adding a new devicelight event listener from a callback
works as expected.');
On 2014/09/29 16:43:53, timvolodine wrote:
> what does "from a callback" mean? i don't see any addEventListeners in
> callback.. maybe it is sufficient to say: "Tests that adding a new devicelight
> event listener invokes a callback".

Done.

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
File LayoutTests/fast/dom/DeviceLight/page-visibility.html (right):

https://codereview.chromium.org/490773003/diff/1/LayoutTests/fast/dom/DeviceL...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:34: testPassed("Received a
devicelight event");
On 2014/09/29 16:43:53, timvolodine wrote:
> nit: a better place for this is inside "deviceLightListener".

Done.

[timvolodine, 2015-03-30 13:00:55]

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html (right):

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html:6:
description('Tests that adding a new devicelight event listener invokes a
callback.');
why is this test called add-listener-from-callback? should it be add-listener or
add-listener-callback?

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html:12: debug('TEST
MODE enabled');
don't think this debug message is necessary

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceLight/multiple-event-listeners.html (right):

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/multiple-event-listeners.html:30: counter++;
technically the same counter is used for secondListener, so if firstListener is
invoked twice (and secondListener 0 times) the test will pass as well. maybe use
firstListenerCounter, secondListenerCounter or something like that?

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceLight/page-visibility.html (right):

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:20: if (window.testRunner)
no need for this if you add a check for window.testRunner in the beginning

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:29: finishJSTest();
also better to use setTimeout(finishJSTest, 0); because you are inside a
callback here.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:36: if (window.testRunner)
same here

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:41: testWithPageVisible();
should use setTimeout(testWithPageVisible, 0); because the addEventListener
receives a callback via a postTask

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:45:
testRunner.setMockDeviceLight(100);
nit: in other test you have debug('This test can not be run without the
TestRunner'); probably best to add here as well for consistency.

[riju_, 2015-03-31 11:03:42]

Thanks Tim for the comments. PTAL.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html (right):

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html:6:
description('Tests that adding a new devicelight event listener invokes a
callback.');
On 2015/03/30 13:00:54, timvolodine wrote:
> why is this test called add-listener-from-callback? should it be add-listener
or
> add-listener-callback?

Done.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/add-listener-from-callback.html:12: debug('TEST
MODE enabled');
On 2015/03/30 13:00:54, timvolodine wrote:
> don't think this debug message is necessary

Done.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceLight/multiple-event-listeners.html (right):

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/multiple-event-listeners.html:30: counter++;
On 2015/03/30 13:00:54, timvolodine wrote:
> technically the same counter is used for secondListener, so if firstListener
is
> invoked twice (and secondListener 0 times) the test will pass as well. maybe
use
> firstListenerCounter, secondListenerCounter or something like that?

Done. i checked a few times, firstListener and secondListener both were invoked 
once. But to make it clear in code, moving to the solution proposed by you.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceLight/page-visibility.html (right):

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:13: finishJSTest();
I guess it is better to use setTimeout(finishJSTest, 0) here also because we are
inside a callback.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:20: if (window.testRunner)
On 2015/03/30 13:00:54, timvolodine wrote:
> no need for this if you add a check for window.testRunner in the beginning

Done.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:29: finishJSTest();
On 2015/03/30 13:00:54, timvolodine wrote:
> also better to use setTimeout(finishJSTest, 0); because you are inside a
> callback here.

Done.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:36: if (window.testRunner)
On 2015/03/30 13:00:55, timvolodine wrote:
> same here

Done.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:41: testWithPageVisible();
On 2015/03/30 13:00:54, timvolodine wrote:
> should use setTimeout(testWithPageVisible, 0); because the addEventListener
> receives a callback via a postTask

Done.

https://codereview.chromium.org/490773003/diff/40001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceLight/page-visibility.html:45:
testRunner.setMockDeviceLight(100);
On 2015/03/30 13:00:54, timvolodine wrote:
> nit: in other test you have debug('This test can not be run without the
> TestRunner'); probably best to add here as well for consistency.

Done.

[timvolodine, 2015-04-02 10:52:46]

lgtm thanks!

[riju_, 2015-04-02 12:30:08]

The CQ bit was checked by rijubrata.bhaumik@intel.com

[commit-bot: I haz the power, 2015-04-02 12:30:59]

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

[commit-bot: I haz the power, 2015-04-02 23:28:38]

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

[commit-bot: I haz the power, 2015-04-02 23:28:39]

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

[riju_, 2015-04-07 17:23:29]

The CQ bit was checked by rijubrata.bhaumik@intel.com

[commit-bot: I haz the power, 2015-04-07 17:23:59]

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

[commit-bot: I haz the power, 2015-04-07 18:51:38]

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