Screen Orientation: fire event on Window instead of Screen.

Screen Orientation: fire event on Window instead of Screen.

This is following spec discussions in public-webapps.

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

[Inactive, 2014-04-17 20:23:16]

On 2014/04/17 20:13:08, Mounir Lamouri wrote:

This bug id looks odd: BUG=241333003

[mlamouri (slow - plz ping), 2014-04-17 20:24:56]

On 2014/04/17 20:23:16, Chris Dumez wrote:
> On 2014/04/17 20:13:08, Mounir Lamouri wrote:
> 
> This bug id looks odd: BUG=241333003

I wonder how the bug id ended up being the CL id. Honestly, that's odd given
that I couldn't know the CL id before creating the CL ;)

[Inactive, 2014-04-17 20:29:48]

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:44:
m_document.domWindow()->dispatchEvent(Event::create(EventTypeNames::orientationchange));
Doesn't this event get fired twice now? (once here and once in
core/frame/LocalFrame.cpp).

It is sad this is not covered by layout tests yet.

[Inactive, 2014-04-17 20:31:41]

https://codereview.chromium.org/241203003/diff/1/Source/core/frame/Screen.idl
File Source/core/frame/Screen.idl (left):

https://codereview.chromium.org/241203003/diff/1/Source/core/frame/Screen.idl...
Source/core/frame/Screen.idl:32: ] interface Screen : EventTarget {
Technically, this change will need approval from an API owner as it is web
exposed and not behind a runtime flag. This was added recently though, did this
ship yet (i.e. is it in M34?).

[mlamouri (slow - plz ping), 2014-04-17 20:34:33]

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:44:
m_document.domWindow()->dispatchEvent(Event::create(EventTypeNames::orientationchange));
On 2014/04/17 20:29:48, Chris Dumez wrote:
> Doesn't this event get fired twice now? (once here and once in
> core/frame/LocalFrame.cpp).
> 
> It is sad this is not covered by layout tests yet.

That is correct. I will make window.orientation implementation entirely based on
screen orientation, at which point it will be fixed. We currently have two ways
to tell the renderer process that the screen orientation has changed. I will
merge those two ways to only one, passing a WebScreenInfo with the updated
values. I have changed WebScreenInfo accordingly and I have patches to populate
WebScreenInfo.

I don't think it's a big deal to have the event sent twice in ToT while this is
being merged.

[mlamouri (slow - plz ping), 2014-04-17 20:36:06]

https://codereview.chromium.org/241203003/diff/1/Source/core/frame/Screen.idl
File Source/core/frame/Screen.idl (left):

https://codereview.chromium.org/241203003/diff/1/Source/core/frame/Screen.idl...
Source/core/frame/Screen.idl:32: ] interface Screen : EventTarget {
On 2014/04/17 20:31:42, Chris Dumez wrote:
> Technically, this change will need approval from an API owner as it is web
> exposed and not behind a runtime flag. This was added recently though, did
this
> ship yet (i.e. is it in M34?).

I doubt this will be a problem but let's add Adam in the reviewers list.

[Inactive, 2014-04-17 20:41:18]

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:44:
m_document.domWindow()->dispatchEvent(Event::create(EventTypeNames::orientationchange));
On 2014/04/17 20:34:33, Mounir Lamouri wrote:
> On 2014/04/17 20:29:48, Chris Dumez wrote:
> > Doesn't this event get fired twice now? (once here and once in
> > core/frame/LocalFrame.cpp).
> > 
> > It is sad this is not covered by layout tests yet.
> 
> That is correct. I will make window.orientation implementation entirely based
on
> screen orientation, at which point it will be fixed. We currently have two
ways
> to tell the renderer process that the screen orientation has changed. I will
> merge those two ways to only one, passing a WebScreenInfo with the updated
> values. I have changed WebScreenInfo accordingly and I have patches to
populate
> WebScreenInfo.
> 
> I don't think it's a big deal to have the event sent twice in ToT while this
is
> being merged.

I'd like to hear Adam's opinion on this. I don't really like that we fire the
event twice during the transition period. Also, having window.orientation depend
on ScreenOrientation might be a bit tricky as ScreenOrientation is behind a
RuntimeEnabled flag (i.e. we'll need to refactoring to make sure the event gets
fired on Window even if the RuntimeEnabled is disabled.

Finally, I was really hoping we would have layout test coverage for screen
orientation events before we change this part of the code.

[abarth-chromium, 2014-04-17 23:36:47]

https://codereview.chromium.org/241203003/diff/1/Source/core/frame/Screen.idl
File Source/core/frame/Screen.idl (left):

https://codereview.chromium.org/241203003/diff/1/Source/core/frame/Screen.idl...
Source/core/frame/Screen.idl:32: ] interface Screen : EventTarget {
On 2014/04/17 20:36:07, Mounir Lamouri wrote:
> I doubt this will be a problem but let's add Adam in the reviewers list.

It seems like a pretty minor change.  It's possible we'll break something, but
I'm not sure worried about it.

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:44:
m_document.domWindow()->dispatchEvent(Event::create(EventTypeNames::orientationchange));
On 2014/04/17 20:41:18, Chris Dumez wrote:
> I'd like to hear Adam's opinion on this. I don't really like that we fire the
> event twice during the transition period.

I think it's fine given that this code is still in the experimental bucket.  We
should make sure we fire it only once before turning the flag on by default,
however.

> Also, having window.orientation depend
> on ScreenOrientation might be a bit tricky as ScreenOrientation is behind a
> RuntimeEnabled flag (i.e. we'll need to refactoring to make sure the event
gets
> fired on Window even if the RuntimeEnabled is disabled.

Yeah, that does seem a bit tricky.  Hopefully we can manager it.

> Finally, I was really hoping we would have layout test coverage for screen
> orientation events before we change this part of the code.

Can we add those tests in this CL?

[mlamouri (slow - plz ping), 2014-04-18 04:06:16]

I have a CL for the test infra: https://codereview.chromium.org/242703002/

[mlamouri (slow - plz ping), 2014-04-18 16:42:17]

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/241203003/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:44:
m_document.domWindow()->dispatchEvent(Event::create(EventTypeNames::orientationchange));
On 2014/04/17 23:36:47, abarth wrote:
> On 2014/04/17 20:41:18, Chris Dumez wrote:
> > I'd like to hear Adam's opinion on this. I don't really like that we fire
the
> > event twice during the transition period.
> 
> I think it's fine given that this code is still in the experimental bucket. 
We
> should make sure we fire it only once before turning the flag on by default,
> however.

That will be the case.

> > Also, having window.orientation depend
> > on ScreenOrientation might be a bit tricky as ScreenOrientation is behind a
> > RuntimeEnabled flag (i.e. we'll need to refactoring to make sure the event
> gets
> > fired on Window even if the RuntimeEnabled is disabled.
> 
> Yeah, that does seem a bit tricky.  Hopefully we can manager it.

Should be fine. ScreenOrientation flag will be enabled on all platforms while
the current window.orientation/window.onorientationchange is only enabled on
Android. onorientationchange will be enabled with ScreenOrientation and
window.orientation will stay behind the other flag. Hopefully, we will be able
to get ride of it at some point too.

> > Finally, I was really hoping we would have layout test coverage for screen
> > orientation events before we change this part of the code.
> 
> Can we add those tests in this CL?

I wrote a CL to improve the infra to write layouttests. I can add tests checking
that the event is sent.

[mlamouri (slow - plz ping), 2014-04-19 01:23:57]

Patch #2 has a test with a TODO for window.onorientationchange. PTAL.

[abarth-chromium, 2014-04-19 16:30:20]

On 2014/04/19 01:23:57, Mounir Lamouri wrote:
> Patch #2 has a test with a TODO for window.onorientationchange. PTAL.

Chris, would you like to review this CL or would you like me to?

[Inactive, 2014-04-19 19:02:02]

On 2014/04/19 16:30:20, abarth wrote:
> On 2014/04/19 01:23:57, Mounir Lamouri wrote:
> > Patch #2 has a test with a TODO for window.onorientationchange. PTAL.
> 
> Chris, would you like to review this CL or would you like me to?

Reviewing it now, thanks.

[Inactive, 2014-04-19 19:05:34]

Mounir, it looks like you forgot to remove the following from
ScreenOrientation.h as well:
DEFINE_STATIC_ATTRIBUTE_EVENT_LISTENER(orientationchange);

As well as the corresponding header include ("core/events/EventTarget.h").

[Inactive, 2014-04-19 19:14:19]

https://codereview.chromium.org/241203003/diff/10001/LayoutTests/screen_orien...
File LayoutTests/screen_orientation/orientationchange-event.html (right):

https://codereview.chromium.org/241203003/diff/10001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/orientationchange-event.html:16: var
eventHandlers = 0;
Maybe "eventHandlerCount"? (Something with "Count" in the name would make this a
bit clearer IMHO).

https://codereview.chromium.org/241203003/diff/10001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/orientationchange-event.html:17: var eventAVL =
0;
What does AVL stand for? we don't use abbreviations in Blink to avoid this kind
of problems.

https://codereview.chromium.org/241203003/diff/10001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/orientationchange-event.html:40:
window.testRunner.setMockScreenOrientation(orientations[getNextIndex()]);
So this fires the 'orientationchange' event synchronously?

[mlamouri (slow - plz ping), 2014-04-19 23:24:20]

I have also updated ScreenOrientation.h to remove the things you pointed.

PTAL.

https://codereview.chromium.org/241203003/diff/10001/LayoutTests/screen_orien...
File LayoutTests/screen_orientation/orientationchange-event.html (right):

https://codereview.chromium.org/241203003/diff/10001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/orientationchange-event.html:16: var
eventHandlers = 0;
On 2014/04/19 19:14:19, Chris Dumez wrote:
> Maybe "eventHandlerCount"? (Something with "Count" in the name would make this
a
> bit clearer IMHO).

I changed the variable name and added a comment.

https://codereview.chromium.org/241203003/diff/10001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/orientationchange-event.html:17: var eventAVL =
0;
On 2014/04/19 19:14:19, Chris Dumez wrote:
> What does AVL stand for? we don't use abbreviations in Blink to avoid this
kind
> of problems.

ditto

https://codereview.chromium.org/241203003/diff/10001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/orientationchange-event.html:40:
window.testRunner.setMockScreenOrientation(orientations[getNextIndex()]);
On 2014/04/19 19:14:19, Chris Dumez wrote:
> So this fires the 'orientationchange' event synchronously?

Correct.

[Inactive, 2014-04-20 00:19:03]

lgtm, thanks.

[mlamouri (slow - plz ping), 2014-04-20 03:04:22]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-20 03:04:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/241203003/30001

[commit-bot: I haz the power, 2014-04-20 03:42:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-20 03:42:02]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[Inactive, 2014-04-20 03:50:47]

On 2014/04/20 03:42:02, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.blink on linux_blink_rel

Failures look legit.

[mlamouri (slow - plz ping), 2014-04-22 13:49:30]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-22 13:49:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/241203003/50001

[commit-bot: I haz the power, 2014-04-22 14:31:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 14:31:23]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[mlamouri (slow - plz ping), 2014-04-22 14:34:22]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-22 14:34:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/241203003/50001

[commit-bot: I haz the power, 2014-04-22 15:06:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 15:06:45]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[mlamouri (slow - plz ping), 2014-04-22 15:24:09]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-22 15:24:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/241203003/50001

[commit-bot: I haz the power, 2014-04-22 16:44:46]

Change committed as 172152