Stop firing orientationchange events at pages that are not visible

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 e3d75596605f1c4ad16a60b6c633c0f784ba0b7f..a6e645583f9b74fa53c0bd6b5f7c4f58afcec32a 100644
--- a/Source/modules/screen_orientation/ScreenOrientationController.cpp
+++ b/Source/modules/screen_orientation/ScreenOrientationController.cpp
@@ -11,6 +11,7 @@
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Screen.h"
+#include "core/page/Page.h"
 #include "platform/LayoutTestSupport.h"
 #include "platform/PlatformScreen.h"
 
@@ -31,7 +32,9 @@ ScreenOrientationController& ScreenOrientationController::from(Document& documen
 }
 
 ScreenOrientationController::ScreenOrientationController(Document& document)
-    : m_document(document)
+    : PageLifecycleObserver(document.page())

[mlamouri (slow - plz ping), 2014/05/28 14:05:33]

Can a document get detached and attached again?

[Inactive, 2014/05/28 15:27:28]

I do not think so.

+    , m_document(document)
+    , m_lastVisibleOrientation(blink::WebScreenOrientationUndefined)
 {
 }
 
@@ -67,8 +70,25 @@ blink::WebScreenOrientationType ScreenOrientationController::computeOrientation(
     }
 }
 
+void ScreenOrientationController::pageVisibilityChanged()
+{
+    if (page() && page()->visibilityState() == PageVisibilityStateVisible) {
+        m_lastVisibleOrientation = blink::WebScreenOrientationUndefined;

[mlamouri (slow - plz ping), 2014/05/28 14:05:33]

I think it would be appropriate to also fire an event. As far as the web page is
concerned, this is a change in orientation.

[Inactive, 2014/05/28 15:27:28]

Done.

+    } else if (m_lastVisibleOrientation == 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_lastVisibleOrientation = orientation();
+    }
+}
+
 blink::WebScreenOrientationType ScreenOrientationController::orientation() const
 {
+    if (m_lastVisibleOrientation != blink::WebScreenOrientationUndefined) {

[mlamouri (slow - plz ping), 2014/05/28 14:05:33]

I'm not a big fan of side effects like that (ie. m_lastVisibleOrientation is
used as the orientation to use when the page is hidden but also to know if the
page is hidden). I would prefer to be explicit and have something like:
if (!page() || page()->visibilityState() != PageVisibilityStateVisible) {
  return m_lastVisibleOrientation;
}

Then, you could do m_lastVisibleOrientation = orientationType later in that
function.

Speed wise, it should be fairly similar, clarity wise I prefer to be explicit
but it's up to you. Though, if you keep m_lastVisibleOrientation as is, could
you consider renaming it as suggested above?

[Inactive, 2014/05/28 15:27:28]

I renamed the variable to m_overrideOrientation for clarity.
The reason I am not sure about using the check you suggest is that I am worried
it is possible initially that the page is not yet visible and
m_overrideOrientation is still undefined as we haven't changed visibility state.

I also added your check as an assertion to document that we are not visible.

+        // The page is not visible, keep returning the last known screen orientation.
+        return m_lastVisibleOrientation;
+    }
+
     LocalFrame* mainFrame = m_document.frame();
     if (!mainFrame)
         return blink::WebScreenOrientationPortraitPrimary;