Chromium plumbing for Screen Orientation API lock/unlock.

Chromium plumbing for Screen Orientation API lock/unlock.

This is using ScreenOrientationDispatcherHost to dispatch lock/unlock
requests to a ScreenOrientationProvider. This CL does not implement
a ScreenOrientationProvider.

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

[mlamouri (slow - plz ping), 2014-02-24 16:50:05]

cdn@chromium.org: Please review changes in 
- content/common/screen_orientation_messages.h

jochen@chromium.org: Please review changes in 
- everything else

[Cris Neckar, 2014-02-24 21:02:27]

https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.cc
(right):

https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:28:
IPC_MESSAGE_HANDLER(ScreenOrientationMsg_Unlock, OnUnlockRequest)
Currently sending these messages will result in a crash. You will need to
request another IPC message review when you implement
ScreenOrientationProvider::LockOrientation and UnlockOrientation... I would
really prefer that until then you don't dereference null and cause a browser
crash.

https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:50:
provider_->LockOrientation(orientations);
remove this until you actually implement it.

You will also need another IPC review from me or another security reviewer on
the CL that implements these.

https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:57:
provider_->UnlockOrientation();
ditto

https://codereview.chromium.org/177793003/diff/30001/content/common/screen_or...
File content/common/screen_orientation_messages.h (right):

https://codereview.chromium.org/177793003/diff/30001/content/common/screen_or...
content/common/screen_orientation_messages.h:26:
IPC_MESSAGE_CONTROL1(ScreenOrientationMsg_Lock,
These messages are Renderer->Browser correct?

The names should be
ScreenOrientationHostMsg_Lock
ScreenOrientationHostMsg_Unlock

[mlamouri (slow - plz ping), 2014-02-24 21:47:50]

https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.cc
(right):

https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:28:
IPC_MESSAGE_HANDLER(ScreenOrientationMsg_Unlock, OnUnlockRequest)
On 2014/02/24 21:02:28, Cris Neckar wrote:
> Currently sending these messages will result in a crash. You will need to
> request another IPC message review when you implement
> ScreenOrientationProvider::LockOrientation and UnlockOrientation... I would
> really prefer that until then you don't dereference null and cause a browser
> crash.

I'm not sure I understand wyh that would crash. Before calling |LockOrientation|
and |UnlockOrientation| on |provider_|, I check if |provider_.get()| is null. It
seems that this is a very common pattern for Android-only features: the provider
is null unless OS_ANDROID is defined. Did I miss something obvious here?

https://codereview.chromium.org/177793003/diff/30001/content/common/screen_or...
File content/common/screen_orientation_messages.h (right):

https://codereview.chromium.org/177793003/diff/30001/content/common/screen_or...
content/common/screen_orientation_messages.h:26:
IPC_MESSAGE_CONTROL1(ScreenOrientationMsg_Lock,
On 2014/02/24 21:02:28, Cris Neckar wrote:
> These messages are Renderer->Browser correct?
> 
> The names should be
> ScreenOrientationHostMsg_Lock
> ScreenOrientationHostMsg_Unlock

Didn't know about that convention. Fixed.

[Cris Neckar, 2014-02-24 22:00:03]

https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.cc
(right):

https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:28:
IPC_MESSAGE_HANDLER(ScreenOrientationMsg_Unlock, OnUnlockRequest)
On 2014/02/24 21:47:50, Mounir Lamouri wrote:
> On 2014/02/24 21:02:28, Cris Neckar wrote:
> > Currently sending these messages will result in a crash. You will need to
> > request another IPC message review when you implement
> > ScreenOrientationProvider::LockOrientation and UnlockOrientation... I would
> > really prefer that until then you don't dereference null and cause a browser
> > crash.
> 
> I'm not sure I understand wyh that would crash. Before calling
|LockOrientation|
> and |UnlockOrientation| on |provider_|, I check if |provider_.get()| is null.
It
> seems that this is a very common pattern for Android-only features: the
provider
> is null unless OS_ANDROID is defined. Did I miss something obvious here?

oh my mistake I was reading too quickly. Saw the check in the constructor and
then you reset it to null. 

Please do request another review when you implement the handler in the provider
though.

[Cris Neckar, 2014-02-24 22:00:17]

On 2014/02/24 22:00:03, Cris Neckar wrote:
>
https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
> File content/browser/screen_orientation/screen_orientation_dispatcher_host.cc
> (right):
> 
>
https://codereview.chromium.org/177793003/diff/30001/content/browser/screen_o...
> content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:28:
> IPC_MESSAGE_HANDLER(ScreenOrientationMsg_Unlock, OnUnlockRequest)
> On 2014/02/24 21:47:50, Mounir Lamouri wrote:
> > On 2014/02/24 21:02:28, Cris Neckar wrote:
> > > Currently sending these messages will result in a crash. You will need to
> > > request another IPC message review when you implement
> > > ScreenOrientationProvider::LockOrientation and UnlockOrientation... I
would
> > > really prefer that until then you don't dereference null and cause a
browser
> > > crash.
> > 
> > I'm not sure I understand wyh that would crash. Before calling
> |LockOrientation|
> > and |UnlockOrientation| on |provider_|, I check if |provider_.get()| is
null.
> It
> > seems that this is a very common pattern for Android-only features: the
> provider
> > is null unless OS_ANDROID is defined. Did I miss something obvious here?
> 
> oh my mistake I was reading too quickly. Saw the check in the constructor and
> then you reset it to null. 
> 
> Please do request another review when you implement the handler in the
provider
> though.

IPC changes lgtm

[mlamouri (slow - plz ping), 2014-02-24 22:02:56]

On 2014/02/24 22:00:17, Cris Neckar wrote:
> IPC changes lgtm

Thanks. I will add you to the review of the Android provider as requested.

[jochen (gone - plz use gerrit), 2014-02-25 16:23:06]

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.h
(right):

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.h:38:
scoped_ptr<ScreenOrientationProvider> provider_;
with this design, you will have one provider per process. How do they sync lock
requests?

Have you considered a design similar to geolocation, where the provider is a
singleton?

Could you first add the provider, and then the plumbing? That will make
reviewing easier.

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.h:39:
DISALLOW_COPY_AND_ASSIGN(ScreenOrientationDispatcherHost);
nit. add empty line before this line

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host_unittest.cc:22:
{}
please clang-format your code

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
File content/browser/screen_orientation/screen_orientation_provider.h (right):

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_provider.h:20: virtual
~ScreenOrientationProvider() {};
nit. no ; at EOL

[mlamouri (slow - plz ping), 2014-02-26 11:48:55]

The CL this CL was based on got reverted so I will keep this CL inactive until I
got the other one landed again.

[mlamouri (slow - plz ping), 2014-02-28 11:53:30]

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.h
(right):

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.h:38:
scoped_ptr<ScreenOrientationProvider> provider_;
On 2014/02/25 16:23:06, jochen (OOO from March 1) wrote:
> with this design, you will have one provider per process. How do they sync
lock
> requests?

I think for the moment, the only requirement we have is that a flow of
lock/unlock calls should arrive to the provider at the same speed as 

> Have you considered a design similar to geolocation, where the provider is a
> singleton?

Geolocation is considered as an anti-pattern nowadays. I am trying to follow a
more modern implementation design like Vibration API which I believe creates a
provider per process.

> Could you first add the provider, and then the plumbing? That will make
> reviewing easier.

It sounds unnatural because we want a plumbing that is generic and not linked to
one specific provider, right? Also, given that you are OOO on Monday, I was
hoping that we could land this before ;)

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.h:39:
DISALLOW_COPY_AND_ASSIGN(ScreenOrientationDispatcherHost);
On 2014/02/25 16:23:06, jochen (OOO from March 1) wrote:
> nit. add empty line before this line

Done.

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host_unittest.cc:22:
{}
On 2014/02/25 16:23:06, jochen (OOO from March 1) wrote:
> please clang-format your code

I did some changes based on clang-format.

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
File content/browser/screen_orientation/screen_orientation_provider.h (right):

https://codereview.chromium.org/177793003/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_provider.h:20: virtual
~ScreenOrientationProvider() {};
On 2014/02/25 16:23:06, jochen (OOO from March 1) wrote:
> nit. no ; at EOL

Done.

[mlamouri (slow - plz ping), 2014-02-28 11:55:57]

jochen@, this patch is rebased on TOT and contains all the review comments.
Hopefully, we can get this in before you are OOO. PTAL :)

[jochen (gone - plz use gerrit), 2014-02-28 12:05:00]

lgtm

[mlamouri (slow - plz ping), 2014-02-28 12:29:09]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-02-28 12:29:20]

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

[commit-bot: I haz the power, 2014-02-28 16:24:24]

Change committed as 254148