[screen-orientation] Expose orientation info in OrientationInformation interface.

Source/modules/screen_orientation/ScreenOrientationController.cpp

Index: Source/modules/screen_orientation/ScreenOrientationController.cpp
diff --git a/Source/modules/screen_orientation/ScreenOrientationController.cpp b/Source/modules/screen_orientation/ScreenOrientationController.cpp
index af35b986950871a69d10afc1166fb7977f197568..c8df9c7c25cf1f06c11210238cc314185e5f8087 100644
--- a/Source/modules/screen_orientation/ScreenOrientationController.cpp
+++ b/Source/modules/screen_orientation/ScreenOrientationController.cpp
@@ -5,15 +5,18 @@
 #include "config.h"
 #include "modules/screen_orientation/ScreenOrientationController.h"
 
+#include "core/events/Event.h"
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Screen.h"
 #include "core/page/Page.h"
+#include "modules/screen_orientation/OrientationInformation.h"
 #include "platform/LayoutTestSupport.h"
 #include "platform/PlatformScreen.h"
 #include "public/platform/WebScreenOrientationClient.h"
 
+

[abarth-chromium, 2014/07/02 18:25:10]

This blank line looks spurious

[mlamouri (slow - plz ping), 2014/07/02 19:18:12]

Done.

 namespace WebCore {
 
 ScreenOrientationController::~ScreenOrientationController()
@@ -39,7 +42,7 @@ ScreenOrientationController& ScreenOrientationController::from(LocalFrame& frame
 
 ScreenOrientationController::ScreenOrientationController(LocalFrame& frame, blink::WebScreenOrientationClient* client)
     : PageLifecycleObserver(frame.page())
-    , m_overrideOrientation(blink::WebScreenOrientationUndefined)
+    , m_orientation(new OrientationInformation)
     , m_client(client)
     , m_frame(frame)
 {
@@ -77,41 +80,68 @@ blink::WebScreenOrientationType ScreenOrientationController::computeOrientation(
     }
 }
 
-void ScreenOrientationController::pageVisibilityChanged()
+void ScreenOrientationController::updateOrientation()
 {
-    if (page() && page()->visibilityState() == PageVisibilityStateVisible) {
-        blink::WebScreenOrientationType oldOrientation = m_overrideOrientation;
-        m_overrideOrientation = blink::WebScreenOrientationUndefined;
-        // FIXME: sendOrientationChangeEvent() currently send an event all the
-        // children of the frame, so it should only be called on the frame on
-        // top of the tree. We would need the embedder to call
-        // sendOrientationChangeEvent on every WebFrame part of a WebView to be
-        // able to remove this.
-        if (m_frame == m_frame.localFrameRoot() && oldOrientation != orientation())
-            m_frame.sendOrientationChangeEvent();
-    } else if (m_overrideOrientation == blink::WebScreenOrientationUndefined) {
-        // The page is no longer visible, store the last know screen orientation
-        // so that we keep returning this orientation until the page becomes
-        // visible again.
-        m_overrideOrientation = orientation();
-    }
-}
-
-blink::WebScreenOrientationType ScreenOrientationController::orientation() const
-{
-    if (m_overrideOrientation != blink::WebScreenOrientationUndefined) {
-        // The page is not visible, keep returning the last known screen orientation.
-        ASSERT(!page() || page()->visibilityState() != PageVisibilityStateVisible);
-        return m_overrideOrientation;
-    }
-
     blink::WebScreenOrientationType orientationType = screenOrientationType(m_frame.view());
     if (orientationType == blink::WebScreenOrientationUndefined) {
         // The embedder could not provide us with an orientation, deduce it ourselves.
         orientationType = computeOrientation(m_frame.view());
     }
     ASSERT(orientationType != blink::WebScreenOrientationUndefined);
-    return orientationType;
+
+    m_orientation->setType(orientationType);
+    m_orientation->setAngle(screenOrientationAngle(m_frame.view()));
+}
+
+void ScreenOrientationController::pageVisibilityChanged()
+{
+    if (!page() || page()->visibilityState() != PageVisibilityStateVisible)
+        return;
+
+    OrientationInformation previousOrientation = *m_orientation;

[abarth-chromium, 2014/07/02 18:25:10]

Hum..  We shouldn't keep DOM object as values on the stack.  What happens if
someone ends up with a pointer to them?  Instead, I'd recommend something like:

OrientationInformation* previousOrientation = m_orientation->clone();

[mlamouri (slow - plz ping), 2014/07/02 19:18:13]

Done.

+    updateOrientation();
+
+    // FIXME: sendOrientationChangeEvent() currently send an event all the
+    // children of the frame, so it should only be called on the frame on
+    // top of the tree. We would need the embedder to call
+    // sendOrientationChangeEvent on every WebFrame part of a WebView to be
+    // able to remove this.
+    if (m_frame == m_frame.localFrameRoot() && previousOrientation != *m_orientation)

[abarth-chromium, 2014/07/02 18:25:10]

Here I'd just call !previousOrientation->equals(m_orientation)

[mlamouri (slow - plz ping), 2014/07/02 19:18:12]

Done.

+        ScreenOrientationController::from(m_frame).notifyOrientationChanged();

[abarth-chromium, 2014/07/02 18:25:10]

Can ScreenOrientationController::from(m_frame) return something other than
|this| ?

[mlamouri (slow - plz ping), 2014/07/02 19:18:12]

Done.

+}
+
+void ScreenOrientationController::notifyOrientationChanged()
+{
+    if (!RuntimeEnabledFeatures::orientationEventEnabled() && !RuntimeEnabledFeatures::screenOrientationEnabled())
+        return;
+
+    if (!page() || page()->visibilityState() != PageVisibilityStateVisible)
+        return;
+
+    updateOrientation();

[abarth-chromium, 2014/07/02 18:25:10]

Can we end up calling this function again when notifyOrientationChanged() is
called from ScreenOrientationController::pageVisibilityChanged?

[mlamouri (slow - plz ping), 2014/07/02 19:18:13]

Done. I check changes in pageVisibilityChanged using the angle so we don't call
this method twice.

+
+    LocalDOMWindow* window = m_frame.domWindow();
+    if (!window)
+        return;
+    window->dispatchEvent(Event::create(EventTypeNames::orientationchange));

[abarth-chromium, 2014/07/02 18:25:10]

What happens if script below this point destroys |this| ?

[mlamouri (slow - plz ping), 2014/07/02 19:18:12]

I created the |childFrames| list before notifying the current frame. We use a
RefPtr<> for LocalFrame so we should keep those children alive regardless of
what happens during the event handling, right?

+
+    // Notify subframes.
+    Vector<RefPtr<LocalFrame> > childFrames;
+    for (Frame* child = m_frame.tree().firstChild(); child; child = child->tree().nextSibling()) {
+        if (child->isLocalFrame())
+            childFrames.append(toLocalFrame(child));
+    }
+
+    for (size_t i = 0; i < childFrames.size(); ++i)
+        ScreenOrientationController::from(*childFrames[i]).notifyOrientationChanged();
+}
+
+OrientationInformation* ScreenOrientationController::orientation()
+{
+    if (!m_orientation->initialized())
+        updateOrientation();
+
+    return m_orientation.get();
 }
 
 void ScreenOrientationController::lockOrientation(blink::WebScreenOrientationLockType orientation, blink::WebLockOrientationCallback* callback)
@@ -132,4 +162,10 @@ void ScreenOrientationController::unlockOrientation()
     m_client->unlockOrientation();
 }
 
+void ScreenOrientationController::trace(Visitor* visitor)
+{
+    WillBeHeapSupplement<LocalFrame>::trace(visitor);
+    visitor->trace(m_orientation);
+}
+
 } // namespace WebCore