Make screen.orientation implementation use WebScreenInfo::orientationType

Make screen.orientation implementation use WebScreenInfo::orientationType

Make screen.orientation implementation use WebScreenInfo::orientationType
instead of Plaform.h's setScreenOrientationListener(). This is better because:
- We can always retrieve a valid screen orientation even before the first
  orientationchange event is fired. i.e. there is no more initialization
  problem.
- The orientation is specific to a Frame instead of being global. This supports
  environment with multiple screens.

Also stop firing the 'orientationchange' event from ScreenOrientationController
anymore and reuse the existing firing code
in LocalFrame::sendOrientationChangeEvent() instead. This avoids firing the
'orientationchange' event twice when screen orientation is enabled. It also
avoids depending on Plaform.h's setScreenOrientationListener() which is
deprecated as it is global instead of per-view (i.e. lack of multi-screen
support).

I tested that the feature is still working on Android target using the
following test app:
http://jsbin.com/IJapIVE/53

R=jochen@chromium.org, mlamouri@chromium.org
BUG=162827
TEST=screen_orientation/orientationchange-event-subframe.html

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

[Inactive, 2014-05-12 19:41:16]

This requires the following Chromium-side change to land first so that
testRunner.setMockScreenOrientation() properly updates
WebScreenInfo::orientationType.

Mounir, is this what you had in mind?

[Inactive, 2014-05-12 19:49:32]

On 2014/05/12 19:41:16, Chris Dumez wrote:
> This requires the following Chromium-side change to land first so that
> testRunner.setMockScreenOrientation() properly updates
> WebScreenInfo::orientationType.

Forgot to paste the URL: https://codereview.chromium.org/284583003/

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

Mounir, I added the fallback mechanism we discussed about when orientationType
is undefined. It seems to work on my Android target.
Please let me know what you think.

Note that the Chromium-side CL (https://codereview.chromium.org/284583003/) is
still needed for the layout tests to pass (because setMockScreenOrientation
needs to be hooked up to WebScreenInfo::orientationType).

[mlamouri (slow - plz ping), 2014-05-15 07:41:51]

On 2014/05/13 15:11:24, Chris Dumez wrote:
> Mounir, I added the fallback mechanism we discussed about when orientationType
> is undefined. It seems to work on my Android target.
> Please let me know what you think.

Thanks for writing this patch! :)

What's your plan to make sure the screen.orientation exposed value is in sync
with the orientationchange event? In other words, we do not want to fire an
event while the page is not visible but we also do not want for the page to be
able to poll for orientation changes. I was hoping that the dispatcher would be
the gateway for that.

[Inactive, 2014-05-15 15:17:21]

On 2014/05/15 07:41:51, Mounir Lamouri wrote:
> On 2014/05/13 15:11:24, Chris Dumez wrote:
> > Mounir, I added the fallback mechanism we discussed about when
orientationType
> > is undefined. It seems to work on my Android target.
> > Please let me know what you think.
> 
> Thanks for writing this patch! :)
> 
> What's your plan to make sure the screen.orientation exposed value is in sync
> with the orientationchange event? In other words, we do not want to fire an
> event while the page is not visible but we also do not want for the page to be
> able to poll for orientation changes. I was hoping that the dispatcher would
be
> the gateway for that.

Two problems:
1. We don't want to fire orientationchange events if the page is not visible
- Proposal: We just add a page visibility check in
LocalFrame::sendOrientationChangeEvent() and fire only if the page is visible

2. screen.orientation should keep returning the old value if the orientation
changes while the page is hidden
- Proposal: Have ScreenOrientationController listen for page visibility events
(like I did in my other patch). When the page becomes invisible, store the
current screen orientation in a class member (e.g.
m_lastKnownScreenOrientation). When the page becomes visible, clear
m_lastKnownScreenOrientation.
When ScreenOrientationController::orientation() is called, return
m_lastKnownScreenOrientation if the page is not visible, use WebScreenInfo
otherwise.

What do you think?

[mlamouri (slow - plz ping), 2014-05-15 16:53:18]

On 2014/05/15 15:17:21, Chris Dumez wrote:
> On 2014/05/15 07:41:51, Mounir Lamouri wrote:
> > On 2014/05/13 15:11:24, Chris Dumez wrote:
> > > Mounir, I added the fallback mechanism we discussed about when
> orientationType
> > > is undefined. It seems to work on my Android target.
> > > Please let me know what you think.
> > 
> > Thanks for writing this patch! :)
> > 
> > What's your plan to make sure the screen.orientation exposed value is in
sync
> > with the orientationchange event? In other words, we do not want to fire an
> > event while the page is not visible but we also do not want for the page to
be
> > able to poll for orientation changes. I was hoping that the dispatcher would
> be
> > the gateway for that.
> 
> Two problems:
> 1. We don't want to fire orientationchange events if the page is not visible
> - Proposal: We just add a page visibility check in
> LocalFrame::sendOrientationChangeEvent() and fire only if the page is visible
> 
> 2. screen.orientation should keep returning the old value if the orientation
> changes while the page is hidden
> - Proposal: Have ScreenOrientationController listen for page visibility events
> (like I did in my other patch). When the page becomes invisible, store the
> current screen orientation in a class member (e.g.
> m_lastKnownScreenOrientation). When the page becomes visible, clear
> m_lastKnownScreenOrientation.
> When ScreenOrientationController::orientation() is called, return
> m_lastKnownScreenOrientation if the page is not visible, use WebScreenInfo
> otherwise.
> 
> What do you think?

I guess if you do that, you could as well always cache the screen orientation
information when ScreenOrientationController gets an orientation change event,
right? It might make things cleaner by having one code path.

[mlamouri (slow - plz ping), 2014-05-15 16:59:09]

On 2014/05/15 16:53:18, Mounir Lamouri wrote:
> I guess if you do that, you could as well always cache the screen orientation
> information when ScreenOrientationController gets an orientation change event,
> right? It might make things cleaner by having one code path.

Hmm, sorry, I forgot that this patch was removing didScreenOrientationChanged().
I thing the behaviour in 2 could be improved by having a simpler code path. It's
unfortunate that the module makes things so complicated. I think that it would
be good to let ScreenOrientationController handle the event.

[Inactive, 2014-05-15 17:10:31]

On 2014/05/15 16:59:09, Mounir Lamouri wrote:
> On 2014/05/15 16:53:18, Mounir Lamouri wrote:
> > I guess if you do that, you could as well always cache the screen
orientation
> > information when ScreenOrientationController gets an orientation change
event,
> > right? It might make things cleaner by having one code path.
> 
> Hmm, sorry, I forgot that this patch was removing
didScreenOrientationChanged().
> I thing the behaviour in 2 could be improved by having a simpler code path.
It's
> unfortunate that the module makes things so complicated. I think that it would
> be good to let ScreenOrientationController handle the event.

As discussed offline, right now it is easy to just let LocalFrame take care of
firing the orientationchange event since it already does and we get this for
free. Later on, we can change this and have the screen orientation module handle
it if we need to. To do this though, I think we'll need a new client interface
so that the module can register itself as a listener for orientationchange (this
is coming from the WebFrame).

For now, I do think this is a step in the right direction, right? It addresses
several of the issue we were having (multi-screen, initialization problem) and
the code is simpler. We don't loose any functionality either and we can get rid
of the platform API that we initially introduced.

[Inactive, 2014-05-19 13:42:32]

+eseidel instead of abarth as I heard Adam is away.

[mlamouri (slow - plz ping), 2014-05-19 14:06:15]

https://codereview.chromium.org/277413002/diff/80001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/277413002/diff/80001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:167: if
(!RuntimeEnabledFeatures::orientationEventEnabled() &&
!RuntimeEnabledFeatures::screenOrientationEnabled())
window.orientation has a bug wrt to orientationchange event that wasn't applying
to screen orientation because of the different implementation. render_view_impl
notifies the main frame about the change of orientation but the main frame
doesn't notify its sub-frames so if you have a frame listening to orientation
changes, it will never be notified.

Maybe we could move the API at the WebView level so the Page could notifies all
the frames? Alternatively, the LocalFrame could notifies its children that would
then notify theirs. I don't know which way would be following Blink common
patterns. I would prefer using Page as a dispatcher FWIW.

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:48: return
blink::WebScreenOrientationPortraitPrimary;
That means we will not get test coverage for this in LayoutTests. Could you
write unit tests then?

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:52: bool
isTallDisplay = rotation % 180 ? rect.height() < rect.width() : rect.height() >
rect.width();
Hmm, I feel like I'm missing something here. Shouldn't that be the opposite?

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:57: return
isTallDisplay ? blink::WebScreenOrientationLandscapePrimary :
blink::WebScreenOrientationPortraitSecondary;
Why did you pick those values here?

[Inactive, 2014-05-19 15:14:29]

https://codereview.chromium.org/277413002/diff/80001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/277413002/diff/80001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:167: if
(!RuntimeEnabledFeatures::orientationEventEnabled() &&
!RuntimeEnabledFeatures::screenOrientationEnabled())
On 2014/05/19 14:06:15, Mounir Lamouri wrote:
> window.orientation has a bug wrt to orientationchange event that wasn't
applying
> to screen orientation because of the different implementation.
render_view_impl
> notifies the main frame about the change of orientation but the main frame
> doesn't notify its sub-frames so if you have a frame listening to orientation
> changes, it will never be notified.
> 
> Maybe we could move the API at the WebView level so the Page could notifies
all
> the frames? Alternatively, the LocalFrame could notifies its children that
would
> then notify theirs. I don't know which way would be following Blink common
> patterns. I would prefer using Page as a dispatcher FWIW.

Ok, I'll take a look. I think we can traverse subframes here and fire the events
on their window as well.

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:48: return
blink::WebScreenOrientationPortraitPrimary;
On 2014/05/19 14:06:15, Mounir Lamouri wrote:
> That means we will not get test coverage for this in LayoutTests. Could you
> write unit tests then?

The issue is that depending on the machine where the tests are ran, the
orientation would be different and thus the tests could fail.

We cannot easily override the screen dimensions from a layout tests. I will
check if it is easier from a unit test (could this be done in a follow-up?).

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:52: bool
isTallDisplay = rotation % 180 ? rect.height() < rect.width() : rect.height() >
rect.width();
On 2014/05/19 14:06:15, Mounir Lamouri wrote:
> Hmm, I feel like I'm missing something here. Shouldn't that be the opposite?

Well, I tested on an Android phone and it worked so it should be correct.

if (rotation % 180) or (rotation % 180 != 0), it means the rotation is either 90
or 270. Since the device is rotated 90 degrees in either direction, the device
has a tall display if the screen height is less than the width (reversed).

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:57: return
isTallDisplay ? blink::WebScreenOrientationLandscapePrimary :
blink::WebScreenOrientationPortraitSecondary;
On 2014/05/19 14:06:15, Mounir Lamouri wrote:
> Why did you pick those values here?

Not sure I understand the question. Those are the same values as from
https://www.captechconsulting.com/blog/eric-miles/programmatically-locking-an...
and they seem to be the common ones.

Could you clarify what you think is wrong or ambiguous?

[Inactive, 2014-05-19 16:05:25]

https://codereview.chromium.org/277413002/diff/80001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/277413002/diff/80001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:167: if
(!RuntimeEnabledFeatures::orientationEventEnabled() &&
!RuntimeEnabledFeatures::screenOrientationEnabled())
On 2014/05/19 14:06:15, Mounir Lamouri wrote:
> window.orientation has a bug wrt to orientationchange event that wasn't
applying
> to screen orientation because of the different implementation.
render_view_impl
> notifies the main frame about the change of orientation but the main frame
> doesn't notify its sub-frames so if you have a frame listening to orientation
> changes, it will never be notified.
> 
> Maybe we could move the API at the WebView level so the Page could notifies
all
> the frames? Alternatively, the LocalFrame could notifies its children that
would
> then notify theirs. I don't know which way would be following Blink common
> patterns. I would prefer using Page as a dispatcher FWIW.

Done and added a layout test to cover this.

[mlamouri (slow - plz ping), 2014-05-20 20:09:25]

Changes in modules/screen_orientation/ LGTM

Thanks! :)

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:48: return
blink::WebScreenOrientationPortraitPrimary;
On 2014/05/19 15:14:30, Chris Dumez wrote:
> On 2014/05/19 14:06:15, Mounir Lamouri wrote:
> > That means we will not get test coverage for this in LayoutTests. Could you
> > write unit tests then?
> 
> The issue is that depending on the machine where the tests are ran, the
> orientation would be different and thus the tests could fail.
> 
> We cannot easily override the screen dimensions from a layout tests. I will
> check if it is easier from a unit test (could this be done in a follow-up?).

Sure, this could be done in a follow-up. We could definitely add something to
override the WebScreenInfo via the test runner. WDYT?

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:52: bool
isTallDisplay = rotation % 180 ? rect.height() < rect.width() : rect.height() >
rect.width();
On 2014/05/19 15:14:30, Chris Dumez wrote:
> On 2014/05/19 14:06:15, Mounir Lamouri wrote:
> > Hmm, I feel like I'm missing something here. Shouldn't that be the opposite?
> 
> Well, I tested on an Android phone and it worked so it should be correct.
> 
> if (rotation % 180) or (rotation % 180 != 0), it means the rotation is either
90
> or 270. Since the device is rotated 90 degrees in either direction, the device
> has a tall display if the screen height is less than the width (reversed).

Sorry, my bad. I got confused with (rotation % 180) meaning (rotation != 0 &&
rotation != 180) instead of (rotation == 0 || rotation == 180).

https://codereview.chromium.org/277413002/diff/80001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:57: return
isTallDisplay ? blink::WebScreenOrientationLandscapePrimary :
blink::WebScreenOrientationPortraitSecondary;
On 2014/05/19 15:14:30, Chris Dumez wrote:
> On 2014/05/19 14:06:15, Mounir Lamouri wrote:
> > Why did you pick those values here?
> 
> Not sure I understand the question. Those are the same values as from
>
https://www.captechconsulting.com/blog/eric-miles/programmatically-locking-an...
> and they seem to be the common ones.
> 
> Could you clarify what you think is wrong or ambiguous?

Nothing was wrong, I was wondering what you used to decide which values should
be set for 90 and 270.

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

https://codereview.chromium.org/277413002/diff/120001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:174: child->sendOrientationChangeEvent();
I would prefer having that in Page to prevent the recursion but I'm not an owner
so I will let the owner decide.

[Inactive, 2014-05-22 20:19:39]

ping review?

[jochen (gone - plz use gerrit), 2014-05-26 10:02:10]

https://codereview.chromium.org/277413002/diff/120001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/orientationchange-event-subframe.html
(right):

https://codereview.chromium.org/277413002/diff/120001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/orientationchange-event-subframe.html:24: return
(currentIndex + 1) % orientations.length;
4space indent everywhere plz

https://codereview.chromium.org/277413002/diff/120001/LayoutTests/screen_orie...
File
LayoutTests/screen_orientation/resources/iframe-listen-orientation-change.html
(right):

https://codereview.chromium.org/277413002/diff/120001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/resources/iframe-listen-orientation-change.html:1:
<script>
<!DOCTYPE html> & 4 space indent

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

https://codereview.chromium.org/277413002/diff/120001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:174: child->sendOrientationChangeEvent();
when iterating over the tree and dispatching events, please first copy the tree
to a Vector<RefPtr<LocalFrame> > and then iterate over that as JS might change
the tree structure.

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

https://codereview.chromium.org/277413002/diff/120001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:48: return
blink::WebScreenOrientationPortraitPrimary;
i thought we use fixed widht/height during layout tests?

[Inactive, 2014-05-26 16:42:38]

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

https://codereview.chromium.org/277413002/diff/120001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:48: return
blink::WebScreenOrientationPortraitPrimary;
On 2014/05/26 10:02:11, jochen wrote:
> i thought we use fixed widht/height during layout tests?

I will double check but last I checked (not long ago) we weren't.

[Inactive, 2014-05-27 14:22:15]

https://codereview.chromium.org/277413002/diff/120001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/orientationchange-event-subframe.html
(right):

https://codereview.chromium.org/277413002/diff/120001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/orientationchange-event-subframe.html:24: return
(currentIndex + 1) % orientations.length;
On 2014/05/26 10:02:11, jochen wrote:
> 4space indent everywhere plz

Done.

https://codereview.chromium.org/277413002/diff/120001/LayoutTests/screen_orie...
File
LayoutTests/screen_orientation/resources/iframe-listen-orientation-change.html
(right):

https://codereview.chromium.org/277413002/diff/120001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/resources/iframe-listen-orientation-change.html:1:
<script>
On 2014/05/26 10:02:11, jochen wrote:
> <!DOCTYPE html> & 4 space indent

Done.

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

https://codereview.chromium.org/277413002/diff/120001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:174: child->sendOrientationChangeEvent();
On 2014/05/26 10:02:11, jochen wrote:
> when iterating over the tree and dispatching events, please first copy the
tree
> to a Vector<RefPtr<LocalFrame> > and then iterate over that as JS might change
> the tree structure.

Good point, sorry I missed that. I fixed it.

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

https://codereview.chromium.org/277413002/diff/120001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:48: return
blink::WebScreenOrientationPortraitPrimary;
On 2014/05/26 16:42:38, Chris Dumez wrote:
> On 2014/05/26 10:02:11, jochen wrote:
> > i thought we use fixed widht/height during layout tests?
> 
> I will double check but last I checked (not long ago) we weren't.

I have just verified that screen.width returns 1920 in the layout test which is
my actual desktop screen width. I don't believe we are using a fixed width /
height in the layout tests.

[jochen (gone - plz use gerrit), 2014-05-27 15:16:18]

i guess it would be nicer if the screen info was fixed during layout tests, but
that shouldn't block this change.

Maybe add a comment where you check the layout test mode that it would be nicer
if the screen info wasn't depending on the computer you run tests on?

lgtm either way

[Inactive, 2014-05-27 15:22:46]

On 2014/05/27 15:16:18, jochen wrote:
> i guess it would be nicer if the screen info was fixed during layout tests,
but
> that shouldn't block this change.
> 
> Maybe add a comment where you check the layout test mode that it would be
nicer
> if the screen info wasn't depending on the computer you run tests on?
> 
> lgtm either way

I added a comment.

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

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-05-27 15:23:23]

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

[commit-bot: I haz the power, 2014-05-27 16:59:33]

Change committed as 174876

[alph, 2014-05-27 18:02:52]

FYI: Broke Oilpan compile:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Oilpan/bu...

[Inactive, 2014-05-27 18:45:04]

On 2014/05/27 18:02:52, alph wrote:
> FYI: Broke Oilpan compile:
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Oilpan/bu...

Uploading build fix at https://codereview.chromium.org/302603005

[Mads Ager (chromium), 2014-05-28 05:20:33]

On 2014/05/27 18:45:04, Chris Dumez wrote:
> On 2014/05/27 18:02:52, alph wrote:
> > FYI: Broke Oilpan compile:
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Oilpan/bu...
> 
> Uploading build fix at https://codereview.chromium.org/302603005

Thanks for fixing this Chris. Alexei, when gardening it is fine to ignore the
oilpan part of the waterfall. At this point they are not considered tree closers
and it is the Oilpan teams responsibility to fix it when it breaks. We *do*
appreciate any help we can get of course. :-)