[WIP] Add Chromium-side support for the Screen Orientation API

[WIP] Add Chromium-side support for the Screen Orientation API

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

Dependeds on blink-side implementation: https://codereview.chromium.org/132113006/

BUG=162827

[kenneth.r.christiansen, 2014-02-06 15:03:12]

https://codereview.chromium.org/150153004/diff/1/content/browser/android/brow...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/150153004/diff/1/content/browser/android/brow...
content/browser/android/browser_jni_registrar.cc:78:
{"ScreenOrientaionProvider",
Spelling error

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.cc
(right):

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:40:
void ScreenOrientationDispatcherHost::OnLockOrientation(unsigned char lock,
why not wrap the argument like in the method above?

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:51:
assert(orientation_lock_);
I would say no, so the Blink implementation needs to handle this so that the web
facing api can never result in an assert... you probably want to write a unit
test for that

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.h
(right):

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
content/browser/screen_orientation/screen_orientation_dispatcher_host.h:22: void
OnLockOrientation(unsigned char, bool*);
should be bool... but you might want to rethink how this could work with
promises. Ie. a promise might not fire immediately, say if the windows is
already animating something so it will have to wait a bit.

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
File content/browser/screen_orientation/screen_orientation_provider_android.cc
(right):

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
content/browser/screen_orientation/screen_orientation_provider_android.cc:27:
return false;
does the spec say that that is not allowed?

I heard that some android phones disallow portrait-secondary by default, but
that apps can actually override that. If that is the case I would say that this
should be allowed. Unlock will restore default platform behavior

https://codereview.chromium.org/150153004/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/150153004/diff/1/content/common/view_messages...
content/common/view_messages.h:2147: // Sent by the renderer to the browser to
lock orientations by given
to request locking ?

https://codereview.chromium.org/150153004/diff/1/content/public/browser/conte...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/150153004/diff/1/content/public/browser/conte...
content/public/browser/content_browser_client.h:598: // FYI: Used by an external
project; please don't remove.
FYI, why not Note,

https://codereview.chromium.org/150153004/diff/1/content/renderer/screen_orie...
File content/renderer/screen_orientation_dispatcher.cc (right):

https://codereview.chromium.org/150153004/diff/1/content/renderer/screen_orie...
content/renderer/screen_orientation_dispatcher.cc:43:
controller_->orientationChanged(web_orientation);
are you making sure this is sent as the same time as the
window.onorientationchange thing that mobile supports? (coming from iOS Safari)

[ostap, 2014-02-06 21:23:27]

Hi Kenneth!

Thanks for review!

https://codereview.chromium.org/150153004/diff/1/content/browser/android/brow...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/150153004/diff/1/content/browser/android/brow...
content/browser/android/browser_jni_registrar.cc:78:
{"ScreenOrientaionProvider",
On 2014/02/06 15:03:12, kenneth.r.christiansen wrote:
> Spelling error

Done.

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.cc
(right):

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:40:
void ScreenOrientationDispatcherHost::OnLockOrientation(unsigned char lock,
On 2014/02/06 15:03:12, kenneth.r.christiansen wrote:
> why not wrap the argument like in the method above?

Done.

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:51:
assert(orientation_lock_);
On 2014/02/06 15:03:12, kenneth.r.christiansen wrote:
> I would say no, so the Blink implementation needs to handle this so that the
web
> facing api can never result in an assert... you probably want to write a unit
> test for that

Ok. Added to TODO list ;)

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.h
(right):

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
content/browser/screen_orientation/screen_orientation_dispatcher_host.h:22: void
OnLockOrientation(unsigned char, bool*);
On 2014/02/06 15:03:12, kenneth.r.christiansen wrote:
> should be bool... but you might want to rethink how this could work with
> promises. Ie. a promise might not fire immediately, say if the windows is
> already animating something so it will have to wait a bit.

In this case I don't think sync IPC could be used. We can't block renderer
thread so long.

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
File content/browser/screen_orientation/screen_orientation_provider_android.cc
(right):

https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
content/browser/screen_orientation/screen_orientation_provider_android.cc:27:
return false;
On 2014/02/06 15:03:12, kenneth.r.christiansen wrote:
> does the spec say that that is not allowed?
> 
> I heard that some android phones disallow portrait-secondary by default, but
> that apps can actually override that. If that is the case I would say that
this
> should be allowed. Unlock will restore default platform behavior

That's android specific.
Current android API can lock only one current orientation, so locking landscape
and portrait at the same time won't work for sure.

https://codereview.chromium.org/150153004/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/150153004/diff/1/content/common/view_messages...
content/common/view_messages.h:2147: // Sent by the renderer to the browser to
lock orientations by given
On 2014/02/06 15:03:12, kenneth.r.christiansen wrote:
> to request locking ?

Done.

https://codereview.chromium.org/150153004/diff/1/content/public/browser/conte...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/150153004/diff/1/content/public/browser/conte...
content/public/browser/content_browser_client.h:598: // FYI: Used by an external
project; please don't remove.
On 2014/02/06 15:03:12, kenneth.r.christiansen wrote:
> FYI, why not Note,

To be consistent with OverrideSystemLocationProvider() and
OverrideVibrationProvider() which already have similar comment.

https://codereview.chromium.org/150153004/diff/1/content/renderer/screen_orie...
File content/renderer/screen_orientation_dispatcher.cc (right):

https://codereview.chromium.org/150153004/diff/1/content/renderer/screen_orie...
content/renderer/screen_orientation_dispatcher.cc:43:
controller_->orientationChanged(web_orientation);
On 2014/02/06 15:03:12, kenneth.r.christiansen wrote:
> are you making sure this is sent as the same time as the
> window.onorientationchange thing that mobile supports? (coming from iOS
Safari)

At least it is hooked at the same RenderViewImpl::OnOrientationChangeEvent(), so
should be the same. It fires just before window.orientationchange because
RenderView observers are processed before call to
webview()->mainFrame()->sendOrientationChangeEvent() .

[kenneth.r.christiansen, 2014-02-07 08:53:02]

>
https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
> File content/browser/screen_orientation/screen_orientation_dispatcher_host.h
> (right):
> 
>
https://codereview.chromium.org/150153004/diff/1/content/browser/screen_orien...
> content/browser/screen_orientation/screen_orientation_dispatcher_host.h:22:
void
> OnLockOrientation(unsigned char, bool*);
> On 2014/02/06 15:03:12, kenneth.r.christiansen wrote:
> > should be bool... but you might want to rethink how this could work with
> > promises. Ie. a promise might not fire immediately, say if the windows is
> > already animating something so it will have to wait a bit.
> 
> In this case I don't think sync IPC could be used. We can't block renderer
> thread so long.

They should be async :-) that is one of the major points why we want it to use a
Promise.

> That's android specific.
> Current android API can lock only one current orientation, so locking
landscape
> and portrait at the same time won't work for sure.

I guess the question then is whether we want it to unset the lock instead then.
That seems to be closest to what the developer intended.

> At least it is hooked at the same RenderViewImpl::OnOrientationChangeEvent(),
so
> should be the same. It fires just before window.orientationchange because
> RenderView observers are processed before call to
> webview()->mainFrame()->sendOrientationChangeEvent() .

ok

[mlamouri (slow - plz ping), 2014-02-20 16:54:57]

As discussed offline, I have been working on an updated plumbing following the
current status of the Screen Orientation API implementation in Blink.

The CL is https://codereview.chromium.org/164913004/

I think we should close this issue.