Stop firing orientationchange events at pages that are not visible

Stop firing orientationchange events at pages that are not visible

Stop firing 'orientationchange' events to pages that are not visible as per the
specification:
https://w3c.github.io/screen-orientation/#handling-screen-orientation-changes

This avoids having background pages handle 'orientationchange' events and
wasting battery.

As indicated in the specification, screen.orientation keeps returning the old
orientation if the orientation changes while the page is not visible.

R=mounir@chromium.org, jochen@chromium.org
BUG=162827
TEST=screen_orientation/page-visibility.html

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

[Inactive, 2014-05-12 15:13:11]

I do not unregister the ScreenOrientationController as an observer when the page
is not visible because I think ScreenOrientationController should keep its
m_orientation member up-to-date. Otherwise, screen.orientation could return an
outdated value if accessed.

Please let me know if you disagree.

[Inactive, 2014-05-12 21:04:25]

On 2014/05/12 15:13:11, Chris Dumez wrote:
> I do not unregister the ScreenOrientationController as an observer when the
page
> is not visible because I think ScreenOrientationController should keep its
> m_orientation member up-to-date. Otherwise, screen.orientation could return an
> outdated value if accessed.
> 
> Please let me know if you disagree.

Arg. I should have read the spec, sorry. It says:
"If the browsing context active document is not visible per [PAGE-VISIBILITY],
these steps have to be aborted and current screen orientation type and current
screen orientation angle MUST continue to return the same values as they
previously did."

So it looks like my assumption was wrong. I will update the CL accordingly.

[Inactive, 2014-05-12 21:23:56]

On 2014/05/12 21:04:25, Chris Dumez wrote:
> On 2014/05/12 15:13:11, Chris Dumez wrote:
> > I do not unregister the ScreenOrientationController as an observer when the
> page
> > is not visible because I think ScreenOrientationController should keep its
> > m_orientation member up-to-date. Otherwise, screen.orientation could return
an
> > outdated value if accessed.
> > 
> > Please let me know if you disagree.
> 
> Arg. I should have read the spec, sorry. It says:
> "If the browsing context active document is not visible per [PAGE-VISIBILITY],
> these steps have to be aborted and current screen orientation type and current
> screen orientation angle MUST continue to return the same values as they
> previously did."
> 
> So it looks like my assumption was wrong. I will update the CL accordingly.

Ok, I updated the patch accordingly. Mounir, could you confirm that this is what
you expected when writing the specification?
Also, what is supposed to happen when the page becomes visible again? Currently
the screen.orientation property may return an outdated value until the next
'orientationchanged' event.

[mlamouri (slow - plz ping), 2014-05-13 05:17:17]

On 2014/05/12 21:23:56, Chris Dumez wrote:
> On 2014/05/12 21:04:25, Chris Dumez wrote:
> > On 2014/05/12 15:13:11, Chris Dumez wrote:
> > > I do not unregister the ScreenOrientationController as an observer when
the
> > page
> > > is not visible because I think ScreenOrientationController should keep its
> > > m_orientation member up-to-date. Otherwise, screen.orientation could
return
> an
> > > outdated value if accessed.
> > > 
> > > Please let me know if you disagree.
> > 
> > Arg. I should have read the spec, sorry. It says:
> > "If the browsing context active document is not visible per
[PAGE-VISIBILITY],
> > these steps have to be aborted and current screen orientation type and
current
> > screen orientation angle MUST continue to return the same values as they
> > previously did."
> > 
> > So it looks like my assumption was wrong. I will update the CL accordingly.
> 
> Ok, I updated the patch accordingly. Mounir, could you confirm that this is
what
> you expected when writing the specification?
> Also, what is supposed to happen when the page becomes visible again?
Currently
> the screen.orientation property may return an outdated value until the next
> 'orientationchanged' event.

Thanks for working on this! :)

Not firing an event and not updating the value when the page isn't visible
sounds good. Updating the value without the event would be odd given that it
would be an observable change (if you poll).

However, when the page is back visible, we should make sure an event is fired
and the value updated. That might be hard to implement with the current design
but using WebScreenInfo, it might be easier.

[Inactive, 2014-05-13 15:23:54]

On 2014/05/13 05:17:17, Mounir Lamouri wrote:
> On 2014/05/12 21:23:56, Chris Dumez wrote:
> > On 2014/05/12 21:04:25, Chris Dumez wrote:
> > > On 2014/05/12 15:13:11, Chris Dumez wrote:
> > > > I do not unregister the ScreenOrientationController as an observer when
> the
> > > page
> > > > is not visible because I think ScreenOrientationController should keep
its
> > > > m_orientation member up-to-date. Otherwise, screen.orientation could
> return
> > an
> > > > outdated value if accessed.
> > > > 
> > > > Please let me know if you disagree.
> > > 
> > > Arg. I should have read the spec, sorry. It says:
> > > "If the browsing context active document is not visible per
> [PAGE-VISIBILITY],
> > > these steps have to be aborted and current screen orientation type and
> current
> > > screen orientation angle MUST continue to return the same values as they
> > > previously did."
> > > 
> > > So it looks like my assumption was wrong. I will update the CL
accordingly.
> > 
> > Ok, I updated the patch accordingly. Mounir, could you confirm that this is
> what
> > you expected when writing the specification?
> > Also, what is supposed to happen when the page becomes visible again?
> Currently
> > the screen.orientation property may return an outdated value until the next
> > 'orientationchanged' event.
> 
> Thanks for working on this! :)
> 
> Not firing an event and not updating the value when the page isn't visible
> sounds good. Updating the value without the event would be odd given that it
> would be an observable change (if you poll).
> 
> However, when the page is back visible, we should make sure an event is fired
> and the value updated. That might be hard to implement with the current design
> but using WebScreenInfo, it might be easier.

Considering we are moving toward using WebScreenInfo, I am proposing to delay
support for this case (event firing when the page becomes visible again) until
the rest of the code gets refactored. What do you think? Is it OK to land this
CL as it is for now and improve the behavior later on?

[mlamouri (slow - plz ping), 2014-05-15 07:43:22]

On 2014/05/13 15:23:54, Chris Dumez wrote:
> Considering we are moving toward using WebScreenInfo, I am proposing to delay
> support for this case (event firing when the page becomes visible again) until
> the rest of the code gets refactored. What do you think? Is it OK to land this
> CL as it is for now and improve the behavior later on?

I think refactoring for WebScreenInfo is more important so we might want to
prioritize for that. This change shouldn't be hard to be done later. However, we
should keep that change in mind while designing the new architecture.

[Inactive, 2014-05-27 19:31:50]

On 2014/05/15 07:43:22, Mounir Lamouri wrote:
> On 2014/05/13 15:23:54, Chris Dumez wrote:
> > Considering we are moving toward using WebScreenInfo, I am proposing to
delay
> > support for this case (event firing when the page becomes visible again)
until
> > the rest of the code gets refactored. What do you think? Is it OK to land
this
> > CL as it is for now and improve the behavior later on?
> 
> I think refactoring for WebScreenInfo is more important so we might want to
> prioritize for that. This change shouldn't be hard to be done later. However,
we
> should keep that change in mind while designing the new architecture.

Now that the refactoring patch landed, I have rebased the patch to use the
approach previously discussed. Please let me know what you think Mounir.

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

Thanks for the patch :)

https://codereview.chromium.org/280283002/diff/100001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/page-visibility.html (right):

https://codereview.chromium.org/280283002/diff/100001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/page-visibility.html:41:
assert_equals(screen.orientation, "portrait-primary");
We should test that an event has been fired too.

https://codereview.chromium.org/280283002/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/280283002/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:170: // Don't fire 'orientationchange' events
at pages that are not visible.
nit: I'm pretty sure the code is self explanatory here ;)

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:35: :
PageLifecycleObserver(document.page())
Can a document get detached and attached again?

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:76:
m_lastVisibleOrientation = blink::WebScreenOrientationUndefined;
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.

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:87: if
(m_lastVisibleOrientation != blink::WebScreenOrientationUndefined) {
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?

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationController.h (right):

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.h:35:
blink::WebScreenOrientationType m_lastVisibleOrientation;
Hmm, with your current implementation, this ought to be called
m_overrideOrientation given that you use that variable to also mark that the
current orientation should be ignored. More later.

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

https://codereview.chromium.org/280283002/diff/100001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/page-visibility.html (right):

https://codereview.chromium.org/280283002/diff/100001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/page-visibility.html:41:
assert_equals(screen.orientation, "portrait-primary");
On 2014/05/28 14:05:33, Mounir Lamouri wrote:
> We should test that an event has been fired too.

Done.

https://codereview.chromium.org/280283002/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/280283002/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:170: // Don't fire 'orientationchange' events
at pages that are not visible.
On 2014/05/28 14:05:33, Mounir Lamouri wrote:
> nit: I'm pretty sure the code is self explanatory here ;)

Done.

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:35: :
PageLifecycleObserver(document.page())
On 2014/05/28 14:05:33, Mounir Lamouri wrote:
> Can a document get detached and attached again?

I do not think so.

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:76:
m_lastVisibleOrientation = blink::WebScreenOrientationUndefined;
On 2014/05/28 14:05:33, Mounir Lamouri wrote:
> 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.

Done.

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:87: if
(m_lastVisibleOrientation != blink::WebScreenOrientationUndefined) {
On 2014/05/28 14:05:33, Mounir Lamouri wrote:
> 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?

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.

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationController.h (right):

https://codereview.chromium.org/280283002/diff/100001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.h:35:
blink::WebScreenOrientationType m_lastVisibleOrientation;
On 2014/05/28 14:05:33, Mounir Lamouri wrote:
> Hmm, with your current implementation, this ought to be called
> m_overrideOrientation given that you use that variable to also mark that the
> current orientation should be ignored. More later.

Done.

[Inactive, 2014-05-28 17:04:31]

+jochen

[Inactive, 2014-05-29 13:54:03]

Mounir, what do you think?

[mlamouri (slow - plz ping), 2014-05-29 14:56:37]

LGTM. Thanks! :)

[Inactive, 2014-05-30 13:22:43]

jochen, could you please take a look as well?

[jochen (gone - plz use gerrit), 2014-06-02 07:25:35]

lgtm

[mlamouri (slow - plz ping), 2014-06-02 09:13:38]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-06-02 09:13:49]

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

[commit-bot: I haz the power, 2014-06-02 09:30:11]

Change committed as 175264