Screen Orientation API: cleanup, simplify and fix implementation.

Source/modules/screen_orientation/ScreenOrientation.cpp

Index: Source/modules/screen_orientation/ScreenOrientation.cpp
diff --git a/Source/modules/screen_orientation/ScreenOrientation.cpp b/Source/modules/screen_orientation/ScreenOrientation.cpp
index 51c609a4e5f5bc9e4251695415cd0473b18db431..d0157a724c479607602bf96a25f93c5be1dcc240 100644
--- a/Source/modules/screen_orientation/ScreenOrientation.cpp
+++ b/Source/modules/screen_orientation/ScreenOrientation.cpp
@@ -10,14 +10,19 @@
 #include "core/frame/Screen.h"
 #include "modules/screen_orientation/ScreenOrientationController.h"
 #include "public/platform/Platform.h"
+#include "public/platform/WebScreenOrientationType.h"
 
-namespace WebCore {
+// This code assume that WebScreenOrientationType values are included in WebScreenOrientationLockType.
+COMPILE_ASSERT((char)blink::WebScreenOrientationPortraitPrimary == (char)blink::WebScreenOrientationLockPortraitPrimary, mismatching_types);

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

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.

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

Done.

+COMPILE_ASSERT((char)blink::WebScreenOrientationPortraitSecondary == (char)blink::WebScreenOrientationLockPortraitSecondary, mismatching_types);
+COMPILE_ASSERT((char)blink::WebScreenOrientationLandscapePrimary == (char)blink::WebScreenOrientationLockLandscapePrimary, mismatching_types);
+COMPILE_ASSERT((char)blink::WebScreenOrientationLandscapeSecondary == (char)blink::WebScreenOrientationLockLandscapeSecondary, mismatching_types);
 
-static const unsigned WebScreenOrientationDefault = 0;
+namespace WebCore {
 
 struct ScreenOrientationInfo {
     const AtomicString& name;
-    blink::WebScreenOrientation orientation;
+    char orientation;

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

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).

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

Done.

 };
 
 static ScreenOrientationInfo* orientationsMap(unsigned& length)
@@ -26,77 +31,74 @@ static ScreenOrientationInfo* orientationsMap(unsigned& length)
     DEFINE_STATIC_LOCAL(const AtomicString, portraitSecondary, ("portrait-secondary", AtomicString::ConstructFromLiteral));
     DEFINE_STATIC_LOCAL(const AtomicString, landscapePrimary, ("landscape-primary", AtomicString::ConstructFromLiteral));
     DEFINE_STATIC_LOCAL(const AtomicString, landscapeSecondary, ("landscape-secondary", AtomicString::ConstructFromLiteral));
+    DEFINE_STATIC_LOCAL(const AtomicString, any, ("any", AtomicString::ConstructFromLiteral));
+    DEFINE_STATIC_LOCAL(const AtomicString, portrait, ("portrait", AtomicString::ConstructFromLiteral));
+    DEFINE_STATIC_LOCAL(const AtomicString, landscape, ("landscape", AtomicString::ConstructFromLiteral));
 
     static ScreenOrientationInfo orientationMap[] = {
-        { portraitPrimary, blink::WebScreenOrientationPortraitPrimary },
-        { portraitSecondary, blink::WebScreenOrientationPortraitSecondary },
-        { landscapePrimary, blink::WebScreenOrientationLandscapePrimary },
-        { landscapeSecondary, blink::WebScreenOrientationLandscapeSecondary }
+        { portraitPrimary, blink::WebScreenOrientationLockPortraitPrimary },
+        { portraitSecondary, blink::WebScreenOrientationLockPortraitSecondary },
+        { landscapePrimary, blink::WebScreenOrientationLockLandscapePrimary },
+        { landscapeSecondary, blink::WebScreenOrientationLockLandscapeSecondary },
+        { any, blink::WebScreenOrientationLockAny },
+        { portrait, blink::WebScreenOrientationLockPortrait },
+        { landscape, blink::WebScreenOrientationLockLandscape }
     };
     length = WTF_ARRAY_LENGTH(orientationMap);
+
     return orientationMap;
 }
 
-static const AtomicString& orientationToString(blink::WebScreenOrientation orientation)
+static const AtomicString& orientationTypeToString(blink::WebScreenOrientationType orientation)
 {
     unsigned length = 0;
     ScreenOrientationInfo* orientationMap = orientationsMap(length);
     for (unsigned i = 0; i < length; ++i) {
-        if (orientationMap[i].orientation == orientation)
+        if (static_cast<blink::WebScreenOrientationType>(orientationMap[i].orientation) == orientation)

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

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.

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

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

             return orientationMap[i].name;
     }
+
     // We do no handle OrientationInvalid and OrientationAny but this is fine because screen.orientation

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

This comment seems outdated now.

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

Removed. The assert should be self explanatory.

     // should never return these and WebScreenOrientation does not define those values.
     ASSERT_NOT_REACHED();
     return nullAtom;
 }
 
-static blink::WebScreenOrientations stringToOrientations(const AtomicString& orientationString)
+static blink::WebScreenOrientationLockType stringToOrientationLock(const AtomicString& orientationLockString)
 {
-    DEFINE_STATIC_LOCAL(const AtomicString, any, ("any", AtomicString::ConstructFromLiteral));
-    DEFINE_STATIC_LOCAL(const AtomicString, portrait, ("portrait", AtomicString::ConstructFromLiteral));
-    DEFINE_STATIC_LOCAL(const AtomicString, landscape, ("landscape", AtomicString::ConstructFromLiteral));
-
-    if (orientationString == any) {
-        return blink::WebScreenOrientationPortraitPrimary | blink::WebScreenOrientationPortraitSecondary |
-            blink::WebScreenOrientationLandscapePrimary | blink::WebScreenOrientationLandscapeSecondary;
-    }
-    if (orientationString == portrait)
-        return blink::WebScreenOrientationPortraitPrimary | blink::WebScreenOrientationPortraitSecondary;
-    if (orientationString == landscape)
-        return blink::WebScreenOrientationLandscapePrimary | blink::WebScreenOrientationLandscapeSecondary;
-
     unsigned length = 0;
     ScreenOrientationInfo* orientationMap = orientationsMap(length);
     for (unsigned i = 0; i < length; ++i) {
-        if (orientationMap[i].name == orientationString)
-            return orientationMap[i].orientation;
+        if (orientationMap[i].name == orientationLockString)
+            return static_cast<blink::WebScreenOrientationLockType>(orientationMap[i].orientation);
     }
-    return 0;
+
+    ASSERT_NOT_REACHED();
+    return blink::WebScreenOrientationLockDefault;
 }
 
 ScreenOrientation::ScreenOrientation(Screen& screen)
     : DOMWindowProperty(screen.frame())
     , m_orientationLockTimer(this, &ScreenOrientation::orientationLockTimerFired)
-    , m_lockedOrientations(WebScreenOrientationDefault)
+    , m_prospectiveLock(blink::WebScreenOrientationLockDefault)
 {
 }
 
-void ScreenOrientation::lockOrientationAsync(blink::WebScreenOrientations orientations)
+void ScreenOrientation::lockOrientationAsync(blink::WebScreenOrientationLockType orientation)
 {
-    if (m_lockedOrientations == orientations)
-        return;
-    m_lockedOrientations = orientations;
-    if (!m_orientationLockTimer.isActive())
-        m_orientationLockTimer.startOneShot(0, FROM_HERE);
+    if (m_orientationLockTimer.isActive())
+        m_orientationLockTimer.stop();
+
+    m_prospectiveLock = orientation;
+    m_orientationLockTimer.startOneShot(0, FROM_HERE);
 }
 
 void ScreenOrientation::orientationLockTimerFired(Timer<ScreenOrientation>*)
 {
-    if (m_lockedOrientations == WebScreenOrientationDefault)
+    if (m_prospectiveLock == blink::WebScreenOrientationLockDefault)
         blink::Platform::current()->unlockOrientation();
     else
-        blink::Platform::current()->lockOrientation(m_lockedOrientations);
+        blink::Platform::current()->lockOrientation(m_prospectiveLock);
 }
 
 const char* ScreenOrientation::supplementName()
@@ -131,24 +133,21 @@ const AtomicString& ScreenOrientation::orientation(Screen& screen)
     ScreenOrientation& screenOrientation = ScreenOrientation::from(screen);
     if (!screenOrientation.document()) {
         // FIXME: we should try to return a better guess, like the latest known value.
-        return orientationToString(blink::WebScreenOrientationPortraitPrimary);
+        return orientationTypeToString(blink::WebScreenOrientationPortraitPrimary);
     }
     ScreenOrientationController& controller = ScreenOrientationController::from(*screenOrientation.document());
-    return orientationToString(controller.orientation());
+    return orientationTypeToString(controller.orientation());
 }
 
-bool ScreenOrientation::lockOrientation(Screen& screen, const AtomicString& orientationString)
+bool ScreenOrientation::lockOrientation(Screen& screen, const AtomicString& lockString)
 {
-    blink::WebScreenOrientations orientations = stringToOrientations(orientationString);
-    if (!orientations)
-        return false;
-    ScreenOrientation::from(screen).lockOrientationAsync(orientations);
+    ScreenOrientation::from(screen).lockOrientationAsync(stringToOrientationLock(lockString));
     return true;
 }
 
 void ScreenOrientation::unlockOrientation(Screen& screen)
 {
-    ScreenOrientation::from(screen).lockOrientationAsync(WebScreenOrientationDefault);
+    ScreenOrientation::from(screen).lockOrientationAsync(blink::WebScreenOrientationLockDefault);
 }
 
 } // namespace WebCore