Screen Orientation API: screen.orientation & orientationchange event

Screen Orientation API: screen.orientation & orientationchange event

Screen Orientation API: add implemention for screen.orientation attribute, keep
it updated via the ScreenOrientationController and notify client of orientation
changes via the 'orientationchange' event.

The layout test infrastructure is still missing to test this. It will be added
in a follow-up patch.

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

[Inactive, 2014-02-17 18:23:12]

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

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:41: if
(ScreenOrientationController* controller =
ScreenOrientationController::from(page()))
I looked at the Geolocation module and it subclasses ActiveDOMObject, then
overrides stop() and unregisters from the controller in stop() instead of the
destructor. Please let me know if you think I should do the same here.

[abarth-chromium, 2014-02-18 01:34:27]

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

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:20:
controller->addObserver(this);
Under what circumstances can |controller| be zero.  I'd be include to ASSERT
that |controller| is non-zero and make it an error to construct a
ScreenOrientation object in those circumstances.

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:42:
controller->removeObserver(this);
I believe the correct thing to do is to override willDetachGlobalObjectFromFrame
and remove the observer then.  Removing the observer should be unconditional. 
We want both the addObserver and removeObserver calls to be unconditional so
that we know they balance.

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:51: return
window->screen();
Why not just:

return m_associatedDOMWindow->screen();

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:59: static
ScreenOrientationInfo* allowedOrientationsMap(unsigned& length)
Please put free static functions at the top of the file.

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

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.h:46: Page* page() const;
Should this be private?

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

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:19: }
Please ASSERT(m_observers.isEmpty()) here.

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:55:
(*it)->orientationChanged();
This is a weak iteration pattern.  What if m_observers changes during this
iteration?  Notice that we're executing JavaScript synchronously below this call
stack.  That means ScreenOrientationController could even be destroyed.

To fix this iteration, you'll need to create a local variable with
RefPtr<Screen>s to hold on to the screen objects and (transitively) the
ScreenOrientation objects before executing any JavaScript.  You'll also need to
be careful not to touch the |this| pointer after executing JavaScript.

https://codereview.chromium.org/169403006/diff/1/Source/web/ScreenOrientation...
File Source/web/ScreenOrientationClientProxy.h (right):

https://codereview.chromium.org/169403006/diff/1/Source/web/ScreenOrientation...
Source/web/ScreenOrientationClientProxy.h:18: class ScreenOrientationClientProxy
FINAL {
Rather than plumb these calls through the client, we should plumb them through
public/platform.  Determining a screen orientation is low-level information
about the platform that's travelling "up" to Blink.  That approach will avoid
the need to duplicate the enum values (just use the public/platform enum values
in Blink directly) and will avoid the need to introduce classes with silly names
like "ClientProxy"

https://codereview.chromium.org/169403006/diff/1/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/169403006/diff/1/Source/web/WebViewImpl.h#new...
Source/web/WebViewImpl.h:763: OwnPtr<ScreenOrientationClientProxy>
m_screenOrientationClientProxy;
Geolocation is plumbed backwards.  Its' not a good model to follow.  Geolocation
predates public/platform.

[abarth-chromium, 2014-02-18 01:37:51]

http://src.chromium.org/viewvc/blink/trunk/Source/modules/device_orientation/
might be a good model to look at.

[abarth-chromium, 2014-02-18 01:40:03]

Notice that the DeviceOrientationDispatcher is static, which means you don't
have the tricky coupling to the page lifetime...  Maybe that doesn't work for
screen orientation because of multimonitor scenarios?

[abarth-chromium, 2014-02-18 01:43:29]

It's also interesting to notice how DeviceOrientationDispatcher deals with the
collection being mutated during iteration.  Basically, it keeps the listeners in
a Vector and zeros out entries in the vector when listeners are removed.  After
iterating, it compacts the vector.  The code is a bit subtle, but it handles all
the edge cases correctly (at least the ones that we thought about).

[mlamouri (slow - plz ping), 2014-02-18 11:19:23]

It is worth pointing that the Screen Orientation API has one small difference
compared to the Device Orientation: you can synchronously access
|screen.orientation| - the same way Webkit/Blink allow you to synchronously
access |window.orientation|. Device Orientation is event based only which means
that you can't access any data outside of what is sent in the DOM events. It is
quite simpler to implement but a bit more painful for developers.

|window.orientation| is currently broken because the implementation is not
taking into account the fact that the API is truly synchronous and the value is
updated only if an event is received from the RVH. We could easily fix that by
passing the orientation value when creating the RV. However, it would require to
always send screen orientation updates to all child processes. I guess it is not
a big deal given that they should not happen that often.

An alternative approach taken by Gecko is to have the child process listening to
the screen orientation events from the parent process when needed but that
requires to do a synchronous IPC call to get the initial |screen.orientation|
value.

[mlamouri (slow - plz ping), 2014-02-18 17:08:55]

https://codereview.chromium.org/169403006/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/169403006/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:415: provideScreenOrientationTo(m_page.get());
You probably want to call |provideScreenOrientationTo| before
ScreenOrientationController::from()...

[abarth-chromium, 2014-02-18 17:50:34]

Hum...  I don't think we should add a synchronous IPC call to implement this
feature.  We shouldn't implement new synchronous APIs.  Instead, the API should
return a promise.

My recommendation should be to not implement screen.orientation at this time and
instead propose an asynchronous, Promise-based API to the working group.

[Inactive, 2014-02-18 19:05:42]

On 2014/02/18 17:50:34, abarth wrote:
> Hum...  I don't think we should add a synchronous IPC call to implement this
> feature.  We shouldn't implement new synchronous APIs.  Instead, the API
should
> return a promise.
> 
> My recommendation should be to not implement screen.orientation at this time
and
> instead propose an asynchronous, Promise-based API to the working group.

I think it is clear that this API would benefit a lot from Promises and I do
hope / expect the spec evolves in this direction. However, we were hoping the
land an implementation behind a runtime flag and then make it evolve as the spec
gets updated. Since we are not shipping this at this point, I did not think this
would be an issue.

Of course, I am not planning to add any synchronous IPC call and I did not
include this in any of my patches. However, you seem to be proposing we hold off
on the implementation until the spec gets updated (although your comment seems
to be specific to screen.orientation, and not screen.lockOrientation() /
unlockOrientation()). For the record, I think lockOrientation() should also
return a Promise so that we can notify the caller if the backend failed to lock
(e.g. does not support locking) as locking is asynchronous but the method
currently returns a boolean synchronously. So, should we hold off on orientation
locking as well until the spec returns a Promise?

I still hope to land an implementation without promises and update it later on
when the spec gets updated. Adam, do you think this would be acceptable or do
you really want we wait to implement until the spec uses Promises?

[abarth-chromium, 2014-02-18 19:09:50]

On 2014/02/18 19:05:42, Chris Dumez wrote:
> I still hope to land an implementation without promises and update it later on
> when the spec gets updated. Adam, do you think this would be acceptable or do
> you really want we wait to implement until the spec uses Promises?

I think it's fine to implement the parts of the spec that don't require
synchronous IPCs.  I wouldn't add any sync IPCs, however, even for code that's
behind a flag.  There's very little chance we'll want to ship a version with
sync IPCs, so there's not much point in adding them.

[Inactive, 2014-02-18 19:17:22]

On 2014/02/18 19:09:50, abarth wrote:
> On 2014/02/18 19:05:42, Chris Dumez wrote:
> > I still hope to land an implementation without promises and update it later
on
> > when the spec gets updated. Adam, do you think this would be acceptable or
do
> > you really want we wait to implement until the spec uses Promises?
> 
> I think it's fine to implement the parts of the spec that don't require
> synchronous IPCs.  I wouldn't add any sync IPCs, however, even for code that's
> behind a flag.  There's very little chance we'll want to ship a version with
> sync IPCs, so there's not much point in adding them.

Ok, thanks. And we are in agreement about the sync IPC.

[mlamouri (slow - plz ping), 2014-02-18 20:13:11]

On 2014/02/18 17:50:34, abarth wrote:
> Hum...  I don't think we should add a synchronous IPC call to implement this
> feature.  We shouldn't implement new synchronous APIs.  Instead, the API
should
> return a promise.
> 
> My recommendation should be to not implement screen.orientation at this time
and
> instead propose an asynchronous, Promise-based API to the working group.

window.orientation is already a synchronous API so unless Blink is expecting to
deprecate it, we will have to provide that information synchronously and
reliably. Right now, it is synchronous but not reliable.

This API is going to switch to Promises for lockOrientation() and
unlockOrientation(). However, I am not really in favour of having
screen.orientation behind a promise. Promises are indeed better than having
callbacks but asynchronous code still remain painful for developers even if
promises make it less painful.

In order to have screen.orientation synchronous in an efficient way, we would
just have to pass the current orientation every time we create a RenderView.
That information is 2 bits and we already pass the entire screen information so
adding those two bits should be harmless. We could even add that information
directly to WebScreenInfo.

[Inactive, 2014-02-18 20:24:22]

On 2014/02/18 20:13:11, Mounir Lamouri wrote:
> On 2014/02/18 17:50:34, abarth wrote:
> > Hum...  I don't think we should add a synchronous IPC call to implement this
> > feature.  We shouldn't implement new synchronous APIs.  Instead, the API
> should
> > return a promise.
> > 
> > My recommendation should be to not implement screen.orientation at this time
> and
> > instead propose an asynchronous, Promise-based API to the working group.
> 
> window.orientation is already a synchronous API so unless Blink is expecting
to
> deprecate it, we will have to provide that information synchronously and
> reliably. Right now, it is synchronous but not reliable.

-> "Blink is expecting to deprecate it"
Blink is not shipping it so there is no need for Blink to deprecate it.

Why can't the spec be updated to use Promises to get the orientation? I
understand Firefox and IE11 are shipping this API but both implementations are
prefixed anyway.

[abarth-chromium, 2014-02-18 20:40:23]

Even if we were shipping window.orientation, that doesn't mean we should add
more synchronous APIs.  As a general rule, we're not going to add any more
synchronous APIs.

[Inactive, 2014-02-18 20:52:46]

I updated the CL to use the same design as for the device_orientation module.
Please take another look.

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

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:20:
controller->addObserver(this);
On 2014/02/18 01:34:27, abarth wrote:
> Under what circumstances can |controller| be zero.  I'd be include to ASSERT
> that |controller| is non-zero and make it an error to construct a
> ScreenOrientation object in those circumstances.

Done. No longer needed now that the controller is listening for changes and
firing events (similarly to DeviceOrientation module).

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:51: return
window->screen();
On 2014/02/18 01:34:27, abarth wrote:
> Why not just:
> 
> return m_associatedDOMWindow->screen();

This function is no longer needed now that the controller fires the event.

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:59: static
ScreenOrientationInfo* allowedOrientationsMap(unsigned& length)
On 2014/02/18 01:34:27, abarth wrote:
> Please put free static functions at the top of the file.

Done.

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

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.h:46: Page* page() const;
On 2014/02/18 01:34:27, abarth wrote:
> Should this be private?

Done. No longer needed.

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

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:19: }
On 2014/02/18 01:34:27, abarth wrote:
> Please ASSERT(m_observers.isEmpty()) here.

observers are now in the Dispatcher class, which is a singleton (similarly to
DeviceOrientation module) so this is no longer needed.

https://codereview.chromium.org/169403006/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:55:
(*it)->orientationChanged();
On 2014/02/18 01:34:27, abarth wrote:
> This is a weak iteration pattern.  What if m_observers changes during this
> iteration?  Notice that we're executing JavaScript synchronously below this
call
> stack.  That means ScreenOrientationController could even be destroyed.
> 
> To fix this iteration, you'll need to create a local variable with
> RefPtr<Screen>s to hold on to the screen objects and (transitively) the
> ScreenOrientation objects before executing any JavaScript.  You'll also need
to
> be careful not to touch the |this| pointer after executing JavaScript.

Done, used the same code as DeviceOrientation.

https://codereview.chromium.org/169403006/diff/1/Source/web/ScreenOrientation...
File Source/web/ScreenOrientationClientProxy.h (right):

https://codereview.chromium.org/169403006/diff/1/Source/web/ScreenOrientation...
Source/web/ScreenOrientationClientProxy.h:18: class ScreenOrientationClientProxy
FINAL {
On 2014/02/18 01:34:27, abarth wrote:
> Rather than plumb these calls through the client, we should plumb them through
> public/platform.  Determining a screen orientation is low-level information
> about the platform that's travelling "up" to Blink.  That approach will avoid
> the need to duplicate the enum values (just use the public/platform enum
values
> in Blink directly) and will avoid the need to introduce classes with silly
names
> like "ClientProxy"

Done.

https://codereview.chromium.org/169403006/diff/1/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/169403006/diff/1/Source/web/WebViewImpl.h#new...
Source/web/WebViewImpl.h:763: OwnPtr<ScreenOrientationClientProxy>
m_screenOrientationClientProxy;
On 2014/02/18 01:34:27, abarth wrote:
> Geolocation is plumbed backwards.  Its' not a good model to follow. 
Geolocation
> predates public/platform.

Done.

[mlamouri (slow - plz ping), 2014-02-19 15:10:11]

On 2014/02/18 20:40:23, abarth wrote:
> Even if we were shipping window.orientation, 

We are shipping window.orientation.

> Why can't the spec be updated to use Promises to get the orientation? I
> understand Firefox and IE11 are shipping this API but both implementations are
> prefixed anyway.

It's all about compromise. How painful is that going to be for API consumers to
use this API if it is asynchronous? How painful it is for browser vendors to
make the API synchronous? Passing two bits from the RVH to the RV sounds like a
small burden.

[abarth-chromium, 2014-02-19 18:44:57]

On 2014/02/19 15:10:11, Mounir Lamouri wrote:
> It's all about compromise. How painful is that going to be for API consumers
to
> use this API if it is asynchronous? How painful it is for browser vendors to
> make the API synchronous? Passing two bits from the RVH to the RV sounds like
a
> small burden.

I'm not sure this CL thread is the best forum for this discussion.  I'm happy to
continue this discussion in another forum.  In the meantime, the status quo is
that we're not implementing any more synchronous APIs.

[abarth-chromium, 2014-02-19 18:51:49]

LGTM

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

https://codereview.chromium.org/169403006/diff/270001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:25:
ASSERT(document);
We don't usually have this ASSERT here.  The code will have crashed already if
document is null.

https://codereview.chromium.org/169403006/diff/270001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:35: // event
is fired.
I'd skip this FIXME

https://codereview.chromium.org/169403006/diff/270001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/169403006/diff/270001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:126:
ScreenOrientationController::from(document);
Is this necessary?  This means we'll always boot up the
ScreenOrientationController...  I guess that's necessary because
ScreenOrientation is a supplement and we don't necessarily boot up
ScreenOrientation when someone starts listening to screen orientation events... 
:(

[abarth-chromium, 2014-02-19 18:52:07]

Thanks for iterating on the design.  This approach looks much cleaner.

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

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

https://codereview.chromium.org/169403006/diff/270001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:25:
ASSERT(document);
On 2014/02/19 18:51:49, abarth wrote:
> We don't usually have this ASSERT here.  The code will have crashed already if
> document is null.

Done.

https://codereview.chromium.org/169403006/diff/270001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:35: // event
is fired.
On 2014/02/19 18:51:49, abarth wrote:
> I'd skip this FIXME

Done.

https://codereview.chromium.org/169403006/diff/270001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/169403006/diff/270001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:126:
ScreenOrientationController::from(document);
On 2014/02/19 18:51:49, abarth wrote:
> Is this necessary?  This means we'll always boot up the
> ScreenOrientationController...  I guess that's necessary because
> ScreenOrientation is a supplement and we don't necessarily boot up
> ScreenOrientation when someone starts listening to screen orientation
events... 
> :(

Yes, I need to make sure we construct the ScreenOrientationController at some
point. Optimally, we would construct it only when someone starts listening for
orientationchange events on screen but I am not sure that's currently possible
(at least I don't know how).

[abarth-chromium, 2014-02-19 21:39:04]

On 2014/02/19 19:04:50, Chris Dumez wrote:
> Yes, I need to make sure we construct the ScreenOrientationController at some
> point. Optimally, we would construct it only when someone starts listening for
> orientationchange events on screen but I am not sure that's currently possible
> (at least I don't know how).

Yeah, that's fine.  We might improve it in the future.

[Inactive, 2014-02-19 22:01:52]

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

[commit-bot: I haz the power, 2014-02-19 22:14:56]

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

[commit-bot: I haz the power, 2014-02-19 23:11:24]

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

[commit-bot: I haz the power, 2014-02-20 07:47:30]

Change committed as 167477