Support screen.lockOrientation() / screen.unlockOrientation() when running layout tests

Support screen.lockOrientation() / screen.unlockOrientation() when running layout tests

Support screen.lockOrientation() / screen.unlockOrientation() when running
layout tests so that we can increase test coverage via layout tests.

The following functionality is implemented when running layout tests:
- The lock value is now stored when calling screen.lockOrientation() from the
  layout tests. If the current orientation is not allowed when the new lock,
  a new suitable orientation is chosen and an 'orientationchange' event is
  fired.
- The lock value is reset to WebScreenOrientationLockDefault when
  screen.unlockOrientation() is called. If the current screen orientation
  was not allowed by the previous lock, an 'orientationchange' event is fired.
- testRunner.setMockScreenOrientation() causes an 'orientationchange' event to
  be fired only if the orientation change is allowed by the current lock.

R=jochen@chromium.org
BUG=162827
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270552

[Inactive, 2014-05-02 20:26:21]

This is needed so that our blink side changes such as the following one can be
tested via layout tests:
https://codereview.chromium.org/261983005/

[mlamouri (slow - plz ping), 2014-05-02 20:33:03]

On 2014/05/02 20:26:21, Chris Dumez wrote:
> This is needed so that our blink side changes such as the following one can be
> tested via layout tests:
> https://codereview.chromium.org/261983005/

I'm not sure why Blink should test lockOrientation() and unlockOrientation()
given that the entire behaviour of those methods are delegated to the embedder.
The fullscreen change you made might be the only thing but I am not sure we
should do that in Blink.

[jochen (gone - plz use gerrit), 2014-05-05 07:28:50]

On 2014/05/02 20:33:03, Mounir Lamouri wrote:
> On 2014/05/02 20:26:21, Chris Dumez wrote:
> > This is needed so that our blink side changes such as the following one can
be
> > tested via layout tests:
> > https://codereview.chromium.org/261983005/
> 
> I'm not sure why Blink should test lockOrientation() and unlockOrientation()
> given that the entire behaviour of those methods are delegated to the
embedder.
> The fullscreen change you made might be the only thing but I am not sure we
> should do that in Blink.

Chris. what's your take on this?

[Inactive, 2014-05-05 13:40:54]

On 2014/05/05 07:28:50, jochen wrote:
> On 2014/05/02 20:33:03, Mounir Lamouri wrote:
> > On 2014/05/02 20:26:21, Chris Dumez wrote:
> > > This is needed so that our blink side changes such as the following one
can
> be
> > > tested via layout tests:
> > > https://codereview.chromium.org/261983005/
> > 
> > I'm not sure why Blink should test lockOrientation() and unlockOrientation()
> > given that the entire behaviour of those methods are delegated to the
> embedder.
> > The fullscreen change you made might be the only thing but I am not sure we
> > should do that in Blink.
> 
> Chris. what's your take on this?

There is currently no way from Layout Tests to check that lockOrientation() /
unlockOrientation() have any impact. This lack of coverage means we cannot
verify that Blink is correctly passing along the information to the embedder.
There is also currently no way for layout tests to check if the orientation is
indeed locked or not.

I think those are problems. As you know, every blink-side change needs
corresponding layout testing and not having the proper test infrastructure makes
things difficult. My fullscreen change is one example of such instance where the
test infrastructure is insufficient to verify (even though we are still debating
if this change should be in Blink or left up to the embedder). Not even
considering future features / bug fixes, the current layout testing for the
Screen Orientation API is currently fairly poor. I would like for all aspects of
the functionality to be covered by layout tests before we can consider this
implementation as stable and deemed acceptable for an intent to ship.

[jochen (gone - plz use gerrit), 2014-05-05 17:45:09]

On 2014/05/05 13:40:54, Chris Dumez wrote:
> On 2014/05/05 07:28:50, jochen wrote:
> > On 2014/05/02 20:33:03, Mounir Lamouri wrote:
> > > On 2014/05/02 20:26:21, Chris Dumez wrote:
> > > > This is needed so that our blink side changes such as the following one
> can
> > be
> > > > tested via layout tests:
> > > > https://codereview.chromium.org/261983005/
> > > 
> > > I'm not sure why Blink should test lockOrientation() and
unlockOrientation()
> > > given that the entire behaviour of those methods are delegated to the
> > embedder.
> > > The fullscreen change you made might be the only thing but I am not sure
we
> > > should do that in Blink.
> > 
> > Chris. what's your take on this?
> 
> There is currently no way from Layout Tests to check that lockOrientation() /
> unlockOrientation() have any impact. This lack of coverage means we cannot
> verify that Blink is correctly passing along the information to the embedder.
> There is also currently no way for layout tests to check if the orientation is
> indeed locked or not.
> 
> I think those are problems. As you know, every blink-side change needs
> corresponding layout testing and not having the proper test infrastructure
makes
> things difficult. My fullscreen change is one example of such instance where
the
> test infrastructure is insufficient to verify (even though we are still
debating
> if this change should be in Blink or left up to the embedder). Not even
> considering future features / bug fixes, the current layout testing for the
> Screen Orientation API is currently fairly poor. I would like for all aspects
of
> the functionality to be covered by layout tests before we can consider this
> implementation as stable and deemed acceptable for an intent to ship.

I tend to agree, Mounir what about you?

[jochen (gone - plz use gerrit), 2014-05-06 15:59:00]

can you also hook this up to content shell?

https://codereview.chromium.org/268673015/diff/1/content/renderer/renderer_we...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/268673015/diff/1/content/renderer/renderer_we...
content/renderer/renderer_webkitplatformsupport_impl.cc:1149: bool
isScreenOrientationAllowedByTestingLock(blink::WebScreenOrientationType
that doesn't look correctly wrapped, did you use clang-format?

[Inactive, 2014-05-06 16:26:21]

https://codereview.chromium.org/268673015/diff/1/content/renderer/renderer_we...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/268673015/diff/1/content/renderer/renderer_we...
content/renderer/renderer_webkitplatformsupport_impl.cc:1149: bool
isScreenOrientationAllowedByTestingLock(blink::WebScreenOrientationType
On 2014/05/06 15:59:01, jochen wrote:
> that doesn't look correctly wrapped, did you use clang-format?

I am not familiar with the Chromium-side coding style, I wrapped manually so it
is definitely possible it is not wrapped the right way. I will double check.

[Inactive, 2014-05-06 16:56:45]

https://codereview.chromium.org/268673015/diff/1/content/renderer/renderer_we...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/268673015/diff/1/content/renderer/renderer_we...
content/renderer/renderer_webkitplatformsupport_impl.cc:1149: bool
isScreenOrientationAllowedByTestingLock(blink::WebScreenOrientationType
On 2014/05/06 15:59:01, jochen wrote:
> that doesn't look correctly wrapped, did you use clang-format?

Done.

[Inactive, 2014-05-06 16:58:03]

On 2014/05/06 15:59:00, jochen wrote:
> can you also hook this up to content shell?

What do you mean? I tested this patch and my Blink-side one and the layout tests
were passing just fine. Is there more code needed in the content shell?

[jochen (gone - plz use gerrit), 2014-05-07 06:48:41]

sorry, I didn't see that it's already hooked up

https://codereview.chromium.org/268673015/diff/20001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/268673015/diff/20001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:148: static
blink::WebScreenOrientationLockType g_test_current_screen_lock =
i'd rather not have so many variables here. Is it possible to put the logic and
variables into a class?

https://codereview.chromium.org/268673015/diff/20001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1149: bool
isScreenOrientationAllowedByTestingLock(
methods should start with capital letters

https://codereview.chromium.org/268673015/diff/20001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1153: return true;
nit { } since the if part is multiline

https://codereview.chromium.org/268673015/diff/20001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1243: if
(!isScreenOrientationAllowedByTestingLock(orientation))
what happens if a test fails before it unlocks?

[Inactive, 2014-05-07 18:41:05]

https://codereview.chromium.org/268673015/diff/20001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/268673015/diff/20001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1149: bool
isScreenOrientationAllowedByTestingLock(
On 2014/05/07 06:48:41, jochen wrote:
> methods should start with capital letters

Done.

https://codereview.chromium.org/268673015/diff/20001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1153: return true;
On 2014/05/07 06:48:41, jochen wrote:
> nit { } since the if part is multiline

Done.

https://codereview.chromium.org/268673015/diff/20001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1243: if
(!isScreenOrientationAllowedByTestingLock(orientation))
On 2014/05/07 06:48:41, jochen wrote:
> what happens if a test fails before it unlocks?

Flakes :) I added a ResetMockScreenOrientationForTesting() method to address the
problem. I have verified that it works but I am not 100% sure the way I hooked
it up is the best way.

[jochen (gone - plz use gerrit), 2014-05-08 07:12:49]

almost there

https://codereview.chromium.org/268673015/diff/30001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/268673015/diff/30001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:153: class
ScreenOrientationDataForTesting {
can you move this class and the lazy instance above line 151 and put all the
globals into and anonymous namespace?

https://codereview.chromium.org/268673015/diff/30001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1163: bool
IsScreenOrientationAllowedByTestingLock(
can these now be members on the ScreenOrientatinoDataForTesting?

Maybe move the class to a separate file?

[Inactive, 2014-05-08 20:07:46]

Thank you for your patience. I moved the class out and made it more object
oriented.

https://codereview.chromium.org/268673015/diff/30001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/268673015/diff/30001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:153: class
ScreenOrientationDataForTesting {
On 2014/05/08 07:12:49, jochen wrote:
> can you move this class and the lazy instance above line 151 and put all the
> globals into and anonymous namespace?

Done.

https://codereview.chromium.org/268673015/diff/30001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1163: bool
IsScreenOrientationAllowedByTestingLock(
On 2014/05/08 07:12:49, jochen wrote:
> can these now be members on the ScreenOrientatinoDataForTesting?
> 
> Maybe move the class to a separate file?

Done.

[Inactive, 2014-05-12 14:28:11]

ping? :)

[jochen (gone - plz use gerrit), 2014-05-14 09:10:54]

lgtm

lgtm with comments addressed

https://codereview.chromium.org/268673015/diff/50001/content/renderer/screen_...
File content/renderer/screen_orientation/mock_screen_orientation_controller.h
(right):

https://codereview.chromium.org/268673015/diff/50001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.h:36: };
disallow copy & assign

https://codereview.chromium.org/268673015/diff/50001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.h:38:
MockScreenOrientationController::MockScreenOrientationController()
please put the implementation into a .cc file

[Inactive, 2014-05-14 13:33:02]

https://codereview.chromium.org/268673015/diff/50001/content/renderer/screen_...
File content/renderer/screen_orientation/mock_screen_orientation_controller.h
(right):

https://codereview.chromium.org/268673015/diff/50001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.h:36: };
On 2014/05/14 09:10:54, jochen wrote:
> disallow copy & assign

Done.

https://codereview.chromium.org/268673015/diff/50001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.h:38:
MockScreenOrientationController::MockScreenOrientationController()
On 2014/05/14 09:10:54, jochen wrote:
> please put the implementation into a .cc file

Done.

[Inactive, 2014-05-14 13:34:43]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-05-14 13:35:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/268673015/70001

[commit-bot: I haz the power, 2014-05-14 15:09:01]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 15:11:28]

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

[commit-bot: I haz the power, 2014-05-14 15:11:28]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[Inactive, 2014-05-14 15:12:39]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-05-14 15:13:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/268673015/70001

[commit-bot: I haz the power, 2014-05-14 16:27:58]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 16:40:38]

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

[commit-bot: I haz the power, 2014-05-14 16:40:38]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Inactive, 2014-05-14 17:01:15]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-05-14 17:01:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/268673015/70001

[commit-bot: I haz the power, 2014-05-14 17:34:12]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 17:49:42]

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

[commit-bot: I haz the power, 2014-05-14 17:49:43]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Inactive, 2014-05-14 17:51:31]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-05-14 17:52:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/268673015/70001

[commit-bot: I haz the power, 2014-05-14 18:09:09]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 18:24:11]

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

[commit-bot: I haz the power, 2014-05-14 18:24:11]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Inactive, 2014-05-14 19:16:43]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-05-14 19:17:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/268673015/70001

[commit-bot: I haz the power, 2014-05-14 19:35:40]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 19:49:58]

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

[commit-bot: I haz the power, 2014-05-14 19:49:59]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Inactive, 2014-05-14 20:16:09]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-05-14 20:17:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/268673015/70001

[commit-bot: I haz the power, 2014-05-14 20:34:01]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 20:48:25]

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

[commit-bot: I haz the power, 2014-05-14 20:48:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Inactive, 2014-05-14 23:57:11]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-05-14 23:59:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/268673015/70001

[commit-bot: I haz the power, 2014-05-15 01:04:51]

Change committed as 270552