Export ScreenOrientationProvider

Export ScreenOrientationProvider

Move ScreenOrientationProvider and ScreenOrientationDispatcherHost from content/browser/screen_orientation/ to content/public/browser/ to expose them to other libraries. This will allow Ash to use them to implement the ScreenOrientationProvider on Chrome OS. Created an implementation of ScreenOrientationDispatcherHost.
BUG=396760
Committed: https://crrev.com/7497152babcbb50ce8e0551944901924e3434352
Cr-Commit-Position: refs/heads/master@{#294819}

[jonross, 2014-08-26 18:48:01]

Patchset #1 (id:1) has been deleted

[jonross, 2014-08-26 20:21:28]

Patchset #1 (id:20001) has been deleted

[jonross, 2014-08-26 20:24:04]

jonross@chromium.org changed reviewers:
+ flackr@chromium.org

[jonross, 2014-08-26 20:24:05]

Hi Rob,

In this change I move two classes of ScreenOrientationProvider to a public
folder, for the purpose of exporting to Ash.

I also divide one of the classes into a public interface with its impl at the
original location.

I'm not targeting ash work within this change.

[jonross, 2014-08-26 20:32:09]

Patchset #1 (id:40001) has been deleted

[flackr, 2014-08-27 22:27:09]

Hey, this seems like the right idea but I'm not confident enough with the
content/ codebase to review. I'd recommend mlamouri for an initial review?

[Michael van Ouwerkerk, 2014-08-28 09:53:11]

mvanouwerkerk@chromium.org changed reviewers:
+ mlamouri@chromium.org

[jonross, 2014-09-02 14:31:32]

Hi Mounir,

Would you be able to provide an initial review for this change? This change move
ScreenOrientationProvider and ScreenOrientationDispatcher to
content/public/browser/ so that they can be exposed to other libraries for
extension.

I've done this as a first step in implementing ScreenOrientationProvider on
ChromeOS within Ash.

[mlamouri (slow - plz ping), 2014-09-03 11:25:40]

On 2014/09/02 14:31:32, jonross wrote:
> Hi Mounir,
> 
> Would you be able to provide an initial review for this change? This change
move
> ScreenOrientationProvider and ScreenOrientationDispatcher to
> content/public/browser/ so that they can be exposed to other libraries for
> extension.
> 
> I've done this as a first step in implementing ScreenOrientationProvider on
> ChromeOS within Ash.

It looks good. Though, do you have a CL using that change?

[mlamouri (slow - plz ping), 2014-09-03 11:25:49]

https://codereview.chromium.org/508603003/diff/60001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc
(right):

https://codereview.chromium.org/508603003/diff/60001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc:12:
#include "content/public/browser/screen_orientation_dispatcher_host.h"
I think screen_orientation_dispatcher_host.h is not needed.

https://codereview.chromium.org/508603003/diff/60001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.h
(right):

https://codereview.chromium.org/508603003/diff/60001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.h:70:
#endif  //
CONTENT_BROWSER_SCREEN_ORIENTATION_SCREEN_ORIENTATION_DISPATCHER_HOST_IMPL_H_
nit: you seem to be adding one space for no reason here

https://codereview.chromium.org/508603003/diff/60001/content/public/browser/s...
File content/public/browser/screen_orientation_provider.h (right):

https://codereview.chromium.org/508603003/diff/60001/content/public/browser/s...
content/public/browser/screen_orientation_provider.h:53: #endif  //
!defined(OS_ANDROID)
nit: this is adding a useless whitespace

https://codereview.chromium.org/508603003/diff/60001/content/public/browser/s...
content/public/browser/screen_orientation_provider.h:55: }  // namespace content
ditto

https://codereview.chromium.org/508603003/diff/60001/content/public/browser/s...
content/public/browser/screen_orientation_provider.h:57: #endif  //
CONTENT_PUBLIC_BROWSER_SCREEN_ORIENTATION_PROVIDER_H_
ditto

[jonross, 2014-09-03 20:33:02]

I have reverted the additional spaces, I had inserted them after one run of
cpplint.

Regarding the ash side implementation, I have a branch containing a partial
implementation. Created to confirm the exporting and the basic functionality on
the ash side. I was planning to include yourself on subsequent reviews.

https://codereview.chromium.org/508603003/diff/60001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc
(right):

https://codereview.chromium.org/508603003/diff/60001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc:12:
#include "content/public/browser/screen_orientation_dispatcher_host.h"
On 2014/09/03 11:25:49, Mounir Lamouri wrote:
> I think screen_orientation_dispatcher_host.h is not needed.

Done.

https://codereview.chromium.org/508603003/diff/60001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.h
(right):

https://codereview.chromium.org/508603003/diff/60001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.h:70:
#endif  //
CONTENT_BROWSER_SCREEN_ORIENTATION_SCREEN_ORIENTATION_DISPATCHER_HOST_IMPL_H_
On 2014/09/03 11:25:49, Mounir Lamouri wrote:
> nit: you seem to be adding one space for no reason here

Was identified when I ran cpplint. I've removed the extra space.

https://codereview.chromium.org/508603003/diff/60001/content/public/browser/s...
File content/public/browser/screen_orientation_provider.h (right):

https://codereview.chromium.org/508603003/diff/60001/content/public/browser/s...
content/public/browser/screen_orientation_provider.h:53: #endif  //
!defined(OS_ANDROID)
On 2014/09/03 11:25:49, Mounir Lamouri wrote:
> nit: this is adding a useless whitespace

Done.

https://codereview.chromium.org/508603003/diff/60001/content/public/browser/s...
content/public/browser/screen_orientation_provider.h:55: }  // namespace content
On 2014/09/03 11:25:49, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/508603003/diff/60001/content/public/browser/s...
content/public/browser/screen_orientation_provider.h:57: #endif  //
CONTENT_PUBLIC_BROWSER_SCREEN_ORIENTATION_PROVIDER_H_
On 2014/09/03 11:25:49, Mounir Lamouri wrote:
> ditto

Done.

[mlamouri (slow - plz ping), 2014-09-03 21:01:03]

On 2014/09/03 20:33:02, jonross wrote:
> I have reverted the additional spaces, I had inserted them after one run of
> cpplint.
> 
> Regarding the ash side implementation, I have a branch containing a partial
> implementation. Created to confirm the exporting and the basic functionality
on
> the ash side. I was planning to include yourself on subsequent reviews.

Is there any change you can upload a WIP CL?

[jonross, 2014-09-04 14:54:38]

On 2014/09/03 21:01:03, Mounir Lamouri wrote:
> On 2014/09/03 20:33:02, jonross wrote:
> > I have reverted the additional spaces, I had inserted them after one run of
> > cpplint.
> > 
> > Regarding the ash side implementation, I have a branch containing a partial
> > implementation. Created to confirm the exporting and the basic functionality
> on
> > the ash side. I was planning to include yourself on subsequent reviews.
> 
> Is there any change you can upload a WIP CL?

I have uploaded a WIP CL for the start of the Ash implementation, and added you
as a reviewer.

https://codereview.chromium.org/508603003/

[mlamouri (slow - plz ping), 2014-09-04 17:17:46]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org

[mlamouri (slow - plz ping), 2014-09-04 17:17:46]

lgtm

+jochen@ who can lgtm on top of me for the files I'm not owning.

[mlamouri (slow - plz ping), 2014-09-04 21:39:17]

https://codereview.chromium.org/508603003/diff/100001/content/public/browser/...
File content/public/browser/screen_orientation_provider.h (right):

https://codereview.chromium.org/508603003/diff/100001/content/public/browser/...
content/public/browser/screen_orientation_provider.h:18: class
ScreenOrientationProvider {
I just realize that this is probably missing CONTENT_EXPORT, isn't it?

[jonross, 2014-09-05 13:39:07]

https://codereview.chromium.org/508603003/diff/100001/content/public/browser/...
File content/public/browser/screen_orientation_provider.h (right):

https://codereview.chromium.org/508603003/diff/100001/content/public/browser/...
content/public/browser/screen_orientation_provider.h:18: class
ScreenOrientationProvider {
On 2014/09/04 21:39:17, Mounir Lamouri wrote:
> I just realize that this is probably missing CONTENT_EXPORT, isn't it?

Done.

[jonross, 2014-09-10 19:59:17]

Hi jochen@,

Would you be able to provide an owner review of the following:
  content/browser/android/content_view_core_impl.cc
  content/browser/web_contents/web_contents_impl.cc
  content/public/browser/screen_orientation_dispatcher_host.h
  content/public/browser/screen_orientation_provider.h

In this change I am moving files from content/browser/screen_orientation/ to
content/public/browser in order to properly expose them to libraries. This is to
allow different platforms to provide their own implementation of
ScreenOrienationProvider.

Thanks,
Jon

[jochen (gone - plz use gerrit), 2014-09-12 14:01:56]

lgtm

please also update the BUILD.gn files in content

[jonross, 2014-09-12 18:09:55]

On 2014/09/12 14:01:56, jochen wrote:
> lgtm
> 
> please also update the BUILD.gn files in content

I've looked at content/BUILD.gn and content/public/browser/BUILD.gn, which
currently seem to rely on content_browser.gypi to generate the list of sources.

Running gn locally and then a fresh build, and compilation was fine.

Do they need any further updates?

content/BUILD.gn
(https://code.google.com/p/chromium/codesearch#chromium/src/content/BUILD.gn&l=15

content/public/browser/BUILD.gn
(https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...)

[mlamouri (slow - plz ping), 2014-09-15 13:04:32]

On 2014/09/12 18:09:55, jonross wrote:
> On 2014/09/12 14:01:56, jochen wrote:
> > lgtm
> > 
> > please also update the BUILD.gn files in content
> 
> I've looked at content/BUILD.gn and content/public/browser/BUILD.gn, which
> currently seem to rely on content_browser.gypi to generate the list of
sources.
> 
> Running gn locally and then a fresh build, and compilation was fine.
> 
> Do they need any further updates?

I don't think so. Feel free to land.

[jonross, 2014-09-15 13:15:53]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-09-15 13:19:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/508603003/120001

[commit-bot: I haz the power, 2014-09-15 14:17:28]

Committed patchset #4 (id:120001) as 94932121451ee8cc6a39dd9e7fde81b530ebed9f

[commit-bot: I haz the power, 2014-09-15 15:08:32]

Patchset 4 (id:??) landed as
https://crrev.com/7497152babcbb50ce8e0551944901924e3434352
Cr-Commit-Position: refs/heads/master@{#294819}