Centralize ScreenOrientationProvider logic, add platform delegates

ScreenOrientationProvider coverts the platform agnositc logic around locking/unlocking and notifying the dispatcher of success.

A ScreenOrientationDelegate has been added to handle platform specific implementations. Such as actual locking/unlocking, as well as if the actions are supported.

BUG=396760
Committed: https://crrev.com/369802998188398963403e612b2a3cb594e52d07
Cr-Commit-Position: refs/heads/master@{#298719}

[jonross, 2014-09-04 21:12:16]

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

[jonross, 2014-09-04 21:12:17]

Hi Mounir,

In this work in progress change I have pulled the creation of a factory for
ScreenOrientationProvider into a separate change. I will comment on the
SetProvider review with my concerns and link them to this.

[mlamouri (slow - plz ping), 2014-09-08 09:10:38]

I will review this after we discuss the plans in
https://codereview.chromium.org/513073002/ so we can solve CrOS and Samsung
requirements together.

[jonross, 2014-09-16 18:48:27]

Hi Mounir,

As a followup to exposing ScreenOrientationProvider, this change creates a
factory so that other libraries can provide their own implementations.

[mlamouri (slow - plz ping), 2014-09-16 20:21:04]

I'm not sure that even compiles on Android. You would also need to modify the
current code to create a factory for Android.

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
content/public/browser/screen_orientation_provider.cc:14:
ScreenOrientationProvider::ScreenOrientationProvider::Create(
isn't there one ScreenOrientationProvider too many?

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
content/public/browser/screen_orientation_provider.cc:19: return NULL;
nit: I would do:
if (!factory_)
  return NULL;
return factory_->Create(...);

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

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
content/public/browser/screen_orientation_provider.h:38: static
ScreenOrientationProvider* Create(
Should that be an own ptr?

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
File content/public/browser/screen_orientation_provider_factory.h (right):

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
content/public/browser/screen_orientation_provider_factory.h:26: virtual
ScreenOrientationProvider* Create(
ditto, should that be an own ptr?

[jonross, 2014-09-16 21:48:13]

I brought back the #!defined back to fix the Android build.

I would be willing to switch Android to using the factory, I would just need to
find an appropriate place within Android to create/own the factory.

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
content/public/browser/screen_orientation_provider.cc:14:
ScreenOrientationProvider::ScreenOrientationProvider::Create(
On 2014/09/16 20:21:04, Mounir Lamouri wrote:
> isn't there one ScreenOrientationProvider too many?

Yeah... not sure why it didn't complain

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
content/public/browser/screen_orientation_provider.cc:19: return NULL;
On 2014/09/16 20:21:04, Mounir Lamouri wrote:
> nit: I would do:
> if (!factory_)
>   return NULL;
> return factory_->Create(...);

Done.

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

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
content/public/browser/screen_orientation_provider.h:38: static
ScreenOrientationProvider* Create(
On 2014/09/16 20:21:04, Mounir Lamouri wrote:
> Should that be an own ptr?

Doing a code search for 'own ptr' points me to wtf::ownptr which is used heavily
in third_party/WebKit

Is there an equivalent being used within the content/ package?

[mlamouri (slow - plz ping), 2014-09-16 22:00:45]

Regarding where to setup the factory for Chromium Android, where do you plan to
set it up for CrOS?

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

https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
content/public/browser/screen_orientation_provider.h:38: static
ScreenOrientationProvider* Create(
On 2014/09/16 21:48:13, jonross wrote:
> On 2014/09/16 20:21:04, Mounir Lamouri wrote:
> > Should that be an own ptr?
> 
> Doing a code search for 'own ptr' points me to wtf::ownptr which is used
heavily
> in third_party/WebKit
> 
> Is there an equivalent being used within the content/ package?

Sorry about that. I got confused with WebKit indeed ;)

You can return a scoped_ptr<ScreenOrientationProvider> to mark that the caller
owns the returned pointer. It will prevent accidental leaks too.

[jonross, 2014-09-16 22:08:56]

For CrOS I plan to use ash/shell.h. It is our location for system lifetime
objects. The CrOS implementation will also be reliant on ash.

On 2014/09/16 22:00:45, Mounir Lamouri wrote:
> Regarding where to setup the factory for Chromium Android, where do you plan
to
> set it up for CrOS?
> 
>
https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
> File content/public/browser/screen_orientation_provider.h (right):
> 
>
https://codereview.chromium.org/546453004/diff/40001/content/public/browser/s...
> content/public/browser/screen_orientation_provider.h:38: static
> ScreenOrientationProvider* Create(
> On 2014/09/16 21:48:13, jonross wrote:
> > On 2014/09/16 20:21:04, Mounir Lamouri wrote:
> > > Should that be an own ptr?
> > 
> > Doing a code search for 'own ptr' points me to wtf::ownptr which is used
> heavily
> > in third_party/WebKit
> > 
> > Is there an equivalent being used within the content/ package?
> 
> Sorry about that. I got confused with WebKit indeed ;)
> 
> You can return a scoped_ptr<ScreenOrientationProvider> to mark that the caller
> owns the returned pointer. It will prevent accidental leaks too.

[jonross, 2014-09-17 14:17:20]

scoped_ptr added.

[mlamouri (slow - plz ping), 2014-09-20 15:43:25]

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

[mlamouri (slow - plz ping), 2014-09-20 15:43:25]

Looks good to me but I'm a bit worried by having an Android-specific code path
here. I would prefer if we could find a place in content/ where we could plug
factories easily.

After a quick look around, I wonder if we could use content::BrowserMainLoop.
There seems to be a few factories set there and other kind of static
initialization. We could maybe create a ScreenOrientationProviderFactory for
Android and set it there?

+jochen@ for his opinion and also because you will need an owner review anyway.

[jonross, 2014-09-25 14:46:22]

Patchset #6 (id:100001) has been deleted

[jonross, 2014-09-25 14:47:16]

I have created a factory for the Android implementation. All creation now passes
through ScreenOrientationProvider which makes use of the factory.

[mlamouri (slow - plz ping), 2014-09-25 15:47:54]

Thanks! :)

lgtm

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

it strikes me as strange to have part of the feature implemented inside content
and the other part outside of it

is it possible to make the screen orientation provider take a delegate that
provides the raw data? both android and ash could implement that then outside of
content. that way, they'd also share more code.

[jonross, 2014-10-01 17:49:10]

The majority of the logic from ScreenOrientationProviderAndroid has now been
centralized within ScreenOrientationProvider. This covers the basic logic around
locking/unlocking and notifying the dispatcher of success.

A ScreenOrientationDelegate has been added to handle platform specific
implementations. Such as actual locking/unlocking, as well as if the actions are
supports. This has an Android implementation in this review. A future review
will have ash implement a delegate.

[jochen (gone - plz use gerrit), 2014-10-02 08:00:53]

great!

if we'd move the android implementation from content to chrome, we wouldn't need
the factory at all.

do you think that's possible?

[mlamouri (slow - plz ping), 2014-10-02 10:37:12]

On 2014/10/02 08:00:53, jochen wrote:
> great!
> 
> if we'd move the android implementation from content to chrome, we wouldn't
need
> the factory at all.
> 
> do you think that's possible?

Layering-wise, everything lives in content/ (even the Java parts) so it might
require having some public interfaces. ScreenOrientationProvider.java might need
to move to content_public.

I wonder if that implementation should really be considered as part of content/
or chrome/ but I trust you to know better than me ;)

[jochen (gone - plz use gerrit), 2014-10-02 10:59:27]

On 2014/10/02 at 10:37:12, mlamouri wrote:
> On 2014/10/02 08:00:53, jochen wrote:
> > great!
> > 
> > if we'd move the android implementation from content to chrome, we wouldn't
need
> > the factory at all.
> > 
> > do you think that's possible?
> 
> Layering-wise, everything lives in content/ (even the Java parts) so it might
require having some public interfaces. ScreenOrientationProvider.java might need
to move to content_public.
> 
> I wonder if that implementation should really be considered as part of
content/ or chrome/ but I trust you to know better than me ;)

having parts in content and parts in chrome is odd.

with the new structure, we'd just need a ScreenOrientationDelegate.java in
chrome, no?

[jonross, 2014-10-02 14:17:19]

On 2014/10/02 10:59:27, jochen wrote:
> On 2014/10/02 at 10:37:12, mlamouri wrote:
> > On 2014/10/02 08:00:53, jochen wrote:
> > > great!
> > > 
> > > if we'd move the android implementation from content to chrome, we
wouldn't
> need
> > > the factory at all.
> > > 
> > > do you think that's possible?
> > 
> > Layering-wise, everything lives in content/ (even the Java parts) so it
might
> require having some public interfaces. ScreenOrientationProvider.java might
need
> to move to content_public.
> > 
> > I wonder if that implementation should really be considered as part of
> content/ or chrome/ but I trust you to know better than me ;)
> 
> having parts in content and parts in chrome is odd.
> 
> with the new structure, we'd just need a ScreenOrientationDelegate.java in
> chrome, no?

One difficulty I see with moving the Android code out is that there are several
entry points for creating WebContents throughout (49 across chrome, components,
content, extensions, ui.) I would think that having a single entry point for
creating the platform specific ScreenOrientationProvider would still be
beneficial.

Moving the android implementation out of content sounds good, however this
change is growing in complexity, I'd prefer to land that as a subsequent change.

[jochen (gone - plz use gerrit), 2014-10-06 07:19:11]

On 2014/10/02 at 14:17:19, jonross wrote:
> On 2014/10/02 10:59:27, jochen wrote:
> > On 2014/10/02 at 10:37:12, mlamouri wrote:
> > > On 2014/10/02 08:00:53, jochen wrote:
> > > > great!
> > > > 
> > > > if we'd move the android implementation from content to chrome, we
wouldn't
> > need
> > > > the factory at all.
> > > > 
> > > > do you think that's possible?
> > > 
> > > Layering-wise, everything lives in content/ (even the Java parts) so it
might
> > require having some public interfaces. ScreenOrientationProvider.java might
need
> > to move to content_public.
> > > 
> > > I wonder if that implementation should really be considered as part of
> > content/ or chrome/ but I trust you to know better than me ;)
> > 
> > having parts in content and parts in chrome is odd.
> > 
> > with the new structure, we'd just need a ScreenOrientationDelegate.java in
> > chrome, no?
> 
> One difficulty I see with moving the Android code out is that there are
several entry points for creating WebContents throughout (49 across chrome,
components, content, extensions, ui.) I would think that having a single entry
point for creating the platform specific ScreenOrientationProvider would still
be beneficial.

True, I'm not suggesting to change that.

> 
> Moving the android implementation out of content sounds good, however this
change is growing in complexity, I'd prefer to land that as a subsequent change.

There's no need to have the factory. If you want to do this in two changes, then
just split the provider into a provider and a delegate, where android is the
only existing delegate.

In the follow-up, you can move the android provider out of content, and set the
delegate via a public API (e.g. a static SetDelegate method on
ScreenOrientationProvider)

[jonross, 2014-10-07 21:41:09]

Sorry, I misinterpreted what your idea was. I hadn't thought of simply making
the delegate static and set directly on the provider.

Change updated, the factories are now gone.

[mlamouri (slow - plz ping), 2014-10-08 09:13:18]

still lgtm

https://codereview.chromium.org/546453004/diff/160001/content/public/browser/...
File content/public/browser/screen_orientation_delegate.h (right):

https://codereview.chromium.org/546453004/diff/160001/content/public/browser/...
content/public/browser/screen_orientation_delegate.h:23: // Returns true if the
tab must be fullscreen in order for
nit: s/tab/|web_contents|/ ?

[jochen (gone - plz use gerrit), 2014-10-08 15:00:23]

lgtm, thanks

[jonross, 2014-10-08 15:02:22]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-10-08 15:03:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/546453004/160001

[commit-bot: I haz the power, 2014-10-08 15:55:24]

Committed patchset #8 (id:160001) as 6b8352af31e3c90a39ad5d36ab41cc1bc1d0f7e3

[commit-bot: I haz the power, 2014-10-08 15:56:11]

Patchset 8 (id:??) landed as
https://crrev.com/369802998188398963403e612b2a3cb594e52d07
Cr-Commit-Position: refs/heads/master@{#298719}