Add end-to-end browser tests for Device Motion/Orientation null-events.

Add end-to-end browser tests for Device Motion/Orientation null-events.

If no sensors are available for the Device Motion/Orientation API
a JavaScript callback should be executed with all values set to null
to let the page know no data can be provided. This currently happens
e.g. on the Linux platform where no relevant sensors are available.

The added browser tests verify that the correct null-event callback
occurs even when the active DOM objects are temporarily suspended
in the beginning.

NOTE: This should land after the blink patch
https://codereview.chromium.org/212983007/ has landed and rolled.

BUG=356693
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261862

[Michael van Ouwerkerk, 2014-04-03 14:41:47]

lgtm

[timvolodine, 2014-04-03 14:53:54]

On 2014/04/03 14:41:47, Michael van Ouwerkerk wrote:
> lgtm

thanks Michael!

[timvolodine, 2014-04-03 18:59:01]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-04-03 18:59:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/214763002/1

[commit-bot: I haz the power, 2014-04-03 18:59:57]

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

[commit-bot: I haz the power, 2014-04-03 18:59:58]

Failed to apply patch for
content/browser/device_orientation/device_inertial_sensor_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
content/browser/device_orientation/device_inertial_sensor_browsertest.cc
  Hunk #2 FAILED at 12.
  1 out of 8 hunks FAILED -- saving rejects to file
content/browser/device_orientation/device_inertial_sensor_browsertest.cc.rej

Patch:      
content/browser/device_orientation/device_inertial_sensor_browsertest.cc
Index: content/browser/device_orientation/device_inertial_sensor_browsertest.cc
diff --git
a/content/browser/device_orientation/device_inertial_sensor_browsertest.cc
b/content/browser/device_orientation/device_inertial_sensor_browsertest.cc
index
a176f712b268dd12b1cb123b890f795a05260a9c..429dc3754ff49ad862331191219b3c68802ba09a
100644
--- a/content/browser/device_orientation/device_inertial_sensor_browsertest.cc
+++ b/content/browser/device_orientation/device_inertial_sensor_browsertest.cc
@@ -4,6 +4,7 @@
 
 #include "base/command_line.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/threading/platform_thread.h"
 #include "content/browser/device_orientation/data_fetcher_shared_memory.h"
 #include "content/browser/device_orientation/device_inertial_sensor_service.h"
 #include "content/common/device_orientation/device_motion_hardware_buffer.h"
@@ -11,7 +12,10 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/test/test_navigation_observer.h"
+#include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
+#include "content/shell/browser/shell_javascript_dialog_manager.h"
 #include "content/test/content_browser_test.h"
 #include "content/test/content_browser_test_utils.h"
 
@@ -25,7 +29,8 @@ class FakeDataFetcher : public DataFetcherSharedMemory {
       : started_orientation_(false, false),
         stopped_orientation_(false, false),
         started_motion_(false, false),
-        stopped_motion_(false, false) {
+        stopped_motion_(false, false),
+        sensor_data_available_(true) {
   }
   virtual ~FakeDataFetcher() { }
 
@@ -34,13 +39,24 @@ class FakeDataFetcher : public DataFetcherSharedMemory {
 
     switch (consumer_type) {
       case CONSUMER_TYPE_MOTION:
-        UpdateMotion(static_cast<DeviceMotionHardwareBuffer*>(buffer));
-        started_motion_.Signal();
+        {
+          DeviceMotionHardwareBuffer* motion_buffer =
+              static_cast<DeviceMotionHardwareBuffer*>(buffer);
+          if (sensor_data_available_)
+            UpdateMotion(motion_buffer);
+          SetMotionBufferReady(motion_buffer);
+          started_motion_.Signal();
+        }
         break;
       case CONSUMER_TYPE_ORIENTATION:
-        UpdateOrientation(
-            static_cast<DeviceOrientationHardwareBuffer*>(buffer));
-        started_orientation_.Signal();
+        {
+          DeviceOrientationHardwareBuffer* orientation_buffer =
+              static_cast<DeviceOrientationHardwareBuffer*>(buffer);
+          if (sensor_data_available_)
+            UpdateOrientation(orientation_buffer);
+          SetOrientationBufferReady(orientation_buffer);
+          started_orientation_.Signal();
+        }
         break;
       default:
         return false;
@@ -70,6 +86,22 @@ class FakeDataFetcher : public DataFetcherSharedMemory {
     return FETCHER_TYPE_DEFAULT;
   }
 
+  void SetSensorDataAvailable(bool available) {
+    sensor_data_available_ = available;
+  }
+
+  void SetMotionBufferReady(DeviceMotionHardwareBuffer* buffer) {
+    buffer->seqlock.WriteBegin();
+    buffer->data.allAvailableSensorsAreActive = true;
+    buffer->seqlock.WriteEnd();
+  }
+
+  void SetOrientationBufferReady(DeviceOrientationHardwareBuffer* buffer) {
+    buffer->seqlock.WriteBegin();
+    buffer->data.allAvailableSensorsAreActive = true;
+    buffer->seqlock.WriteEnd();
+  }
+
   void UpdateMotion(DeviceMotionHardwareBuffer* buffer) {
     buffer->seqlock.WriteBegin();
     buffer->data.accelerationX = 1;
@@ -114,6 +146,7 @@ class FakeDataFetcher : public DataFetcherSharedMemory {
   base::WaitableEvent stopped_orientation_;
   base::WaitableEvent started_motion_;
   base::WaitableEvent stopped_motion_;
+  bool sensor_data_available_;
 
  private:
 
@@ -142,6 +175,23 @@ class DeviceInertialSensorBrowserTest : public
ContentBrowserTest  {
     io_loop_finished_event_.Signal();
   }
 
+  void DelayAndQuit(base::TimeDelta delay) {
+    base::PlatformThread::Sleep(delay);
+    base::MessageLoop::current()->QuitWhenIdle();
+  }
+
+  void WaitForAlertDialogAndQuitAfterDelay(base::TimeDelta delay) {
+    ShellJavaScriptDialogManager* dialog_manager=
+        static_cast<ShellJavaScriptDialogManager*>(
+            shell()->GetJavaScriptDialogManager());
+
+    scoped_refptr<MessageLoopRunner> runner = new MessageLoopRunner();
+    dialog_manager->set_dialog_request_callback(
+        base::Bind(&DeviceInertialSensorBrowserTest::DelayAndQuit, this,
+            delay));
+    runner->Run();
+  }
+
   FakeDataFetcher* fetcher_;
 
  private:
@@ -175,6 +225,46 @@ IN_PROC_BROWSER_TEST_F(DeviceInertialSensorBrowserTest,
MotionTest) {
   fetcher_->stopped_motion_.Wait();
 }
 
+IN_PROC_BROWSER_TEST_F(DeviceInertialSensorBrowserTest, OrientationNullTest) {
+  // The test page will register an event handler for orientation events,
+  // expects to get an event with null values. The test raises a modal alert
+  // dialog with a delay to test that the one-off null-event still propagates
+  // to window and the callback is invoked which navigates to #pass.
+  fetcher_->SetSensorDataAvailable(false);
+  TestNavigationObserver same_tab_observer(shell()->web_contents(), 2);
+
+  GURL test_url = GetTestUrl(
+      "device_orientation", "device_orientation_null_test.html");
+  shell()->LoadURL(test_url);
+
+  WaitForAlertDialogAndQuitAfterDelay(base::TimeDelta::FromMilliseconds(1000));
+
+  fetcher_->started_orientation_.Wait();
+  fetcher_->stopped_orientation_.Wait();
+  same_tab_observer.Wait();
+  EXPECT_EQ("pass", shell()->web_contents()->GetLastCommittedURL().ref());
+}
+
+IN_PROC_BROWSER_TEST_F(DeviceInertialSensorBrowserTest, MotionNullTest) {
+  // The test page will register an event handler for motion events,
+  // expects to get an event with null values. The test raises a modal alert
+  // dialog with a delay to test that the one-off null-event still propagates
+  // to window and the callback is invoked which navigates to #pass.
+  fetcher_->SetSensorDataAvailable(false);
+  TestNavigationObserver same_tab_observer(shell()->web_contents(), 2);
+
+  GURL test_url = GetTestUrl(
+      "device_orientation", "device_motion_null_test.html");
+  shell()->LoadURL(test_url);
+
+  WaitForAlertDialogAndQuitAfterDelay(base::TimeDelta::FromMilliseconds(1000));
+
+  fetcher_->started_motion_.Wait();
+  fetcher_->stopped_motion_.Wait();
+  same_tab_observer.Wait();
+  EXPECT_EQ("pass", shell()->web_contents()->GetLastCommittedURL().ref());
+}
+
 } //  namespace
 
 } //  namespace content

[mlamouri (slow - plz ping), 2014-04-04 13:47:07]

https://codereview.chromium.org/214763002/diff/1/content/browser/device_orien...
File content/browser/device_orientation/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/214763002/diff/1/content/browser/device_orien...
content/browser/device_orientation/device_inertial_sensor_browsertest.cc:240:
WaitForAlertDialogAndQuitAfterDelay(base::TimeDelta::FromMilliseconds(1000));
That does not LG2M. Having a test with a delay/timeout is a recipe for flakyness
and slow tests.

I think we should find another way to do that. Do we know for sure that the
event will be fired at the next event loop after calling addEventListener()? Can
we communicate from the web page to the test process? Worse case scenario, we
could poll that the URL contains #pass or #fail but that should be a last
resort.

https://codereview.chromium.org/214763002/diff/1/content/browser/device_orien...
content/browser/device_orientation/device_inertial_sensor_browsertest.cc:260:
WaitForAlertDialogAndQuitAfterDelay(base::TimeDelta::FromMilliseconds(1000));
ditto

https://codereview.chromium.org/214763002/diff/1/content/test/data/device_ori...
File content/test/data/device_orientation/device_motion_null_test.html (right):

https://codereview.chromium.org/214763002/diff/1/content/test/data/device_ori...
content/test/data/device_orientation/device_motion_null_test.html:19:
window.removeEventListener('devicemotion', onMotion);
That sounds odd. Shouldn't you always remove the event listener? Otherwise, you
might have situations like:
* unexpected event
* unexpected event
* null event
=> test pass

I think doing this would be safer:
window.removeEventListener('devicemotion', onMotion);
checkMotionEvent(event) ? pass() : fail();

Also, note that you could do:
window.addEventListener('devicemotion', function(e) {
  window.removeEventListener('devicemotion', arguments.callee);
  checkMotionEvent(event) ? pass() : fail();
});

https://codereview.chromium.org/214763002/diff/1/content/test/data/device_ori...
File content/test/data/device_orientation/device_orientation_null_test.html
(right):

https://codereview.chromium.org/214763002/diff/1/content/test/data/device_ori...
content/test/data/device_orientation/device_orientation_null_test.html:13:
window.removeEventListener('deviceorientation', onOrientation);
ditto

[timvolodine, 2014-04-04 18:03:18]

thanks for the comments Mounir!

https://codereview.chromium.org/214763002/diff/1/content/browser/device_orien...
File content/browser/device_orientation/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/214763002/diff/1/content/browser/device_orien...
content/browser/device_orientation/device_inertial_sensor_browsertest.cc:240:
WaitForAlertDialogAndQuitAfterDelay(base::TimeDelta::FromMilliseconds(1000));
On 2014/04/04 13:47:07, Mounir Lamouri wrote:
> That does not LG2M. Having a test with a delay/timeout is a recipe for
flakyness
> and slow tests.
> 
> I think we should find another way to do that. Do we know for sure that the
> event will be fired at the next event loop after calling addEventListener()?
Can
> we communicate from the web page to the test process? Worse case scenario, we
> could poll that the URL contains #pass or #fail but that should be a last
> resort.

This is a test for a very specific scenario and I spent some time thinking how
to best test this. Currently it does not look feasible to test this at Blink
level i.e. in LayoutTests, therefore this construction with an alert+delay which
seems the most 'straightforward' way. I've filed a bug (crbug.com/360044) and
added a comment to investigate this in the future.

https://codereview.chromium.org/214763002/diff/1/content/test/data/device_ori...
File content/test/data/device_orientation/device_motion_null_test.html (right):

https://codereview.chromium.org/214763002/diff/1/content/test/data/device_ori...
content/test/data/device_orientation/device_motion_null_test.html:19:
window.removeEventListener('devicemotion', onMotion);
On 2014/04/04 13:47:07, Mounir Lamouri wrote:
> That sounds odd. Shouldn't you always remove the event listener? Otherwise,
you
> might have situations like:
> * unexpected event
> * unexpected event
> * null event
> => test pass
> 
> I think doing this would be safer:
> window.removeEventListener('devicemotion', onMotion);
> checkMotionEvent(event) ? pass() : fail();
> 
> Also, note that you could do:
> window.addEventListener('devicemotion', function(e) {
>   window.removeEventListener('devicemotion', arguments.callee);
>   checkMotionEvent(event) ? pass() : fail();
> });

Hmm, yes that sounds better indeed, I've updated the code to always remove the
listener.

[timvolodine, 2014-04-04 18:05:01]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-04-04 18:05:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/214763002/20001

[commit-bot: I haz the power, 2014-04-04 20:57:13]

Change committed as 261862