Add initial Blink-side support for the Screen Orientation API

Add initial Blink-side support for the Screen Orientation API

Add initial Blink-side support for the Screen Orientation API:
https://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html

The feature is behind a "ScreenOrientation" runtime-enabled flag. The intent
to implement was sent to blink-dev:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/lgUq8K9IGlU

This CL changes Screen to be an EventTarget. While the Screen Orientation API
has been implemented behind a RuntimeEnabled flag, there is no way of having
inheritance depend on the flag's value. The Web exposed effect of this is that
people will now be able to dispatch custom events on the window.screen
interface, which in itself is harmless.

R=abarth, jochen, kenneth.r.christiansen@intel.com
BUG=162827
TEST=screen_orientation/*

[Inactive, 2014-02-05 16:08:12]

I was working on Screen Orientation API support for blink but it has recently
come to my attention that Google may be working on this as well.

Could you tell us what is the status of this work and what your plans are? Is it
worth for me continuing this work and sending out an intent to implement to
blink-dev?

We care about this API, especially the screen orientation locking capability. I
believe Intel is also interested.

[kenneth.r.christiansen, 2014-02-05 17:33:48]

https://codereview.chromium.org/132113006/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/132113006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.h:23:
OrientationPortraitPrimary = 1,
These arguments might change. Beverloo suggested "current" and
"portrait-preferred" to replace the "-primary" one. Which will lock to the
current portrait orientation, if already in portrait

https://codereview.chromium.org/132113006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.h:41: static bool
lockOrientation(Screen*, const Vector<String>& orientations);
These will probably change to return a Promise. That was already agreed with the
maintainer of the spec

[kenneth.r.christiansen, 2014-02-05 17:41:59]

https://codereview.chromium.org/132113006/diff/1/public/web/WebScreenOrientat...
File public/web/WebScreenOrientationClient.h (right):

https://codereview.chromium.org/132113006/diff/1/public/web/WebScreenOrientat...
public/web/WebScreenOrientationClient.h:18: virtual void
lockOrientation(WebScreenOrientations) = 0;
The spec (editor draft at least) says that if any of the orientations isnt
supported you should return false. (or in the future, make the Promise fail)

[Inactive, 2014-02-05 18:22:44]

Sure, if the spec is updated to use Promises or change the argument strings, I
will happily update the implementation.

Right now, this reflects the latest public draft.

https://codereview.chromium.org/132113006/diff/1/public/web/WebScreenOrientat...
File public/web/WebScreenOrientationClient.h (right):

https://codereview.chromium.org/132113006/diff/1/public/web/WebScreenOrientat...
public/web/WebScreenOrientationClient.h:18: virtual void
lockOrientation(WebScreenOrientations) = 0;
On 2014/02/05 17:42:00, kenneth.r.christiansen wrote:
> The spec (editor draft at least) says that if any of the orientations isnt
> supported you should return false. (or in the future, make the Promise fail)

Done.

[abarth-chromium, 2014-02-05 19:14:48]

miguelg is probably a good person to talked with about this topic.

[Miguel Garcia, 2014-02-06 09:36:11]

On 2014/02/05 19:14:48, abarth wrote:
> miguelg is probably a good person to talked with about this topic.

Yes, Mounir and John have been looking into this API as well and were planning
on sending an intent to implement in blink now that the spec is getting stable.
I would love if we could do this as a joint effort. Would you be ok if we'd send
a patch for the android implementation side while you work on landing this
patch?

[kenneth.r.christiansen, 2014-02-06 15:05:13]

On 2014/02/06 09:36:11, Miguel Garcia wrote:
> On 2014/02/05 19:14:48, abarth wrote:
> > miguelg is probably a good person to talked with about this topic.
> 
> Yes, Mounir and John have been looking into this API as well and were planning
> on sending an intent to implement in blink now that the spec is getting
stable.

The spec wasn't updated in a while, but I know there were talks about changing
the arguments and returning a Promise, as I participated in those discussions.

Do you have an update on the latest agreements? upcoming changes?

> I would love if we could do this as a joint effort. Would you be ok if we'd
send
> a patch for the android implementation side while you work on landing this
> patch?

The Chromium side is here: https://codereview.chromium.org/150153004/

[Inactive, 2014-02-06 20:53:01]

On 2014/02/06 09:36:11, Miguel Garcia wrote:
> On 2014/02/05 19:14:48, abarth wrote:
> > miguelg is probably a good person to talked with about this topic.
> 
> Yes, Mounir and John have been looking into this API as well and were planning
> on sending an intent to implement in blink now that the spec is getting
stable.
> I would love if we could do this as a joint effort. Would you be ok if we'd
send
> a patch for the android implementation side while you work on landing this
> patch?

Hi, sure I think it would be awesome if this could be a joint effort! We are OK
letting you guys handle the Android implementation part. As you can see, Slava
already did some work on this (https://codereview.chromium.org/150153004/) but
we are happy to let you take over that part / patch if we focus on the Blink
side.
Can we send the intent to implement or will you? If you do, please put slava and
my emails on there as well.

[mlamouri (slow - plz ping), 2014-02-10 18:20:57]

Some drive-by comments.

https://codereview.chromium.org/132113006/diff/290001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/screenorientation-api.html (right):

https://codereview.chromium.org/132113006/diff/290001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/screenorientation-api.html:10:
shouldBeDefined("screen.orientation");
Shouldn't you add a check for screen.onorientationchange?

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:144: return false;
What are you trying to do here? It sounds like you check that the lock is made
on a subset of the current lock. That would not be the right thing to do. We
could do some checks like early return true if we try to lock to the same thing
as the current lock but I am not sure it would be really needed.

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:156:
m_lockedOrientations &= orientations;
That doesn't sound needed.

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
File Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp
(right):

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp:58:
m_lockedOrientations = orientations;
Shouldn't you change the screen orientation asynchronously after that call? If
the page is in portrait mode and then get locks to landscape, screen.orientation
shouldn't return 'portrait-*'.

[Inactive, 2014-02-10 22:25:24]

https://codereview.chromium.org/132113006/diff/290001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/screenorientation-api.html (right):

https://codereview.chromium.org/132113006/diff/290001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/screenorientation-api.html:10:
shouldBeDefined("screen.orientation");
On 2014/02/10 18:20:57, Mounir Lamouri wrote:
> Shouldn't you add a check for screen.onorientationchange?

Makes sense, Done.

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:144: return false;
On 2014/02/10 18:20:57, Mounir Lamouri wrote:
> What are you trying to do here? It sounds like you check that the lock is made
> on a subset of the current lock. That would not be the right thing to do. We
> could do some checks like early return true if we try to lock to the same
thing
> as the current lock but I am not sure it would be really needed.

This was removed in a later iteration. This part of the code was not ready.

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:156:
m_lockedOrientations &= orientations;
On 2014/02/10 18:20:57, Mounir Lamouri wrote:
> That doesn't sound needed.

Right, should be a simple assignment. Done.

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
File Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp
(right):

https://codereview.chromium.org/132113006/diff/290001/Source/modules/screen_o...
Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp:58:
m_lockedOrientations = orientations;
On 2014/02/10 18:20:57, Mounir Lamouri wrote:
> Shouldn't you change the screen orientation asynchronously after that call? If
> the page is in portrait mode and then get locks to landscape,
screen.orientation
> shouldn't return 'portrait-*'.

You are right, I fixed it in a later iteration, but not on client side. This is
handled in ScreenOrientation.cpp, the client's lockOrientation() method is now
called asynchronously. unlockOrientation() is still called synchronously though,
I am waiting for clarification on the spec to know if it should be async as
well.

[Inactive, 2014-02-11 20:17:57]

The patch is now complete and ready for feedback, please take a look.

I hope we can land an implementation matching the current spec and then update
it with the spec.

[kenneth.r.christiansen, 2014-02-12 13:42:36]

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/lockOrientation-array-argument.html (right):

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/lockOrientation-array-argument.html:6:
description("Validates that lockOrientation() can take an array in argument.");
nit: as argument

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/lockOrientation-array-argument.html:46:
setTimeout(changeOrientation, 100);
no way to avoid the time out? isnt your mock object instant?

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/locking-then-unlocking.html (right):

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/locking-then-unlocking.html:47:
shouldBeEqualToString("screen.orientation", "portrait-primary");
according to the spec a platform can say that it only supports a subset of the
orientations and it should fail if the lock includes orientations not supported.
ie it might only support ['landscape-primary']. I guess you will handle this
from chromium side, but maybe it could be tested with the mock as well?

https://codereview.chromium.org/132113006/diff/650001/Source/core/frame/Scree...
File Source/core/frame/Screen.idl (right):

https://codereview.chromium.org/132113006/diff/650001/Source/core/frame/Scree...
Source/core/frame/Screen.idl:30: interface Screen : EventTarget {
So we are making it an EventTarget and there is no runtime flag to turn that
off... not sure if that is possible. As this is an intent to implement, we need
it to be a runtime flag

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:50: // New page loaded,
unlock screen orientation.
so basically this should return to the default UA orientations.

I was told that some UAs might default to a small default set but allow locking
to a larger set.

ie. default: [portrait-primary, landscape] allowing 3 orientations

but you could still do lockOrientation([portrait, landscape]) allowing four
orientations. unlock should then restore UA defaults.

I was told that this is useful for phones who most probably will disable
portrait-secondary by default because of hardware keys, but still allow people
to opt into enabling it

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:200: if
(m_lockedOrientations == OrientationAny)
This is what might be wrong depending on the default UA allowed orientations

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.h:44: static bool
lockOrientation(Screen*, const Vector<String>& orientations);
I wonder if you some way need to know which orientations are actually supported
to be able to fail or ignore locks to those

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

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:14: ,
m_orientation(OrientationPortraitPrimary)
So this might actually be wrong... how are you making sure it is updated
properly before queried?

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
File Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp
(right):

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp:43: if
(!m_controllerTimer.isActive())
maybe for the mock this doesnt have to be async?

[Inactive, 2014-02-12 16:03:10]

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/lockOrientation-array-argument.html (right):

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/lockOrientation-array-argument.html:6:
description("Validates that lockOrientation() can take an array in argument.");
On 2014/02/12 13:42:37, kenneth.r.christiansen wrote:
> nit: as argument 

Done.

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/lockOrientation-array-argument.html:46:
setTimeout(changeOrientation, 100);
On 2014/02/12 13:42:37, kenneth.r.christiansen wrote:
> no way to avoid the time out? isnt your mock object instant?

We need to give the events a chance to fire. Events are fired asynchronously so
I don't think we can avoid the timeout.

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/locking-then-unlocking.html (right):

https://codereview.chromium.org/132113006/diff/650001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/locking-then-unlocking.html:47:
shouldBeEqualToString("screen.orientation", "portrait-primary");
On 2014/02/12 13:42:37, kenneth.r.christiansen wrote:
> according to the spec a platform can say that it only supports a subset of the
> orientations and it should fail if the lock includes orientations not
supported.
> ie it might only support ['landscape-primary']. I guess you will handle this
> from chromium side, but maybe it could be tested with the mock as well?

Yes, this has to happen on Chromium side as this may differ on the various
supported platforms. Also, since the locking is asynchronous, we don't currently
have a way to tell the JS that the locking failed (i.e. lockOrientation() cannot
return false).
This will be solved when we switched to Promises though.

https://codereview.chromium.org/132113006/diff/650001/Source/core/frame/Scree...
File Source/core/frame/Screen.idl (right):

https://codereview.chromium.org/132113006/diff/650001/Source/core/frame/Scree...
Source/core/frame/Screen.idl:30: interface Screen : EventTarget {
On 2014/02/12 13:42:37, kenneth.r.christiansen wrote:
> So we are making it an EventTarget and there is no runtime flag to turn that
> off... not sure if that is possible. As this is an intent to implement, we
need
> it to be a runtime flag

I don't believe there is a way to alter JS prototype inheritance at runtime. I
think this impact of this change is low. Yes it adds a couple of methods on the
screen object but these won't have an effect unless the runtime flag is enabled.

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.h:44: static bool
lockOrientation(Screen*, const Vector<String>& orientations);
On 2014/02/12 13:42:37, kenneth.r.christiansen wrote:
> I wonder if you some way need to know which orientations are actually
supported
> to be able to fail or ignore locks to those

This would mean having lockOrientation() request Chromium synchronously which
orientations the implementation supports. I don't think we want to do this right
now. We know the API is likely going to switch to Promises and we won't need to
ask Chromium which orientation it supports then. We will just fail
asynchronously and the JS side will have a way to know it failed.

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

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:14: ,
m_orientation(OrientationPortraitPrimary)
On 2014/02/12 13:42:37, kenneth.r.christiansen wrote:
> So this might actually be wrong... how are you making sure it is updated
> properly before queried?

Yes, this is not optimal. I haven't given much thought into how this should work
yet. My guess is that we'll need a way to ask Chromium what the current
orientation is without having to wait for the first orientation change event. I
will give this more thought. I don't know if this needs to be fixed in this
initial CL already. Maybe we can just have a FIXME comment for now?

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
File Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp
(right):

https://codereview.chromium.org/132113006/diff/650001/Source/modules/screen_o...
Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp:43: if
(!m_controllerTimer.isActive())
On 2014/02/12 13:42:37, kenneth.r.christiansen wrote:
> maybe for the mock this doesnt have to be async?

Hmm, I did this because calling setScreenOrientation() from JS side can cause an
orientationchange event to be fired. We want this event to be fired
asynchronously. This timer makes sure the event is fired asynchronously.

[kenneth.r.christiansen, 2014-02-12 16:35:45]

On 2014/02/12 16:03:10, Chris Dumez wrote:
> We need to give the events a chance to fire. Events are fired asynchronously
so
> I don't think we can avoid the timeout.

I see, but as we can avoid the timeouts when we move to Promises I think this is
OK for now.

> Yes, this has to happen on Chromium side as this may differ on the various
> supported platforms. Also, since the locking is asynchronous, we don't
currently
> have a way to tell the JS that the locking failed (i.e. lockOrientation()
cannot
> return false).
> This will be solved when we switched to Promises though.

Right

> I don't believe there is a way to alter JS prototype inheritance at runtime. I
> think this impact of this change is low. Yes it adds a couple of methods on
the
> screen object but these won't have an effect unless the runtime flag is
enabled.

It does allow people to call dispatchEvent etc on window.screen. It might not
matter, but you could do a separate Intent to Implement/Ship for the
EventTarget. Screen Orientation is on the roadmap for the year and it might be
possible to ship the EventTarget change already.


> This would mean having lockOrientation() request Chromium synchronously which
> orientations the implementation supports. 

Or be told when it creates the Page. Maybe this doesn't matter when we move to
the Promise based version, so let's forget about it for now.

> Yes, this is not optimal. I haven't given much thought into how this should
work
> yet. My guess is that we'll need a way to ask Chromium what the current
> orientation is without having to wait for the first orientation change event.
I
> will give this more thought. I don't know if this needs to be fixed in this
> initial CL already. Maybe we can just have a FIXME comment for now?

I think that is fine. Please add that.

> Hmm, I did this because calling setScreenOrientation() from JS side can cause
an
> orientationchange event to be fired. We want this event to be fired
> asynchronously. This timer makes sure the event is fired asynchronously.

Right

[kenneth.r.christiansen, 2014-02-12 16:51:03]

lgtm, but needs API owners review

[Peter Beverloo, 2014-02-12 18:19:36]

Thanks for the patch, Chris! Hereby a few comments.

https://codereview.chromium.org/132113006/diff/920001/Source/core/frame/Scree...
File Source/core/frame/Screen.idl (right):

https://codereview.chromium.org/132113006/diff/920001/Source/core/frame/Scree...
Source/core/frame/Screen.idl:30: interface Screen : EventTarget {
As Kenneth pointed out, since this enables events to be dispatched on the
window.screen interface (regardless of the value of the flag), please clarify
that it does so in the CL description.  I think it's harmless enough, but it is
a Web exposed change so being overly communicative is a good thing.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/OWNERS (right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/OWNERS:3: mlamouri@chromium.org
nit: Since Mounir isn't a committer yet, it may be best to include either me or
johnme@ until he is.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:24: ASSERT(screen);
We'd already have crashed if !screen, since you're calling Screen::frame() on it
as part of the DOMWindowProperty initialization.  Under what circumstances would
!screen occur?

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:82: supplement = new
ScreenOrientation(screen);
If you do need to ASSERT(screen), this would be a nice place to do it since the
constructor is private.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:123:
ASSERT_NOT_REACHED();
nit: maybe add a note that OrientationInvalid and OrientationAny are not being
handled, but that that's fine because the incoming value needs to be in the
WebScreenOrientation enum as well, which doesn't define them?

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

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.h:24: static const
char* supplementName();
nit: add a comment to indicate that these implement the Supplement<Page> API?

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/testing/InternalsScreenOrientation.cpp
(right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/testing/InternalsScreenOrientation.cpp:19:
ScreenOrientationClientMock* client = new ScreenOrientationClientMock();
What owns this client? Won't we leak it?

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/testing/InternalsScreenOrientation.cpp:19:
ScreenOrientationClientMock* client = new ScreenOrientationClientMock();
What/who owns this client? Won't we leak it?

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/testing/InternalsScreenOrientation.h
(right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/testing/InternalsScreenOrientation.h:8:
#include "wtf/text/AtomicString.h"
nit: You can forward declare AtomicString as well since you're only using it as
a reference.

https://codereview.chromium.org/132113006/diff/920001/Source/web/WebScreenOri...
File Source/web/WebScreenOrientationController.cpp (right):

https://codereview.chromium.org/132113006/diff/920001/Source/web/WebScreenOri...
Source/web/WebScreenOrientationController.cpp:14:
m_private->orientationChanged(static_cast<WebCore::ScreenOrientationValue>(orientation));
Can we add some COMPILE_ASSERTS to make sure that the WebScreenOrientation
values stay in sync with ScreenOrientationValue?

https://codereview.chromium.org/132113006/diff/920001/public/web/WebScreenOri...
File public/web/WebScreenOrientationController.h (right):

https://codereview.chromium.org/132113006/diff/920001/public/web/WebScreenOri...
public/web/WebScreenOrientationController.h:31: // can be created by the
consumers of Chromium WebKit.
s/Chromium WebKit/Blink/

[Inactive, 2014-02-12 19:38:36]

https://codereview.chromium.org/132113006/diff/920001/Source/core/frame/Scree...
File Source/core/frame/Screen.idl (right):

https://codereview.chromium.org/132113006/diff/920001/Source/core/frame/Scree...
Source/core/frame/Screen.idl:30: interface Screen : EventTarget {
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> As Kenneth pointed out, since this enables events to be dispatched on the
> window.screen interface (regardless of the value of the flag), please clarify
> that it does so in the CL description.  I think it's harmless enough, but it
is
> a Web exposed change so being overly communicative is a good thing.

Done.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/OWNERS (right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/OWNERS:3: mlamouri@chromium.org
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> nit: Since Mounir isn't a committer yet, it may be best to include either me
or
> johnme@ until he is.

Done.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:24: ASSERT(screen);
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> We'd already have crashed if !screen, since you're calling Screen::frame() on
it
> as part of the DOMWindowProperty initialization.  Under what circumstances
would
> !screen occur?

Good point, I will move this assertion to the caller.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:82: supplement = new
ScreenOrientation(screen);
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> If you do need to ASSERT(screen), this would be a nice place to do it since
the
> constructor is private.

Done.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:123:
ASSERT_NOT_REACHED();
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> nit: maybe add a note that OrientationInvalid and OrientationAny are not being
> handled, but that that's fine because the incoming value needs to be in the
> WebScreenOrientation enum as well, which doesn't define them?

Done.

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

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.h:24: static const
char* supplementName();
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> nit: add a comment to indicate that these implement the Supplement<Page> API?

Done.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/testing/InternalsScreenOrientation.cpp
(right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/testing/InternalsScreenOrientation.cpp:19:
ScreenOrientationClientMock* client = new ScreenOrientationClientMock();
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> What/who owns this client? Won't we leak it?

You are right, this was leaked. This is now deleted by the
ScreenOrientationController's destructor.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/testing/InternalsScreenOrientation.h
(right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/testing/InternalsScreenOrientation.h:8:
#include "wtf/text/AtomicString.h"
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> nit: You can forward declare AtomicString as well since you're only using it
as
> a reference.

Done. I included "wtf/Forward.h".

https://codereview.chromium.org/132113006/diff/920001/Source/web/WebScreenOri...
File Source/web/WebScreenOrientationController.cpp (right):

https://codereview.chromium.org/132113006/diff/920001/Source/web/WebScreenOri...
Source/web/WebScreenOrientationController.cpp:14:
m_private->orientationChanged(static_cast<WebCore::ScreenOrientationValue>(orientation));
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> Can we add some COMPILE_ASSERTS to make sure that the WebScreenOrientation
> values stay in sync with ScreenOrientationValue?

Good idea, Done.

https://codereview.chromium.org/132113006/diff/920001/public/web/WebScreenOri...
File public/web/WebScreenOrientationController.h (right):

https://codereview.chromium.org/132113006/diff/920001/public/web/WebScreenOri...
public/web/WebScreenOrientationController.h:31: // can be created by the
consumers of Chromium WebKit.
On 2014/02/12 18:19:37, Peter Beverloo wrote:
> s/Chromium WebKit/Blink/

Done.

[mlamouri (slow - plz ping), 2014-02-12 20:11:25]

The tests are great. So many of them! :) However, I am a bit concerned with your
usage of setTimeout. I do not think you should be using setTimeout(foo, x) with
x != 0. You seem to be using 100 nearly all the time. Why is that? Wouldn't
setTimeout(foo) simply work (x=0 by default)? Also, you use setTimeout(foo, 200)
once. This is definitely a bad idea. If you want to go twice in the event loop
before checking something, you could nest two setTimeouts.

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/lockOrientation-bad-array-argument.html
(right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/lockOrientation-bad-array-argument.html:47:
shouldBeFalse("screen.lockOrientation(['portrait-primary',
'invalid-orientation'])");
Maybe you could test that false is returned with different values that just an
invalid string?

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/locked-no-orientation-change-event.html
(right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/locked-no-orientation-change-event.html:6:
description("Validates that no orientationchange event is fired when we are
locked.");
This test seems to essentially test the mock object. It is the role of the
backend to make sure the orientation does not change. If the
ScreenOrientationController gets an orientationChanged call for
'portrait-secondary' while the screen is locked to 'portrait-primary', Blink
couldn't do much to fix the situation: the screen orientation would have changed
and not firing the event might not even be the right thing to do.

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/locking-failure.html (right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/locking-failure.html:24:
shouldThrow("screen.lockOrientation()", '"TypeError: Failed to execute
\'lockOrientation\' on \'Screen\': 1 argument required, but only 0 present."');
Could you add tests that pass some garbage values like special values (null,
undefined), objects, integers and strings that are different from the 6 known
keywords?

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/screenorientation-api.html (right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/screenorientation-api.html:6:
description("Validates the Screen Orientation API is exposed.");
You might want to check that Screen is an EventTarget too. Though, this could be
done in a test specific to Screen.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:188:
m_orientationLockTimer.startOneShot(0);
I do not think having this call async here is really useful. Given that we will
likely cross process boundaries, locking will be async anyway. What we really
care about is that the orientationchange event is sent asynchronously compared
to the lockOrientation() call. Why not specifying this in
WebScreenOrientationClient to make sure the implementation keep
lockOrientation() async?

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:201: return; // Not
currently locked.
This is actually a false assumption. I believe that the specification says that
unlockOrientation() should revert the lock to the default which can be anything.
For example, on most Android applications, it will be: [ 'portrait-primary',
'landscape' ].

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:203:
m_lockedOrientations = OrientationAny;
Generally speaking, I think you can get ride of m_lockedOrientations. It does
not seem needed.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:203:
m_lockedOrientations = OrientationAny;
I

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.h:32: OrientationAny =
OrientationPortrait | OrientationLandscape
I am not sure that you want to define that. See below.

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

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:15: // event
is fired.
What could we do to fix that? Would that be reasonable to mark m_orientation as
unset so if orientation is requested, it could be explicitly requested unless a
new value was provided?

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:16: ,
m_orientation(OrientationPortraitPrimary)
Does that mean that if a page is loaded in another orientation than
'portrait-primary', an orientationchange event will be fired even if the
orientation never changed?

[Inactive, 2014-02-12 21:08:03]

used setTimeout(, 0) everywhere.

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/lockOrientation-bad-array-argument.html
(right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/lockOrientation-bad-array-argument.html:47:
shouldBeFalse("screen.lockOrientation(['portrait-primary',
'invalid-orientation'])");
On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> Maybe you could test that false is returned with different values that just an
> invalid string?

Done.

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/locked-no-orientation-change-event.html
(right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/locked-no-orientation-change-event.html:6:
description("Validates that no orientationchange event is fired when we are
locked.");
On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> This test seems to essentially test the mock object. It is the role of the
> backend to make sure the orientation does not change. If the
> ScreenOrientationController gets an orientationChanged call for
> 'portrait-secondary' while the screen is locked to 'portrait-primary', Blink
> couldn't do much to fix the situation: the screen orientation would have
changed
> and not firing the event might not even be the right thing to do.

So you want to remove this layout test?

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/locking-failure.html (right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/locking-failure.html:24:
shouldThrow("screen.lockOrientation()", '"TypeError: Failed to execute
\'lockOrientation\' on \'Screen\': 1 argument required, but only 0 present."');
On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> Could you add tests that pass some garbage values like special values (null,
> undefined), objects, integers and strings that are different from the 6 known
> keywords?

Done.

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/screenorientation-api.html (right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/screenorientation-api.html:6:
description("Validates the Screen Orientation API is exposed.");
On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> You might want to check that Screen is an EventTarget too. Though, this could
be
> done in a test specific to Screen.

Done.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:188:
m_orientationLockTimer.startOneShot(0);
On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> I do not think having this call async here is really useful. Given that we
will
> likely cross process boundaries, locking will be async anyway. What we really
> care about is that the orientationchange event is sent asynchronously compared
> to the lockOrientation() call. Why not specifying this in
> WebScreenOrientationClient to make sure the implementation keep
> lockOrientation() async?

I am not sure about this one. I think this is a quite common way of making sure
we do the task asynchronously, as mandated by the spec. I am slightly worried
about making assumptions about crossing process boundaries. As you can see
already, it is not the case for my mock client and I would have to move this
Timer there anyway.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:201: return; // Not
currently locked.
On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> This is actually a false assumption. I believe that the specification says
that
> unlockOrientation() should revert the lock to the default which can be
anything.
> For example, on most Android applications, it will be: [ 'portrait-primary',
> 'landscape' ].

Well, we are not locked due to JS. Why pass call unlockOrientation() on the
client if the JS did not previously lock the orientation?

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:203:
m_lockedOrientations = OrientationAny;
On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> Generally speaking, I think you can get ride of m_lockedOrientations. It does
> not seem needed.

I need it as long as I keep locking asynchronously via the
m_orientationLockTimer timer. See my comment about why I'd prefer to keep the
Timer.

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

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:15: // event
is fired.
On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> What could we do to fix that? Would that be reasonable to mark m_orientation
as
> unset so if orientation is requested, it could be explicitly requested unless
a
> new value was provided?

Yes, we could then initialize m_orientation to OrientationInvalid and request
the orientation from the client if it is requested and m_orientation is invalid.
Sadly, this would have to be synchronous, right? (since screen.orientation
returns the orientation synchronously).

However, right now I have no support for requesting the orientation from the
client so I have to initialize it to something valid.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:16: ,
m_orientation(OrientationPortraitPrimary)
On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> Does that mean that if a page is loaded in another orientation than
> 'portrait-primary', an orientationchange event will be fired even if the
> orientation never changed?

I don't think it would, as there would be no orientation change according to the
client. However, the issue would be if the device is then rotated from
'landscape-primary' to 'portrait-primary'. We likely won't fire an event because
the controller will think we are already in "portrait-primary". So yes, we have
to handle this better. I think initializing to an invalid / unknown orientation
is a good idea. The question is what should I do if screen.orientation is
accessed and I do not know the current orientation. I think it would be bad to
add a method to request the orientation from the client as this would mean a
synchronous call and this is likely to happen on application startup.

[kenneth.r.christiansen, 2014-02-13 09:40:53]

> Yes, we could then initialize m_orientation to OrientationInvalid and request
> the orientation from the client if it is requested and m_orientation is
invalid.
> Sadly, this would have to be synchronous, right? (since screen.orientation
> returns the orientation synchronously).
> 
> However, right now I have no support for requesting the orientation from the
> client so I have to initialize it to something valid.

I guess the question is in which cases you need to update this?

1) Page/Mainframe got created and loaded (could be set as part of the page
initial properties)
2) Visibility changed, so we probably don't want to send the emails to pages
which are hidden, so they should fire when it becomes visible
3) Potentially (with HDMI, Miracast): Window got moved to a new screen, or the
screen changed.

We already transfer a lot of info when we create the page, so maybe it is not so
bad to do the same.

[Inactive, 2014-02-13 15:47:29]

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
Source/modules/screen_orientation/ScreenOrientationController.cpp:14: // FIXME:
This orientation is not necessary the right one until the first
orientationchange
I would propose to address this issue in a follow-up patch as I need more time
to investigate and the CL is already big.

[mlamouri (slow - plz ping), 2014-02-13 16:12:07]

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/locked-no-orientation-change-event.html
(right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/locked-no-orientation-change-event.html:6:
description("Validates that no orientationchange event is fired when we are
locked.");
On 2014/02/12 21:08:04, Chris Dumez wrote:
> On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> > This test seems to essentially test the mock object. It is the role of the
> > backend to make sure the orientation does not change. If the
> > ScreenOrientationController gets an orientationChanged call for
> > 'portrait-secondary' while the screen is locked to 'portrait-primary', Blink
> > couldn't do much to fix the situation: the screen orientation would have
> changed
> > and not firing the event might not even be the right thing to do.
> 
> So you want to remove this layout test?

I guess we could remove it yes. There is no point in keeping that test and it
might give the wrong impression that this behaviour is actually tested.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:188:
m_orientationLockTimer.startOneShot(0);
On 2014/02/12 21:08:04, Chris Dumez wrote:
> On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> > I do not think having this call async here is really useful. Given that we
> will
> > likely cross process boundaries, locking will be async anyway. What we
really
> > care about is that the orientationchange event is sent asynchronously
compared
> > to the lockOrientation() call. Why not specifying this in
> > WebScreenOrientationClient to make sure the implementation keep
> > lockOrientation() async?
> 
> I am not sure about this one. I think this is a quite common way of making
sure
> we do the task asynchronously, as mandated by the spec. I am slightly worried
> about making assumptions about crossing process boundaries. As you can see
> already, it is not the case for my mock client and I would have to move this
> Timer there anyway.

I guess I will let the Webkit/Blink experienced folks decide here. Though, I am
not saying that IPC will do the job just that we should let the embedder take
care of the asynchronous part. Forcing the Blink bits to be async would simply
slow down things IMO. It might be bad for responsiveness. Also, note that
unlockOrientation() does not seem to follow that rule (ie. it is sync).

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:201: return; // Not
currently locked.
On 2014/02/12 21:08:04, Chris Dumez wrote:
> On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> > This is actually a false assumption. I believe that the specification says
> that
> > unlockOrientation() should revert the lock to the default which can be
> anything.
> > For example, on most Android applications, it will be: [ 'portrait-primary',
> > 'landscape' ].
> 
> Well, we are not locked due to JS. Why pass call unlockOrientation() on the
> client if the JS did not previously lock the orientation?

This code would be call if |screen.unlockOrientation| is called, right? It seems
that there are a lot of ways to have a broken behaviour here.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:203:
m_lockedOrientations = OrientationAny;
On 2014/02/12 21:08:04, Chris Dumez wrote:
> On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> > Generally speaking, I think you can get ride of m_lockedOrientations. It
does
> > not seem needed.
> 
> I need it as long as I keep locking asynchronously via the
> m_orientationLockTimer timer. See my comment about why I'd prefer to keep the
> Timer.

Sadly, indeed.

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

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:15: // event
is fired.
On 2014/02/12 21:08:04, Chris Dumez wrote:
> On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> > What could we do to fix that? Would that be reasonable to mark m_orientation
> as
> > unset so if orientation is requested, it could be explicitly requested
unless
> a
> > new value was provided?
> 
> Yes, we could then initialize m_orientation to OrientationInvalid and request
> the orientation from the client if it is requested and m_orientation is
invalid.
> Sadly, this would have to be synchronous, right? (since screen.orientation
> returns the orientation synchronously).
> 
> However, right now I have no support for requesting the orientation from the
> client so I have to initialize it to something valid.

Let's solve that in a follow-up.

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:16: ,
m_orientation(OrientationPortraitPrimary)
On 2014/02/12 21:08:04, Chris Dumez wrote:
> On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> > Does that mean that if a page is loaded in another orientation than
> > 'portrait-primary', an orientationchange event will be fired even if the
> > orientation never changed?
> 
> I don't think it would, as there would be no orientation change according to
the
> client. However, the issue would be if the device is then rotated from
> 'landscape-primary' to 'portrait-primary'. We likely won't fire an event
because
> the controller will think we are already in "portrait-primary". So yes, we
have
> to handle this better. I think initializing to an invalid / unknown
orientation
> is a good idea. The question is what should I do if screen.orientation is
> accessed and I do not know the current orientation. I think it would be bad to
> add a method to request the orientation from the client as this would mean a
> synchronous call and this is likely to happen on application startup.

And that.

[Inactive, 2014-02-13 16:32:25]

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/920001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:188:
m_orientationLockTimer.startOneShot(0);
On 2014/02/13 16:12:08, Mounir Lamouri wrote:
> On 2014/02/12 21:08:04, Chris Dumez wrote:
> > On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> > > I do not think having this call async here is really useful. Given that we
> > will
> > > likely cross process boundaries, locking will be async anyway. What we
> really
> > > care about is that the orientationchange event is sent asynchronously
> compared
> > > to the lockOrientation() call. Why not specifying this in
> > > WebScreenOrientationClient to make sure the implementation keep
> > > lockOrientation() async?
> > 
> > I am not sure about this one. I think this is a quite common way of making
> sure
> > we do the task asynchronously, as mandated by the spec. I am slightly
worried
> > about making assumptions about crossing process boundaries. As you can see
> > already, it is not the case for my mock client and I would have to move this
> > Timer there anyway.
> 
> I guess I will let the Webkit/Blink experienced folks decide here. Though, I
am
> not saying that IPC will do the job just that we should let the embedder take
> care of the asynchronous part. Forcing the Blink bits to be async would simply
> slow down things IMO. It might be bad for responsiveness. Also, note that
> unlockOrientation() does not seem to follow that rule (ie. it is sync).

Yes, I asked for clarification on the spec about unlockOrientation():
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24614

[Peter Beverloo, 2014-02-13 16:33:41]

lgtm

I'd like to see a little bit more explanation in your CL's description about
*how* making Screen an EventTarget changes Web exposed behavior, even when the
Screen Orientation API hasn't been enabled yet.

Something like this may work:
  "This CL changes Screen to be an EventTarget. While the Screen Orientation API
has been implemented behind a RuntimeEnabled flag, there is no way of having
inheritence depend on the flag's value. The Web exposed effect of this is that
people will now be able to dispatch custom events on the window.screen
interface, which in itself is harmless."

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
Source/modules/screen_orientation/ScreenOrientationController.cpp:14: // FIXME:
This orientation is not necessary the right one until the first
orientationchange
On 2014/02/13 15:47:30, Chris Dumez wrote:
> I would propose to address this issue in a follow-up patch as I need more time
> to investigate and the CL is already big.

That sounds fair to me.  Ideally we'd be able to unify this with the
window.orientation path and pull that out of core as well.

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
File Source/modules/screen_orientation/testing/InternalsScreenOrientation.idl
(right):

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
Source/modules/screen_orientation/testing/InternalsScreenOrientation.idl:6: void
setScreenOrientationClientMock(Document document);
nit: createScreenOrientationClientMock would be a better name, but it's very
trivial.

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
File Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp
(right):

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp:68:
for (unsigned i = 0; i < supportedOrientationValues.size(); ++i) {
nit: we can avoid the static local here by just using a simple array:

ScreenOrientationValues supportedOrientationValues[] = {
    OrientationPortraitPrimary,
    OrientationLandscapePrimary,
    OrientationPortraitSecondary,
    OrientationLandscapeSecondary
};

for (unsigned i = 0; i < WTF_ARRAY_LENGTH(supportedOrientationValues); ++i) {
    ....
}

https://codereview.chromium.org/132113006/diff/1280001/Source/web/WebScreenOr...
File Source/web/WebScreenOrientationController.cpp (right):

https://codereview.chromium.org/132113006/diff/1280001/Source/web/WebScreenOr...
Source/web/WebScreenOrientationController.cpp:16:
COMPILE_ASSERT(WebScreenOrientationLandscapeSecondary ==
static_cast<WebScreenOrientation>(WebCore::OrientationLandscapeSecondary),
WebScreenOrientationLandscapeSecondary_Is_Wrong_Should_Be_OrientationLandscapeSecondary);
This is more verbose than I hoped it'd be.. I recall a thread where someone
argued in favor of having a conversion function
(toScreenOrientationValue(WebScreenOrientation value) with a switch), but I
can't find it.  Perhaps a more experienced reviewer can comment on this.

https://codereview.chromium.org/132113006/diff/1280001/public/web/WebScreenOr...
File public/web/WebScreenOrientation.h (right):

https://codereview.chromium.org/132113006/diff/1280001/public/web/WebScreenOr...
public/web/WebScreenOrientation.h:19: } //  namespace blink
micro nit: double space after the //.

[Inactive, 2014-02-13 17:03:22]

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
File LayoutTests/screen_orientation/locked-no-orientation-change-event.html
(right):

https://codereview.chromium.org/132113006/diff/920001/LayoutTests/screen_orie...
LayoutTests/screen_orientation/locked-no-orientation-change-event.html:6:
description("Validates that no orientationchange event is fired when we are
locked.");
On 2014/02/13 16:12:08, Mounir Lamouri wrote:
> On 2014/02/12 21:08:04, Chris Dumez wrote:
> > On 2014/02/12 20:11:26, Mounir Lamouri wrote:
> > > This test seems to essentially test the mock object. It is the role of the
> > > backend to make sure the orientation does not change. If the
> > > ScreenOrientationController gets an orientationChanged call for
> > > 'portrait-secondary' while the screen is locked to 'portrait-primary',
Blink
> > > couldn't do much to fix the situation: the screen orientation would have
> > changed
> > > and not firing the event might not even be the right thing to do.
> > 
> > So you want to remove this layout test?
> 
> I guess we could remove it yes. There is no point in keeping that test and it
> might give the wrong impression that this behaviour is actually tested.

Done.

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
File Source/modules/screen_orientation/testing/InternalsScreenOrientation.idl
(right):

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
Source/modules/screen_orientation/testing/InternalsScreenOrientation.idl:6: void
setScreenOrientationClientMock(Document document);
On 2014/02/13 16:33:42, Peter Beverloo wrote:
> nit: createScreenOrientationClientMock would be a better name, but it's very
> trivial.

I chose this name for consistency with InternalsGeolocation.idl:
setGeolocationClientMock()

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
File Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp
(right):

https://codereview.chromium.org/132113006/diff/1280001/Source/modules/screen_...
Source/modules/screen_orientation/testing/ScreenOrientationClientMock.cpp:68:
for (unsigned i = 0; i < supportedOrientationValues.size(); ++i) {
On 2014/02/13 16:33:42, Peter Beverloo wrote:
> nit: we can avoid the static local here by just using a simple array:
> 
> ScreenOrientationValues supportedOrientationValues[] = {
>     OrientationPortraitPrimary,
>     OrientationLandscapePrimary,
>     OrientationPortraitSecondary,
>     OrientationLandscapeSecondary
> };
> 
> for (unsigned i = 0; i < WTF_ARRAY_LENGTH(supportedOrientationValues); ++i) {
>     ....
> }

Done.

https://codereview.chromium.org/132113006/diff/1280001/Source/web/WebScreenOr...
File Source/web/WebScreenOrientationController.cpp (right):

https://codereview.chromium.org/132113006/diff/1280001/Source/web/WebScreenOr...
Source/web/WebScreenOrientationController.cpp:16:
COMPILE_ASSERT(WebScreenOrientationLandscapeSecondary ==
static_cast<WebScreenOrientation>(WebCore::OrientationLandscapeSecondary),
WebScreenOrientationLandscapeSecondary_Is_Wrong_Should_Be_OrientationLandscapeSecondary);
On 2014/02/13 16:33:42, Peter Beverloo wrote:
> This is more verbose than I hoped it'd be.. I recall a thread where someone
> argued in favor of having a conversion function
> (toScreenOrientationValue(WebScreenOrientation value) with a switch), but I
> can't find it.  Perhaps a more experienced reviewer can comment on this.

This is the pattern I found in other files in Blink so at least it is not new. I
remember someone advising against this though on WebKit side, I am not aware of
any similar "rule" in Blink. That said, either way is fine with me.

https://codereview.chromium.org/132113006/diff/1280001/public/web/WebScreenOr...
File public/web/WebScreenOrientation.h (right):

https://codereview.chromium.org/132113006/diff/1280001/public/web/WebScreenOr...
public/web/WebScreenOrientation.h:19: } //  namespace blink
On 2014/02/13 16:33:42, Peter Beverloo wrote:
> micro nit: double space after the //.

Done.

[Inactive, 2014-02-13 17:06:37]

https://codereview.chromium.org/132113006/diff/1280001/Source/web/WebScreenOr...
File Source/web/WebScreenOrientationController.cpp (right):

https://codereview.chromium.org/132113006/diff/1280001/Source/web/WebScreenOr...
Source/web/WebScreenOrientationController.cpp:16:
COMPILE_ASSERT(WebScreenOrientationLandscapeSecondary ==
static_cast<WebScreenOrientation>(WebCore::OrientationLandscapeSecondary),
WebScreenOrientationLandscapeSecondary_Is_Wrong_Should_Be_OrientationLandscapeSecondary);
On 2014/02/13 17:03:24, Chris Dumez wrote:
> On 2014/02/13 16:33:42, Peter Beverloo wrote:
> > This is more verbose than I hoped it'd be.. I recall a thread where someone
> > argued in favor of having a conversion function
> > (toScreenOrientationValue(WebScreenOrientation value) with a switch), but I
> > can't find it.  Perhaps a more experienced reviewer can comment on this.
> 
> This is the pattern I found in other files in Blink so at least it is not new.
I
> remember someone advising against this though on WebKit side, I am not aware
of
> any similar "rule" in Blink. That said, either way is fine with me.

Oh, I have just found:
Source/web/AssertMatchingEnums.cpp:COMPILE_ASSERT_MATCHING_ENUM()

Sounds like this is the expected way of doing this. I'll update the CL.

[Inactive, 2014-02-13 17:12:29]

https://codereview.chromium.org/132113006/diff/1280001/Source/web/WebScreenOr...
File Source/web/WebScreenOrientationController.cpp (right):

https://codereview.chromium.org/132113006/diff/1280001/Source/web/WebScreenOr...
Source/web/WebScreenOrientationController.cpp:16:
COMPILE_ASSERT(WebScreenOrientationLandscapeSecondary ==
static_cast<WebScreenOrientation>(WebCore::OrientationLandscapeSecondary),
WebScreenOrientationLandscapeSecondary_Is_Wrong_Should_Be_OrientationLandscapeSecondary);
On 2014/02/13 17:06:38, Chris Dumez wrote:
> On 2014/02/13 17:03:24, Chris Dumez wrote:
> > On 2014/02/13 16:33:42, Peter Beverloo wrote:
> > > This is more verbose than I hoped it'd be.. I recall a thread where
someone
> > > argued in favor of having a conversion function
> > > (toScreenOrientationValue(WebScreenOrientation value) with a switch), but
I
> > > can't find it.  Perhaps a more experienced reviewer can comment on this.
> > 
> > This is the pattern I found in other files in Blink so at least it is not
new.
> I
> > remember someone advising against this though on WebKit side, I am not aware
> of
> > any similar "rule" in Blink. That said, either way is fine with me.
> 
> Oh, I have just found:
> Source/web/AssertMatchingEnums.cpp:COMPILE_ASSERT_MATCHING_ENUM()
> 
> Sounds like this is the expected way of doing this. I'll update the CL.

Done.

[Inactive, 2014-02-14 13:28:20]

jochen and/or abarth, could you please take a look when you get a chance?

[abarth-chromium, 2014-02-15 18:44:26]

not lgtm

This CL is too large to review.  Please break up the change into smaller pieces
that can be reviewed individually.  There's a lot of confusion in this CL about
the lifecycles of various objects.  It's hard to sort all that out with such a
large CL.  Hopefully as we work through smaller pieces, those issues will become
clear.

https://codereview.chromium.org/132113006/diff/1520001/Source/core/frame/Scre...
File Source/core/frame/Screen.h (right):

https://codereview.chromium.org/132113006/diff/1520001/Source/core/frame/Scre...
Source/core/frame/Screen.h:59: virtual ExecutionContext* executionContext()
const OVERRIDE;
Is it ok for this to return 0?

https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
Source/modules/screen_orientation/ScreenOrientation.cpp:19: ,
ActiveDOMObject(frame() ? frame()->document() : 0)
What are the consequences of passing 0 to ActiveDOMObject here?  Presumably
we'll never get the stop notification, which would be bad.

https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
Source/modules/screen_orientation/ScreenOrientation.cpp:51: }
Is this object a Frame-lifetime object or a Document-lifetime object?  You're
mixing up concepts from both.  I suspect it ought to be a Document-lifetime
object, which means you shouldn't do any processing when loads commit.

https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
Source/modules/screen_orientation/ScreenOrientation.cpp:63: }
These don't make much sense.  We're a supplement of m_screen, which means we're
owned by m_screen.

https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
Source/modules/screen_orientation/ScreenOrientation.cpp:69:
controller->removeObserver(this);
What are the consequences of |controller| being zero here?  Won't we fail to
unregister ourselves?

https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
Source/modules/screen_orientation/ScreenOrientation.h:36: class
ScreenOrientation FINAL : public Supplement<Screen>, public DOMWindowProperty,
public PageLifecycleObserver, public ActiveDOMObject {
This seems like a lot of base classes.  It's odd that you'd need to be a
DOMWindowProperty, a PageLifecycleObserver, and a ActiveDOMObject.  Normally one
of these would suffice.

[Inactive, 2014-02-16 01:04:38]

On 2014/02/15 18:44:26, abarth wrote:
> not lgtm
> 
> This CL is too large to review.  Please break up the change into smaller
pieces
> that can be reviewed individually.  There's a lot of confusion in this CL
about
> the lifecycles of various objects.  It's hard to sort all that out with such a
> large CL.  Hopefully as we work through smaller pieces, those issues will
become
> clear.
> 
>
https://codereview.chromium.org/132113006/diff/1520001/Source/core/frame/Scre...
> File Source/core/frame/Screen.h (right):
> 
>
https://codereview.chromium.org/132113006/diff/1520001/Source/core/frame/Scre...
> Source/core/frame/Screen.h:59: virtual ExecutionContext* executionContext()
> const OVERRIDE;
> Is it ok for this to return 0?
> 
>
https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
> File Source/modules/screen_orientation/ScreenOrientation.cpp (right):
> 
>
https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
> Source/modules/screen_orientation/ScreenOrientation.cpp:19: ,
> ActiveDOMObject(frame() ? frame()->document() : 0)
> What are the consequences of passing 0 to ActiveDOMObject here?  Presumably
> we'll never get the stop notification, which would be bad.
> 
>
https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
> Source/modules/screen_orientation/ScreenOrientation.cpp:51: }
> Is this object a Frame-lifetime object or a Document-lifetime object?  You're
> mixing up concepts from both.  I suspect it ought to be a Document-lifetime
> object, which means you shouldn't do any processing when loads commit.
> 
>
https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
> Source/modules/screen_orientation/ScreenOrientation.cpp:63: }
> These don't make much sense.  We're a supplement of m_screen, which means
we're
> owned by m_screen.
> 
>
https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
> Source/modules/screen_orientation/ScreenOrientation.cpp:69:
> controller->removeObserver(this);
> What are the consequences of |controller| being zero here?  Won't we fail to
> unregister ourselves?
> 
>
https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
> File Source/modules/screen_orientation/ScreenOrientation.h (right):
> 
>
https://codereview.chromium.org/132113006/diff/1520001/Source/modules/screen_...
> Source/modules/screen_orientation/ScreenOrientation.h:36: class
> ScreenOrientation FINAL : public Supplement<Screen>, public DOMWindowProperty,
> public PageLifecycleObserver, public ActiveDOMObject {
> This seems like a lot of base classes.  It's odd that you'd need to be a
> DOMWindowProperty, a PageLifecycleObserver, and a ActiveDOMObject.  Normally
one
> of these would suffice.

Ok, thanks for taking the time to look at the CL Adam. I am happy to split the
patch into smaller pieces and get this reviewed / landed incrementally. I just
wanted to have something somewhat complete to show first. Sorry about the mess
with DOMWindowProperty / PageLifecycleObserver / ActiveDOMObject, I experienced
flaky crashes in my earlier iterations and I experimented with / added them
until I was able to get rid of the crashes.

[abarth-chromium, 2014-02-16 20:51:08]

On 2014/02/16 01:04:38, Chris Dumez wrote:
> Sorry about the mess
> with DOMWindowProperty / PageLifecycleObserver / ActiveDOMObject, I
experienced
> flaky crashes in my earlier iterations and I experimented with / added them
> until I was able to get rid of the crashes.

No worries.  Now that we've fixed the lifetime issues with DOMWindow, we can
unify many of these concepts, which will make them much easier to use.  Maybe
after you finish screen orientation I'll be able to tempt you into that
refactoring project.  :)