Add SetProvider function

Add SetProvider function

This CL will move static function ScreenOrientationProvider::Create implementation
to cc file. So, other platforms can also include screen_orientation_provider.h and
won't have many times defined error. Meanwhile, add SetProvider function so that
other platform can inherit class ScreenOrientationProvider and reset the provider
rather than null.

R=mlamouri@chromium.org
BUG=408573

[jiajia.qin, 2014-08-28 07:22:42]

Hi, Lamouri. Please help to review it.

[mlamouri (slow - plz ping), 2014-08-28 09:53:34]

On 2014/08/28 07:22:42, jiajia.qin wrote:
> Hi, Lamouri. Please help to review it.

The idea was that new implementation would create a
screen_orientation_provider_foo.{h,cpp} and change the #if !defined(OS_ANDROID)
to #if !(defined(OS_ANDROID) || defined(FOO)).
Why wouldn't that work here?

[jiajia.qin, 2014-08-28 16:53:02]

On 2014/08/28 09:53:34, Mounir Lamouri wrote:
> On 2014/08/28 07:22:42, jiajia.qin wrote:
> > Hi, Lamouri. Please help to review it.
> 
> The idea was that new implementation would create a
> screen_orientation_provider_foo.{h,cpp} and change the #if
!defined(OS_ANDROID)
> to #if !(defined(OS_ANDROID) || defined(FOO)).
> Why wouldn't that work here?

See bug https://code.google.com/p/chromium/issues/detail?id=408573

[Raphael Kubo da Costa (rakuco), 2014-08-29 10:46:12]

On 2014/08/28 09:53:34, Mounir Lamouri wrote:
> The idea was that new implementation would create a
> screen_orientation_provider_foo.{h,cpp} and change the #if
!defined(OS_ANDROID)
> to #if !(defined(OS_ANDROID) || defined(FOO)).
> Why wouldn't that work here?

It would, but the check might end up getting quite big depending on the number
of platforms and, most importantly, it's not very friendly to downstreams, as
subclassing ScreenOrientationProvider always involves introducing a change to
the content layer instead of just consuming it as a user.

On 2014-08-28 16:53:02, jiajia.qin wrote:
> See bug https://code.google.com/p/chromium/issues/detail?id=408573

It would be helpful if you mentioned it in the CL instead of having BUG=none
then.

[jiajia.qin, 2014-09-02 07:21:47]

On 2014/08/29 10:46:12, Raphael Kubo da Costa (rakuco) wrote:
> On 2014-08-28 16:53:02, jiajia.qin wrote:
> > See bug https://code.google.com/p/chromium/issues/detail?id=408573
> 
> It would be helpful if you mentioned it in the CL instead of having BUG=none
> then.
Done.

Hi, Lamouri. What's your opinion?

[mlamouri (slow - plz ping), 2014-09-03 12:39:55]

In your implementation, ScreenOrientationProvider::Create() will return NULL and
then you will inject the right provider? Is that correct?

Also, would that CL help you in any way?
https://codereview.chromium.org/508603003/

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

On 2014/09/03 12:39:55, Mounir Lamouri wrote:
> In your implementation, ScreenOrientationProvider::Create() will return NULL
and
> then you will inject the right provider? Is that correct?
> 
> Also, would that CL help you in any way?
> https://codereview.chromium.org/508603003/

Could you rebase your CL on top of https://codereview.chromium.org/508603003/
and rename the title to actually point the two things this CL is doing? Thanks
:)

[jonross, 2014-09-04 21:18:58]

On 2014/09/04 17:19:08, Mounir Lamouri wrote:
> On 2014/09/03 12:39:55, Mounir Lamouri wrote:
> > In your implementation, ScreenOrientationProvider::Create() will return NULL
> and
> > then you will inject the right provider? Is that correct?
> > 
> > Also, would that CL help you in any way?
> > https://codereview.chromium.org/508603003/
> 
> Could you rebase your CL on top of https://codereview.chromium.org/508603003/
> and rename the title to actually point the two things this CL is doing? Thanks
> :)

Hi,

In the review that Mounir linked I move ScreenOrientationProvider to allow for
its export so that other libraries can provide their own implementations. As a
follow up I have been considering the use of a factory for the creation of
ScreenOrientationProvider.

This is because the instances of ScreenOrientationDispatcher are held within
WebContentsImpl. It may be difficult for implementing platforms to access these
to then call your SetProvider. There are multiple points for the creation of
WebContentsImpl outside of content/browser, with many in chrome/browser. I'm
hesitant to see implementing platforms having to modify these locations to add
their own provider. This could lead to new dependencies, such as when I
implement a provider from within ash/

Could you take a look at the factory review and provide your thoughts?

https://codereview.chromium.org/546453004/

[jiajia.qin, 2014-09-05 14:38:50]

On 2014/09/04 21:18:58, jonross wrote:
> On 2014/09/04 17:19:08, Mounir Lamouri wrote:
> > On 2014/09/03 12:39:55, Mounir Lamouri wrote:
> > > In your implementation, ScreenOrientationProvider::Create() will return
NULL
> > and
> > > then you will inject the right provider? Is that correct?
> > > 
> > > Also, would that CL help you in any way?
> > > https://codereview.chromium.org/508603003/
> > 
> > Could you rebase your CL on top of
https://codereview.chromium.org/508603003/
> > and rename the title to actually point the two things this CL is doing?
Thanks
> > :)
> 
> Hi,
> 
> In the review that Mounir linked I move ScreenOrientationProvider to allow for
> its export so that other libraries can provide their own implementations. As a
> follow up I have been considering the use of a factory for the creation of
> ScreenOrientationProvider.
> 
> This is because the instances of ScreenOrientationDispatcher are held within
> WebContentsImpl. It may be difficult for implementing platforms to access
these
> to then call your SetProvider. There are multiple points for the creation of
> WebContentsImpl outside of content/browser, with many in chrome/browser. I'm
> hesitant to see implementing platforms having to modify these locations to add
> their own provider. This could lead to new dependencies, such as when I
> implement a provider from within ash/
> 
> Could you take a look at the factory review and provide your thoughts?
> 
> https://codereview.chromium.org/546453004/

What I want to do is let ScreenOrientationDispatcher can change its own provider
so that somewhere outside of content/ can inherit ScreenOrientationProvider and
ScreenOrientationDispatcher can change its provider to this new one.
The reason that I need to change DispatcherHost's provider is that my provider's
implementation is outside of content/. So when WebContentsImpl
is initilized, it can't see my implementation. It will use the original one.
Then, I have to use
web_contents->GetScreenOrientationDispatcherHost()->SetProvider to force inject
my provider to instead of the default NULL(for non android platform).  

Jonross, I am not very clear about your implementation
https://codereview.chromium.org/546453004/. Is this Cl's target same with the
current one?
It seems that it can't solve my problem. I can't see the new added function
SetFactory's purpose in your CL if you put it in  ScreenOrientationProvider.h

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

On 2014/09/05 14:38:50, jiajia.qin wrote:
> Jonross, I am not very clear about your implementation
> https://codereview.chromium.org/546453004/. Is this Cl's target same with the
> current one?
> It seems that it can't solve my problem. I can't see the new added function
> SetFactory's purpose in your CL if you put it in  ScreenOrientationProvider.h

Given that ScreenOrientationProvider would be moved to content/public, would you
still not be able to access it?

[jonross, 2014-09-08 15:06:35]

On 2014/09/08 09:09:24, Mounir Lamouri wrote:
> On 2014/09/05 14:38:50, jiajia.qin wrote:
> > Jonross, I am not very clear about your implementation
> > https://codereview.chromium.org/546453004/. Is this Cl's target same with
the
> > current one?
> > It seems that it can't solve my problem. I can't see the new added function
> > SetFactory's purpose in your CL if you put it in 
ScreenOrientationProvider.h
> 
> Given that ScreenOrientationProvider would be moved to content/public, would
you
> still not be able to access it?

jiajia.qln,

The code review where I add a factory
(https://codereview.chromium.org/546453004/) is based on another review
(https://codereview.chromium.org/508603003/) in which I am moving
ScreenOrientationProvide and ScreenOrientationDispatcherHost from
content/browser/screen_orientation/ to content/public/browser/

This is to allow for them to be exported and used by libraries other than
content.

The purpose of the SetFactory method would be to allow other libraries to
override the default creation of ScreenOrientationProvider, so that they can
provide their own implementation.

This is to address the same goal as your change, it just does not require
libraries to actively get each WebContents object and set their own provider.

[jiajia.qin, 2014-09-09 11:43:49]

On 2014/09/08 09:09:24, Mounir Lamouri wrote:
> On 2014/09/05 14:38:50, jiajia.qin wrote:
> > Jonross, I am not very clear about your implementation
> > https://codereview.chromium.org/546453004/. Is this Cl's target same with
the
> > current one?
> > It seems that it can't solve my problem. I can't see the new added function
> > SetFactory's purpose in your CL if you put it in 
ScreenOrientationProvider.h
> 
> Given that ScreenOrientationProvider would be moved to content/public, would
you
> still not be able to access it?

Hi, Mounir. Thanks for your advice. Based on
https://codereview.chromium.org/546453004/, it can see my implementation. It
seems that my cl doesn't needed.

[jiajia.qin, 2014-09-09 11:47:49]

On 2014/09/09 11:43:49, jiajia.qin wrote:
> On 2014/09/08 09:09:24, Mounir Lamouri wrote:
> > On 2014/09/05 14:38:50, jiajia.qin wrote:
> > > Jonross, I am not very clear about your implementation
> > > https://codereview.chromium.org/546453004/. Is this Cl's target same with
> the
> > > current one?
> > > It seems that it can't solve my problem. I can't see the new added
function
> > > SetFactory's purpose in your CL if you put it in 
> ScreenOrientationProvider.h
> > 
> > Given that ScreenOrientationProvider would be moved to content/public, would
> you
> > still not be able to access it?
> 
> Hi, Mounir. Thanks for your advice. Based on
> https://codereview.chromium.org/546453004/, it can see my implementation. It
> seems that my cl doesn't needed.

Sorry for the wrong link that I paste. The right one is
https://codereview.chromium.org/508603003/ which resolves my problem.

[mlamouri (slow - plz ping), 2014-09-09 12:00:40]

On 2014/09/09 11:43:49, jiajia.qin wrote:
> On 2014/09/08 09:09:24, Mounir Lamouri wrote:
> > On 2014/09/05 14:38:50, jiajia.qin wrote:
> > > Jonross, I am not very clear about your implementation
> > > https://codereview.chromium.org/546453004/. Is this Cl's target same with
> the
> > > current one?
> > > It seems that it can't solve my problem. I can't see the new added
function
> > > SetFactory's purpose in your CL if you put it in 
> ScreenOrientationProvider.h
> > 
> > Given that ScreenOrientationProvider would be moved to content/public, would
> you
> > still not be able to access it?
> 
> Hi, Mounir. Thanks for your advice. Based on
> https://codereview.chromium.org/546453004/, it can see my implementation. It
> seems that my cl doesn't needed.

Great! Feel free to close this CL then. We will get the other to land as soon as
possible.