Chromium plumbing for Screen Orientation API orientationchange events.

Chromium plumbing for Screen Orientation API orientationchange events.

This is using RenderViewObserver::OrientationChangeEvent and dispatch
the information down to the renderer. More will come with locking
and unlocking abilities.

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

[mlamouri (slow - plz ping), 2014-02-20 14:56:48]

This patch is the Chromium plumbing on top of the current implementation in
Blink. In order to work, backends would have to call the OnOrientationChange()
method on ScreenOrientationDispatcherHost.

[jochen (gone - plz use gerrit), 2014-02-20 16:11:03]

https://codereview.chromium.org/164913004/diff/30001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/164913004/diff/30001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:174:
screen_orientation_dispatcher_host() const;
nit. inline the body here

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

https://codereview.chromium.org/164913004/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:18:
return true;
should return false if the message wasn't handled

https://codereview.chromium.org/164913004/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:26:
Send(new ScreenOrientationMsg_OrientationChange(orientation));
DCHECK that orientation is a valid value?

What will call OnOrientationChange?

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

https://codereview.chromium.org/164913004/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.h:15:
ScreenOrientationDispatcherHost();
needs a virtual dtor

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(right):

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:56:
listener_->didChangeScreenOrientation(web_orientation);
what if listener_ is NULL?

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
File content/renderer/screen_orientation/screen_orientation_dispatcher.h
(right):

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
content/renderer/screen_orientation/screen_orientation_dispatcher.h:23:
ScreenOrientationDispatcher(RenderThread*);
explicit.

also, needs virtual dtor

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
content/renderer/screen_orientation/screen_orientation_dispatcher.h:34: };
disallow copy/assign

[mlamouri (slow - plz ping), 2014-02-20 16:32:46]

Thanks for the quick review. I applied the comments. PTAL.

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

https://codereview.chromium.org/164913004/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:18:
return true;
On 2014/02/20 16:11:04, jochen wrote:
> should return false if the message wasn't handled

Gasp. That's what I meant :)

https://codereview.chromium.org/164913004/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:26:
Send(new ScreenOrientationMsg_OrientationChange(orientation));
On 2014/02/20 16:11:04, jochen wrote:
> DCHECK that orientation is a valid value?

Shouldn't the dispatcher just dispatch? This value is going to go trough so many
layers that I think we could just leave checking the validity at the end. Also,
anyway, this will probably switch from an int to an enum at which point the
validity will be guaranteed.

> What will call OnOrientationChange?

On Android, the ContentViewCoreImpl will get the ScreenOrientationDispatcherHost
from the RenderProcessHostImpl and call this. I guess other implementations
would do something similar.

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

https://codereview.chromium.org/164913004/diff/30001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host.h:15:
ScreenOrientationDispatcherHost();
On 2014/02/20 16:11:04, jochen wrote:
> needs a virtual dtor

Done.

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(right):

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:56:
listener_->didChangeScreenOrientation(web_orientation);
On 2014/02/20 16:11:04, jochen wrote:
> what if listener_ is NULL?

Hmm. I was assuming that could not happen and DCHECK would have been useless
given that we would crash anyway but it actually can happen.

Fixed.

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
File content/renderer/screen_orientation/screen_orientation_dispatcher.h
(right):

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
content/renderer/screen_orientation/screen_orientation_dispatcher.h:23:
ScreenOrientationDispatcher(RenderThread*);
On 2014/02/20 16:11:04, jochen wrote:
> explicit.
> 
> also, needs virtual dtor

Done.

https://codereview.chromium.org/164913004/diff/30001/content/renderer/screen_...
content/renderer/screen_orientation/screen_orientation_dispatcher.h:34: };
On 2014/02/20 16:11:04, jochen wrote:
> disallow copy/assign

Done.

[mlamouri (slow - plz ping), 2014-02-20 16:33:21]

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

[mlamouri (slow - plz ping), 2014-02-20 16:49:14]

https://codereview.chromium.org/164913004/diff/30001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/164913004/diff/30001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:174:
screen_orientation_dispatcher_host() const;
On 2014/02/20 16:11:04, jochen wrote:
> nit. inline the body here

Oups, I forgot that one. I did not inline the definition because it requires to
include screen_orientation_dispatcher_host.h.

[mlamouri (slow - plz ping), 2014-02-20 16:50:22]

-abarth -ch.dumez as reviewers
+abart +ch.dumez in CC

Please, feel free to jump in if you have any comments regarding this CL.

[Cris Neckar, 2014-02-20 18:47:45]

https://codereview.chromium.org/164913004/diff/130001/content/renderer/screen...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(right):

https://codereview.chromium.org/164913004/diff/130001/content/renderer/screen...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:54:
NOTREACHED();
We prefer that you not handle failures when it comes to IPC. Just return as
something has gone wonky.

[mlamouri (slow - plz ping), 2014-02-20 19:01:11]

Comments applied. PTAL.

https://codereview.chromium.org/164913004/diff/130001/content/renderer/screen...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(right):

https://codereview.chromium.org/164913004/diff/130001/content/renderer/screen...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:54:
NOTREACHED();
On 2014/02/20 18:47:45, Cris Neckar wrote:
> We prefer that you not handle failures when it comes to IPC. Just return as
> something has gone wonky.

Done.

[Cris Neckar, 2014-02-20 19:55:01]

On 2014/02/20 19:01:11, Mounir Lamouri wrote:
> Comments applied. PTAL.
> 
>
https://codereview.chromium.org/164913004/diff/130001/content/renderer/screen...
> File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
> (right):
> 
>
https://codereview.chromium.org/164913004/diff/130001/content/renderer/screen...
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc:54:
> NOTREACHED();
> On 2014/02/20 18:47:45, Cris Neckar wrote:
> > We prefer that you not handle failures when it comes to IPC. Just return as
> > something has gone wonky.
> 
> Done.

IPC changes LGTM

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

It's interesting that my Android build did not caught those issue with the dtor
being public for refcounted class. Does someone know by any chance why?

[ostap, 2014-02-20 20:51:12]

https://codereview.chromium.org/164913004/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/164913004/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:174:
screen_orientation_dispatcher_host() const;
Don't really understand how this should work.
RenderPorcessHostImpl will keep pointer at the instance of
ScreenOrientationDispatcherHost instance and also add it to filter list. At the
same time returned scoped_refptr might deallocate this instance outside of
RenderPorcessHostImpl . Isn't it a problem here?
Actually, where screen_orientation_dispatcher_host() will be used? Is it
necessary?

https://codereview.chromium.org/164913004/diff/260001/content/browser/screen_...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.cc
(right):

https://codereview.chromium.org/164913004/diff/260001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:26:
Send(new ScreenOrientationMsg_OrientationChange(orientation));
The RenderView on renderer side already has OnOrientationChangeEvent(). Why not
to use it? In this case extra IPC is not necessary.

[mlamouri (slow - plz ping), 2014-02-20 21:07:04]

https://codereview.chromium.org/164913004/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/164913004/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:174:
screen_orientation_dispatcher_host() const;
On 2014/02/20 20:51:12, ostap wrote:
> Don't really understand how this should work.
> RenderPorcessHostImpl will keep pointer at the instance of
> ScreenOrientationDispatcherHost instance and also add it to filter list. At
the
> same time returned scoped_refptr might deallocate this instance outside of
> RenderPorcessHostImpl . Isn't it a problem here?
> Actually, where screen_orientation_dispatcher_host() will be used? Is it
> necessary?

It would be used in order to send a message to the renderer process. 
See https://codereview.chromium.org/172473004/

https://codereview.chromium.org/164913004/diff/260001/content/browser/screen_...
File content/browser/screen_orientation/screen_orientation_dispatcher_host.cc
(right):

https://codereview.chromium.org/164913004/diff/260001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_dispatcher_host.cc:26:
Send(new ScreenOrientationMsg_OrientationChange(orientation));
On 2014/02/20 20:51:12, ostap wrote:
> The RenderView on renderer side already has OnOrientationChangeEvent(). Why
not
> to use it? In this case extra IPC is not necessary.

The idea is to merge window.orientation and screen.orientation implementation
later. At that point, OnOrientationChangeEvent will be be replaced with this.

[jochen (gone - plz use gerrit), 2014-02-24 11:53:19]

Can you include the android changes in this CL?

Also, can you use enum values instead of degree values?

[mlamouri (slow - plz ping), 2014-02-24 13:58:54]

On 2014/02/24 11:53:19, jochen wrote:
> Can you include the android changes in this CL?
> 
> Also, can you use enum values instead of degree values?

Done. Note that the Android changes are really about plugging the current
window.orientation implementation with screen.orientation. There will be bigger
changes with the lock/unlock plumbing patch (which should come right after that
one).

[jochen (gone - plz use gerrit), 2014-02-24 14:08:26]

lgtm with nits

https://codereview.chromium.org/164913004/diff/680001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/164913004/diff/680001/content/browser/android...
content/browser/android/content_view_core_impl.cc:1804: break;
default: NOTREACHED()?

https://codereview.chromium.org/164913004/diff/680001/content/common/screen_o...
File content/common/screen_orientation_messages.h (right):

https://codereview.chromium.org/164913004/diff/680001/content/common/screen_o...
content/common/screen_orientation_messages.h:16:
IPC_ENUM_TRAITS(blink::WebScreenOrientation)
nit. use IPC_ENUM_TRAITS_MAX_VALUE

https://codereview.chromium.org/164913004/diff/680001/content/renderer/screen...
File
content/renderer/screen_orientation/screen_orientation_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/164913004/diff/680001/content/renderer/screen...
content/renderer/screen_orientation/screen_orientation_dispatcher_unittest.cc:21:
virtual ~MockScreenOrientationListener() { }
no space inside {}

https://codereview.chromium.org/164913004/diff/680001/content/renderer/screen...
content/renderer/screen_orientation/screen_orientation_dispatcher_unittest.cc:25:
bool did_change_screen_orientation_;
members should be private. please use getters to access them.

https://codereview.chromium.org/164913004/diff/680001/content/renderer/screen...
content/renderer/screen_orientation/screen_orientation_dispatcher_unittest.cc:27:
};
disallow copy/assign

[mlamouri (slow - plz ping), 2014-02-24 14:20:20]

https://codereview.chromium.org/164913004/diff/680001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/164913004/diff/680001/content/browser/android...
content/browser/android/content_view_core_impl.cc:1804: break;
On 2014/02/24 14:08:26, jochen wrote:
> default: NOTREACHED()?

Done.

https://codereview.chromium.org/164913004/diff/680001/content/common/screen_o...
File content/common/screen_orientation_messages.h (right):

https://codereview.chromium.org/164913004/diff/680001/content/common/screen_o...
content/common/screen_orientation_messages.h:16:
IPC_ENUM_TRAITS(blink::WebScreenOrientation)
On 2014/02/24 14:08:26, jochen wrote:
> nit. use IPC_ENUM_TRAITS_MAX_VALUE

I can't because blink::WebScreenOrientation is a bit field so the expected
values are 0b0001, 0b0010, 0b0100, 0b1000. Actually, when I say I can't, I am
exaggerating, it's just that using IPC_ENUM_TRAITS_MAX_VALUE would not help.

https://codereview.chromium.org/164913004/diff/680001/content/renderer/screen...
File
content/renderer/screen_orientation/screen_orientation_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/164913004/diff/680001/content/renderer/screen...
content/renderer/screen_orientation/screen_orientation_dispatcher_unittest.cc:21:
virtual ~MockScreenOrientationListener() { }
On 2014/02/24 14:08:26, jochen wrote:
> no space inside {}

Done.

https://codereview.chromium.org/164913004/diff/680001/content/renderer/screen...
content/renderer/screen_orientation/screen_orientation_dispatcher_unittest.cc:25:
bool did_change_screen_orientation_;
On 2014/02/24 14:08:26, jochen wrote:
> members should be private. please use getters to access them.

Done.

https://codereview.chromium.org/164913004/diff/680001/content/renderer/screen...
content/renderer/screen_orientation/screen_orientation_dispatcher_unittest.cc:27:
};
On 2014/02/24 14:08:26, jochen wrote:
> disallow copy/assign

Done.

[mlamouri (slow - plz ping), 2014-02-24 14:42:55]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-02-24 14:43:13]

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

[commit-bot: I haz the power, 2014-02-24 20:18:32]

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

[commit-bot: I haz the power, 2014-02-24 21:01:59]

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

[commit-bot: I haz the power, 2014-02-24 23:39:04]

Change committed as 253029

[acleung1, 2014-02-25 04:01:45]

A revert of this CL has been created in
https://codereview.chromium.org/177293003/ by acleung@chromium.org.

The reason for reverting is: SEE:
https://code.google.com/p/chromium/issues/detail?id=346595.