Screen Orientation API: cleanup, simplify and fix implementation.

Screen Orientation API: cleanup, simplify and fix implementation.

This CL creates new types to better reflect screen orientation types. It
also simplify ScreenOrientation.cpp, following spec simplifications.
Finally, there are some changes related to |m_lockedOrientations| that
was a bit buggy by nature, assuming that only the current fram could
change the locking.

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

[mlamouri (slow - plz ping), 2014-03-31 16:01:55]

abarth@chromium.org: Please review changes in 
public/platform/*

[mlamouri (slow - plz ping), 2014-03-31 16:20:29]

I have cleanup the patch a bit. No more #if BLINK_IMPLEMENTATION. However, it
now requires https://codereview.chromium.org/215313002/ to land.

[Inactive, 2014-03-31 18:52:35]

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:19: char orientation;
Any reason we are not using a WebScreenOrientationLockType here? It seems you
initialize only with WebScreenOrientationLockType values.

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:51: if
(static_cast<blink::WebScreenOrientationType>(orientationMap[i].orientation) ==
orientation)
So we rely on the values shared by WebScreenOrientationLockType and
WebScreenOrientationType to be the same. This looks fragile, especially without
compile-time assertions.

Also, the map contains values that are no valid WebScreenOrientationType values
(i.e. WebScreenOrientationLockAny and following). We should probably assert
against this before casting.

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:87:
m_orientationLockTimer.startOneShot(0, FROM_HERE);
So consecutive calls to lockOrientation("portrait") are going to call into the
platform each time? There used to be a m_lockedOrientations == orientations
early return to protect against this.

https://codereview.chromium.org/219463003/diff/20001/public/platform/WebScree...
File public/platform/WebScreenOrientationLockType.h (right):

https://codereview.chromium.org/219463003/diff/20001/public/platform/WebScree...
public/platform/WebScreenOrientationLockType.h:12:
WebScreenOrientationLockPortraitPrimary ,
extra space

[mlamouri (slow - plz ping), 2014-03-31 19:45:35]

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:19: char orientation;
On 2014/03/31 18:52:36, Chris Dumez wrote:
> Any reason we are not using a WebScreenOrientationLockType here? It seems you
> initialize only with WebScreenOrientationLockType values.

I could put WebScreenOrientationLockType but it would give the false impression
that it should only be used for that. I don't care much TBH.

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:51: if
(static_cast<blink::WebScreenOrientationType>(orientationMap[i].orientation) ==
orientation)
On 2014/03/31 18:52:36, Chris Dumez wrote:
> So we rely on the values shared by WebScreenOrientationLockType and
> WebScreenOrientationType to be the same. This looks fragile, especially
without
> compile-time assertions.
> 
> Also, the map contains values that are no valid WebScreenOrientationType
values
> (i.e. WebScreenOrientationLockAny and following). We should probably assert
> against this before casting.

Argh. I meant to add compile time checks but I forgot.. Will update that.
Regarding values not valid for WebScreenOrientationType, given that we get the
orientation from the embedder, we could probably trust the value. Actually, we
get a value and we want a string. It sounds just not possible to not get what we
want. Assuming we have that compile check.

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:87:
m_orientationLockTimer.startOneShot(0, FROM_HERE);
On 2014/03/31 18:52:36, Chris Dumez wrote:
> So consecutive calls to lockOrientation("portrait") are going to call into the
> platform each time? There used to be a m_lockedOrientations == orientations
> early return to protect against this.

If another ScreenOrientation instance lock the screen to some value,
m_lockedOrientations == orientations would falsely early return.

Also, a behaviour we want is that the following code locks to portrait:
screen.lockOrientation('landscape');
screen.lockOrientation('portrait');

It seems that the current code would lock to 'landscape'.

https://codereview.chromium.org/219463003/diff/20001/public/platform/WebScree...
File public/platform/WebScreenOrientationLockType.h (right):

https://codereview.chromium.org/219463003/diff/20001/public/platform/WebScree...
public/platform/WebScreenOrientationLockType.h:12:
WebScreenOrientationLockPortraitPrimary ,
On 2014/03/31 18:52:36, Chris Dumez wrote:
> extra space

Done.

[Inactive, 2014-03-31 19:59:42]

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:87:
m_orientationLockTimer.startOneShot(0, FROM_HERE);
On 2014/03/31 19:45:35, Mounir Lamouri wrote:
> On 2014/03/31 18:52:36, Chris Dumez wrote:
> > So consecutive calls to lockOrientation("portrait") are going to call into
the
> > platform each time? There used to be a m_lockedOrientations == orientations
> > early return to protect against this.
> 
> If another ScreenOrientation instance lock the screen to some value,
> m_lockedOrientations == orientations would falsely early return.
> 
> Also, a behaviour we want is that the following code locks to portrait:
> screen.lockOrientation('landscape');
> screen.lockOrientation('portrait');
> 
> It seems that the current code would lock to 'landscape'.

What do you mean by another ScreenOrientation instance?

Why would the existing code lock to landscape? The second call would override
the value of m_lockedOrientations member before the timer has fired, right?

[mlamouri (slow - plz ping), 2014-03-31 20:05:04]

On 2014/03/31 19:59:42, Chris Dumez wrote:
>
https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
> File Source/modules/screen_orientation/ScreenOrientation.cpp (right):
> 
>
https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
> Source/modules/screen_orientation/ScreenOrientation.cpp:87:
> m_orientationLockTimer.startOneShot(0, FROM_HERE);
> On 2014/03/31 19:45:35, Mounir Lamouri wrote:
> > On 2014/03/31 18:52:36, Chris Dumez wrote:
> > > So consecutive calls to lockOrientation("portrait") are going to call into
> the
> > > platform each time? There used to be a m_lockedOrientations ==
orientations
> > > early return to protect against this.
> > 
> > If another ScreenOrientation instance lock the screen to some value,
> > m_lockedOrientations == orientations would falsely early return.
> > 
> > Also, a behaviour we want is that the following code locks to portrait:
> > screen.lockOrientation('landscape');
> > screen.lockOrientation('portrait');
> > 
> > It seems that the current code would lock to 'landscape'.
> 
> What do you mean by another ScreenOrientation instance?

ScreenOrientation instance is linked to a Screen, right? If you have multiple
Screen objects, you would have multiple ScreenOrientation? Is that correct?

> Why would the existing code lock to landscape? The second call would override
> the value of m_lockedOrientations member before the timer has fired, right?

My bad indeed.

[Inactive, 2014-04-01 13:09:06]

On 2014/03/31 20:05:04, Mounir Lamouri wrote:
> On 2014/03/31 19:59:42, Chris Dumez wrote:
> >
>
https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
> > File Source/modules/screen_orientation/ScreenOrientation.cpp (right):
> > 
> >
>
https://codereview.chromium.org/219463003/diff/20001/Source/modules/screen_or...
> > Source/modules/screen_orientation/ScreenOrientation.cpp:87:
> > m_orientationLockTimer.startOneShot(0, FROM_HERE);
> > On 2014/03/31 19:45:35, Mounir Lamouri wrote:
> > > On 2014/03/31 18:52:36, Chris Dumez wrote:
> > > > So consecutive calls to lockOrientation("portrait") are going to call
into
> > the
> > > > platform each time? There used to be a m_lockedOrientations ==
> orientations
> > > > early return to protect against this.
> > > 
> > > If another ScreenOrientation instance lock the screen to some value,
> > > m_lockedOrientations == orientations would falsely early return.
> > > 
> > > Also, a behaviour we want is that the following code locks to portrait:
> > > screen.lockOrientation('landscape');
> > > screen.lockOrientation('portrait');
> > > 
> > > It seems that the current code would lock to 'landscape'.
> > 
> > What do you mean by another ScreenOrientation instance?
> 
> ScreenOrientation instance is linked to a Screen, right? If you have multiple
> Screen objects, you would have multiple ScreenOrientation? Is that correct?

Yes, I believe you are right. Likely in the case of frames, we would have
multiple ScreenOrientation instances for a single tab since each frame would
have its own window / screen object.

[Inactive, 2014-04-01 13:20:44]

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:16:
COMPILE_ASSERT((char)blink::WebScreenOrientationPortraitPrimary ==
(char)blink::WebScreenOrientationLockPortraitPrimary, mismatching_types);
We prefer C++ casting instead of C casting (here static_cast<int>() for e.g.).

I am note sure about the location of those compile assertions. Here is fine by
me but abarth@ should confirm.

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:25: char orientation;
If you don't want to switch to the enum type, shouldn't we switch to an unsigned
type at least? (unsigned or unsigned char).

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:57: if
(static_cast<blink::WebScreenOrientationType>(orientationMap[i].orientation) ==
orientation)
I still think we should add some kind of assertion to make sure we never cast
WebScreenOrientationLockAny and following values in orientationMap to a
WebScreenOrientationType.

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:61: // We do no handle
OrientationInvalid and OrientationAny but this is fine because
screen.orientation
This comment seems outdated now.

https://codereview.chromium.org/219463003/diff/40001/public/platform/WebScree...
File public/platform/WebScreenOrientation.h (right):

https://codereview.chromium.org/219463003/diff/40001/public/platform/WebScree...
public/platform/WebScreenOrientation.h:13: typedef unsigned char
WebScreenOrientations;
Should we add a FIXME that this file should be removed once chromium side is
updated?

[mlamouri (slow - plz ping), 2014-04-01 16:30:20]

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:16:
COMPILE_ASSERT((char)blink::WebScreenOrientationPortraitPrimary ==
(char)blink::WebScreenOrientationLockPortraitPrimary, mismatching_types);
On 2014/04/01 13:20:44, Chris Dumez wrote:
> We prefer C++ casting instead of C casting (here static_cast<int>() for e.g.).
> 
> I am note sure about the location of those compile assertions. Here is fine by
> me but abarth@ should confirm.

Done.

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:25: char orientation;
On 2014/04/01 13:20:44, Chris Dumez wrote:
> If you don't want to switch to the enum type, shouldn't we switch to an
unsigned
> type at least? (unsigned or unsigned char).

Done.

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:57: if
(static_cast<blink::WebScreenOrientationType>(orientationMap[i].orientation) ==
orientation)
On 2014/04/01 13:20:44, Chris Dumez wrote:
> I still think we should add some kind of assertion to make sure we never cast
> WebScreenOrientationLockAny and following values in orientationMap to a
> WebScreenOrientationType.

Are you more comfortable with |orientation| being casted as an unsigned char?

https://codereview.chromium.org/219463003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:61: // We do no handle
OrientationInvalid and OrientationAny but this is fine because
screen.orientation
On 2014/04/01 13:20:44, Chris Dumez wrote:
> This comment seems outdated now.

Removed. The assert should be self explanatory.

https://codereview.chromium.org/219463003/diff/40001/public/platform/WebScree...
File public/platform/WebScreenOrientation.h (right):

https://codereview.chromium.org/219463003/diff/40001/public/platform/WebScree...
public/platform/WebScreenOrientation.h:13: typedef unsigned char
WebScreenOrientations;
On 2014/04/01 13:20:44, Chris Dumez wrote:
> Should we add a FIXME that this file should be removed once chromium side is
> updated?

It goes without saying that this should be updated. I've added as FIXME to make
this clear.

[Inactive, 2014-04-01 17:34:24]

lgtm but Adam needs to take a look at it as well.

https://codereview.chromium.org/219463003/diff/60001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/219463003/diff/60001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:15: // This code assume
that WebScreenOrientationType values are included in
WebScreenOrientationLockType.
nit: assumes

https://codereview.chromium.org/219463003/diff/60001/public/platform/WebScree...
File public/platform/WebScreenOrientation.h (right):

https://codereview.chromium.org/219463003/diff/60001/public/platform/WebScree...
public/platform/WebScreenOrientation.h:8: // FIXME: this file should be removed
as soon as Chrome will no longer depend on it.
nit: depends

[mlamouri (slow - plz ping), 2014-04-01 19:37:52]

https://codereview.chromium.org/219463003/diff/60001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/219463003/diff/60001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:15: // This code assume
that WebScreenOrientationType values are included in
WebScreenOrientationLockType.
On 2014/04/01 17:34:24, Chris Dumez wrote:
> nit: assumes

Done.

https://codereview.chromium.org/219463003/diff/60001/public/platform/WebScree...
File public/platform/WebScreenOrientation.h (right):

https://codereview.chromium.org/219463003/diff/60001/public/platform/WebScree...
public/platform/WebScreenOrientation.h:8: // FIXME: this file should be removed
as soon as Chrome will no longer depend on it.
On 2014/04/01 17:34:24, Chris Dumez wrote:
> nit: depends

Done.

[abarth-chromium, 2014-04-07 19:59:35]

lgtm

https://codereview.chromium.org/219463003/diff/70001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/219463003/diff/70001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:21:
COMPILE_ASSERT_MATCHING_ENUM(WebScreenOrientationLandscapeSecondary,
WebScreenOrientationLockLandscapeSecondary);
We usually put these in AssertMatchingEnums.cpp

https://codereview.chromium.org/219463003/diff/70001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:27: unsigned char
orientation;
Why "unsigned char" and not blink::WebScreenOrientationType?  I guess you're
trying to squeeze out some binary size?

If you keep this, can you add a COMPILE_ASSERT that we don't overflow unsigned
char?

[mlamouri (slow - plz ping), 2014-04-13 01:59:30]

https://codereview.chromium.org/219463003/diff/70001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/219463003/diff/70001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:21:
COMPILE_ASSERT_MATCHING_ENUM(WebScreenOrientationLandscapeSecondary,
WebScreenOrientationLockLandscapeSecondary);
On 2014/04/07 19:59:36, abarth wrote:
> We usually put these in AssertMatchingEnums.cpp

I left this here because web/AssertMatchingEnums.cpp is for WebCore:: vs
blink::. I can create a AssertMatchingEnums.cpp file in
modules/screen_orientation/ if you want.

https://codereview.chromium.org/219463003/diff/70001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:27: unsigned char
orientation;
On 2014/04/07 19:59:36, abarth wrote:
> Why "unsigned char" and not blink::WebScreenOrientationType?  I guess you're
> trying to squeeze out some binary size?
> 
> If you keep this, can you add a COMPILE_ASSERT that we don't overflow unsigned
> char?

I switched from 'unsigned char' to 'unsigned'.

[mlamouri (slow - plz ping), 2014-04-13 01:59:36]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-13 01:59:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/219463003/90001

[commit-bot: I haz the power, 2014-04-13 02:27:03]

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

[commit-bot: I haz the power, 2014-04-13 02:27:03]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[mlamouri (slow - plz ping), 2014-04-13 02:37:34]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-13 02:37:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/219463003/110001

[commit-bot: I haz the power, 2014-04-13 03:05:22]

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

[commit-bot: I haz the power, 2014-04-13 03:05:22]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[mlamouri (slow - plz ping), 2014-04-13 03:15:52]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-13 03:15:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/219463003/110001

[commit-bot: I haz the power, 2014-04-13 03:46:14]

Change committed as 171405