[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

This CL also removes declarition of
content::RenderFrameHostDelegate::GetScreenOrientationProvider() because
no codes expect RenderFrameHostDelegate has such a method.

BUG=678545
TEST=content_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2649823002
Cr-Commit-Position: refs/heads/master@{#448617}
Committed: https://chromium.googlesource.com/chromium/src/+/e4db177a6f1e8cde2f3f85788161d42dab5cb8e0

[leonhsl(Using Gerrit), 2017-01-23 04:54:39]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 04:54:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 06:24:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 06:24:27]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-01-23 07:15:01]

Description was changed from

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

BUG=678545
==========

to

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

BUG=678545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[leonhsl(Using Gerrit), 2017-01-23 07:15:38]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 07:15:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-01-23 07:18:59]

Description was changed from

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

BUG=678545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

This CL also removes declarition of
content::RenderFrameHostDelegate::GetScreenOrientationProvider() because
no codes expect RenderFrameHostDelegate has such a method.

BUG=678545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-01-23 08:28:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 08:28:43]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-01-23 09:57:10]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 09:57:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-01-23 10:02:34]

Description was changed from

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

This CL also removes declarition of
content::RenderFrameHostDelegate::GetScreenOrientationProvider() because
no codes expect RenderFrameHostDelegate has such a method.

BUG=678545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

This CL also removes declarition of
content::RenderFrameHostDelegate::GetScreenOrientationProvider() because
no codes expect RenderFrameHostDelegate has such a method.

BUG=678545
Test=content_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[leonhsl(Using Gerrit), 2017-01-23 10:02:47]

Description was changed from

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

This CL also removes declarition of
content::RenderFrameHostDelegate::GetScreenOrientationProvider() because
no codes expect RenderFrameHostDelegate has such a method.

BUG=678545
Test=content_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

This CL also removes declarition of
content::RenderFrameHostDelegate::GetScreenOrientationProvider() because
no codes expect RenderFrameHostDelegate has such a method.

BUG=678545
TEST=content_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[leonhsl(Using Gerrit), 2017-01-23 10:11:09]

leon.han@intel.com changed reviewers:
+ blundell@chromium.org, mlamouri@chromium.org

[leonhsl(Using Gerrit), 2017-01-23 10:11:10]

PTAL with my attached questions :) Thanks!

https://codereview.chromium.org/2649823002/diff/40001/content/browser/screen_...
File content/browser/screen_orientation/screen_orientation_provider_unittest.cc
(right):

https://codereview.chromium.org/2649823002/diff/40001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:14:
namespace content {
Currently this file is just to show a sketch, if the sketch is OK I'll try to
add more tests for various scenarios.

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

https://codereview.chromium.org/2649823002/diff/40001/content/public/browser/...
content/public/browser/screen_orientation_provider.h:45: static void
SetDelegate(ScreenOrientationDelegate* delegate_);
I'm not sure whether we should move all implementations of
ScreenOrientationProvider to
content/browser/screen_orientation/screen_orientation_provider_impl.cc, except
this SetDelegate(), which seems is the only one needed to be content public.

[commit-bot: I haz the power, 2017-01-23 11:52:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 11:52:17]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-01-24 09:58:24]

LGTM

I was going to ask whether your unittests would be able to catch the problem
fixed in your last CL, but I realized that this refactoring makes that problem
obsolete, so it's not a meaningful question.

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

https://codereview.chromium.org/2649823002/diff/40001/content/public/browser/...
content/public/browser/screen_orientation_provider.h:45: static void
SetDelegate(ScreenOrientationDelegate* delegate_);
On 2017/01/23 10:11:10, leonhsl wrote:
> I'm not sure whether we should move all implementations of
> ScreenOrientationProvider to
> content/browser/screen_orientation/screen_orientation_provider_impl.cc, except
> this SetDelegate(), which seems is the only one needed to be content public.

I'm tempted to completely hide ScreenOrientationProvider inside WebContentsImpl.
That would require only the following AFAICT:

- Exposing SetScreenOrientationDelegate() on web_contents.h
- Exposing OnScreenOrientationChange() on web_contents_impl.h

Then it would be quite clear what the touchpoints of ScreenOrientationProvider
with the rest of the browser process are, which makes it all the easier to
analyze them (and to avoid those touchpoints growing larger as we try to
eliminate them). Mounir, what do you think?

[blundell, 2017-01-24 09:58:45]

To be clear, my suggested refactoring is for a followup CL :).

[leonhsl(Using Gerrit), 2017-01-25 10:37:44]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 10:38:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-01-25 10:52:21]

Uploaded ps#4 to add more tests, PTAnL, Thanks!

https://codereview.chromium.org/2649823002/diff/60001/content/browser/screen_...
File content/browser/screen_orientation/screen_orientation_provider_unittest.cc
(right):

https://codereview.chromium.org/2649823002/diff/60001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:39:
lock_count_++;
I can't find a way to enable the fake screen orientation delegate to really do
the lock operation on the web contents so that
ScreenOrientationProvider::LockMatchesCurrentOrientation() could return true and
then resolve the pending lock request with a success result.
So simply record count here.

[leonhsl(Using Gerrit), 2017-01-25 10:58:17]

On 2017/01/24 09:58:45, blundell wrote:
> To be clear, my suggested refactoring is for a followup CL :).

Thanks for the suggestion! I think it will make things much more clear, and I'd
like to do it with a followup CL if Mounir is also happy about this :)

[commit-bot: I haz the power, 2017-01-25 11:43:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 11:43:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[blundell, 2017-01-26 08:03:05]

Is the redness on the trybots related to this change?

[leonhsl(Using Gerrit), 2017-01-26 08:52:58]

On 2017/01/26 08:03:05, blundell wrote:
> Is the redness on the trybots related to this change?

Yeah several newly added tests are failing, I'm checking how to fix now :)

[leonhsl(Using Gerrit), 2017-01-26 11:01:06]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[leonhsl(Using Gerrit), 2017-01-26 11:01:22]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-01-26 11:01:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-01-26 12:11:32]

Uploaded ps#5 to fix tests, trybots have turned green, PTAnL, Thanks.

[commit-bot: I haz the power, 2017-01-26 12:22:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 12:22:56]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-01-26 15:03:45]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2017-01-26 15:03:46]

Drive-by comment: I see that you are removing ways for frames to get their
screen orientation. In general, we need to update screen orientation to work
with out-of-process iframes, so I want to ensure we aren't going in a direction
making it harder for us to make the orientation a property that each frame knows
about.

[blundell, 2017-01-26 15:32:37]

On 2017/01/26 15:03:46, nasko wrote:
> Drive-by comment: I see that you are removing ways for frames to get their
> screen orientation. In general, we need to update screen orientation to work
> with out-of-process iframes, so I want to ensure we aren't going in a
direction
> making it harder for us to make the orientation a property that each frame
knows
> about.

Hi Nasko, presumably it's the removal of the method in RenderFrameHostDelegate
that's concerning you? I think this was done in this CL just because it's not
currently needed in the codebase (as evidenced by the fact that no callsites had
to be altered). We could keep it in RFHDelegate (i.e., it's orthogonal to the
work this CL is doing). As long as it's a dead method though, it would always be
vulnerable to being removed until it starts getting used. In general we will be
looking at redesigning screen orientation to servicify it, so I think that your
concern is well-founded (i.e., by default we won't design for properties that we
don't see in the codebase as it stands today). We're designing that
servicification now-ish. What's the timeframe for getting screen orientation
working with OOPIF?

[nasko, 2017-01-26 17:36:10]

On 2017/01/26 15:32:37, blundell wrote:
> On 2017/01/26 15:03:46, nasko wrote:
> > Drive-by comment: I see that you are removing ways for frames to get their
> > screen orientation. In general, we need to update screen orientation to work
> > with out-of-process iframes, so I want to ensure we aren't going in a
> direction
> > making it harder for us to make the orientation a property that each frame
> knows
> > about.
> 
> Hi Nasko, presumably it's the removal of the method in RenderFrameHostDelegate
> that's concerning you? I think this was done in this CL just because it's not
> currently needed in the codebase (as evidenced by the fact that no callsites
had
> to be altered). We could keep it in RFHDelegate (i.e., it's orthogonal to the
> work this CL is doing). As long as it's a dead method though, it would always
be
> vulnerable to being removed until it starts getting used. 

Yes, removing dead code is always good! I just wanted to raise awareness.

> In general we will be
> looking at redesigning screen orientation to servicify it, so I think that
your
> concern is well-founded (i.e., by default we won't design for properties that
we
> don't see in the codebase as it stands today). We're designing that
> servicification now-ish. What's the timeframe for getting screen orientation
> working with OOPIF?

We don't have anyone that is actively working on it and not sure when we will
do. However, if you are servicifying it now, it will be great to be designed in
a way that is compatible with OOPIF. If you end up fixing it to work with OOPIFs
as you are moving it to a service, I will be very very grateful ;).

[leonhsl(Using Gerrit), 2017-01-27 04:10:41]

On 2017/01/26 17:36:10, nasko wrote:
> On 2017/01/26 15:32:37, blundell wrote:
> > On 2017/01/26 15:03:46, nasko wrote:
> > > Drive-by comment: I see that you are removing ways for frames to get their
> > > screen orientation. In general, we need to update screen orientation to
work
> > > with out-of-process iframes, so I want to ensure we aren't going in a
> > direction
> > > making it harder for us to make the orientation a property that each frame
> > knows
> > > about.
> > 
> > Hi Nasko, presumably it's the removal of the method in
RenderFrameHostDelegate
> > that's concerning you? I think this was done in this CL just because it's
not
> > currently needed in the codebase (as evidenced by the fact that no callsites
> had
> > to be altered). We could keep it in RFHDelegate (i.e., it's orthogonal to
the
> > work this CL is doing). As long as it's a dead method though, it would
always
> be
> > vulnerable to being removed until it starts getting used. 
> 
> Yes, removing dead code is always good! I just wanted to raise awareness.
> 
> > In general we will be
> > looking at redesigning screen orientation to servicify it, so I think that
> your
> > concern is well-founded (i.e., by default we won't design for properties
that
> we
> > don't see in the codebase as it stands today). We're designing that
> > servicification now-ish. What's the timeframe for getting screen orientation
> > working with OOPIF?
> 
> We don't have anyone that is actively working on it and not sure when we will
> do. However, if you are servicifying it now, it will be great to be designed
in
> a way that is compatible with OOPIF. If you end up fixing it to work with
OOPIFs
> as you are moving it to a service, I will be very very grateful ;).

Hi, nasko@, I'm not sure about what difference would OOPIF bring in for screen
orientation. Maybe my understanding bellow is non-sense concerning with OOPIF,
would you please help me to understand better? Thanks.

I think currently screen orientation should also be able to work well even with
OOPIF, because screen_orientation.{lock,unlock} requests are sent via the
frame-level mojo connection, no matter which renderer process the frame exists,
browser side would target on the WebContents associated with the corresponding
frame host to handle these requests. Specifically, currently one WebContents's
ScreenOrientationProvider is already be able to handle {lock,unlock} requests
from multiple render frames in multiple renderer processes.

Also, after mlamouri@'s OK review as screen_orientation OWNER, would you PTAL
for an overall review as content/ OWNER? Thanks :)

[blundell, 2017-01-27 12:57:23]

Mounir: friendly ping. Will you be able to get to this review soon? If not, do
you have another reviewer to suggest?

[mlamouri (slow - plz ping), 2017-01-28 02:17:51]

This CL looks fantastic! :)

lgtm

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_provider_unittest.cc
(right):

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:29:
~FakeScreenOrientationDelegate() override {}
= default;

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:45:
int unlock_count() { return unlock_count_; }
lock_count() and unlock_count() should be `const`

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:60:
~FakeWebContentsDelegate() override {}
= default; for both ctor and dtor

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:77:
WebContents* fullscreened_contents_;
= nullptr; so you can have a `= default;` ctor


... 


maybe I like `= default;` too much? :)

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:91:
ScreenOrientationProviderTest() {}
oh another one :D

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:154:
EXPECT_FALSE(result_1);
`.has_value()` just to be explicit maybe?

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:191:
EXPECT_FALSE(result_2);
ditto and for the following

https://codereview.chromium.org/2649823002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2649823002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:929: const {
curiosity (not the spaceship): is `git cl format` producing this wrapping? I
would expect something with the return type in its own line

[leonhsl(Using Gerrit), 2017-02-04 02:35:57]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 02:36:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-02-04 03:10:48]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 03:11:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-02-04 03:13:45]

Hi, nasko@, would you PTAL for content/ OWNER review? Thanks. 
ps#7 has addressed all nit comments from mlamouri@.

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_provider_unittest.cc
(right):

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:29:
~FakeScreenOrientationDelegate() override {}
On 2017/01/28 02:17:51, mlamouri (slow - travels) wrote:
> = default;

Done.

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:45:
int unlock_count() { return unlock_count_; }
On 2017/01/28 02:17:51, mlamouri (slow - travels) wrote:
> lock_count() and unlock_count() should be `const`

Done.

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:60:
~FakeWebContentsDelegate() override {}
On 2017/01/28 02:17:51, mlamouri (slow - travels) wrote:
> = default; for both ctor and dtor

Done.

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:77:
WebContents* fullscreened_contents_;
On 2017/01/28 02:17:51, mlamouri (slow - travels) wrote:
> = nullptr; so you can have a `= default;` ctor
> 
> 
> ... 
> 
> 
> maybe I like `= default;` too much? :)

Done :)

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:91:
ScreenOrientationProviderTest() {}
On 2017/01/28 02:17:51, mlamouri (slow - travels) wrote:
> oh another one :D

Done.

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:154:
EXPECT_FALSE(result_1);
On 2017/01/28 02:17:51, mlamouri (slow - travels) wrote:
> `.has_value()` just to be explicit maybe?

Done.

https://codereview.chromium.org/2649823002/diff/100001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_provider_unittest.cc:191:
EXPECT_FALSE(result_2);
On 2017/01/28 02:17:51, mlamouri (slow - travels) wrote:
> ditto and for the following

Done.

https://codereview.chromium.org/2649823002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2649823002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:929: const {
On 2017/01/28 02:17:51, mlamouri (slow - travels) wrote:
> curiosity (not the spaceship): is `git cl format` producing this wrapping? I
> would expect something with the return type in its own line

Yeah this is produced by 'git cl format'. I changed to let the return type be in
one separate line but 'git cl format' will restore it again. So I think we'd
better follow 'git cl format' otherwise we may need to care about this every
time when we change here in future.

[commit-bot: I haz the power, 2017-02-04 05:11:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 05:11:50]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
no build URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)
  win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[leonhsl(Using Gerrit), 2017-02-04 05:12:56]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 05:13:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 07:13:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 07:14:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
no build URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)
  win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[leonhsl(Using Gerrit), 2017-02-04 07:23:27]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 07:23:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 09:24:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 09:24:22]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
no build URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)
  win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[leonhsl(Using Gerrit), 2017-02-04 09:33:07]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 09:33:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 11:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 11:33:54]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
no build URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)
  win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[leonhsl(Using Gerrit), 2017-02-07 05:02:20]

Friendly ping nasko@, Thanks.

[leonhsl(Using Gerrit), 2017-02-07 05:04:50]

Hi, mlamouri@, WDYT about suggestions in 
https://codereview.chromium.org/2649823002/#msg19? Thanks.

[leonhsl(Using Gerrit), 2017-02-07 07:05:20]

Patchset #7 (id:140001) has been deleted

[leonhsl(Using Gerrit), 2017-02-07 07:05:28]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 07:05:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 08:24:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 08:24:43]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-02-07 09:17:00]

leon.han@intel.com changed reviewers:
+ kinuko@chromium.org

[leonhsl(Using Gerrit), 2017-02-07 09:17:01]

+kinuko@, would you mind to help take a took for content/ OWNER review, too?
Thanks. #I suppose nasko@ has not started the review yet :)

[kinuko, 2017-02-07 11:53:32]

Putting aside nasko's concern about having this work well with OOPIF, this
change itself lgtm as a nice cleanup.

https://codereview.chromium.org/2649823002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (left):

https://codereview.chromium.org/2649823002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:191: virtual
ScreenOrientationProvider* GetScreenOrientationProvider();
I see that currently no one calls this...

[kinuko, 2017-02-07 11:54:09]

On 2017/02/07 11:53:32, kinuko wrote:
> Putting aside nasko's concern about having this work well with OOPIF, this

having this work well -> having ScreenOrientation work well

> change itself lgtm as a nice cleanup.
> 
>
https://codereview.chromium.org/2649823002/diff/160001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_delegate.h (left):
> 
>
https://codereview.chromium.org/2649823002/diff/160001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_delegate.h:191: virtual
> ScreenOrientationProvider* GetScreenOrientationProvider();
> I see that currently no one calls this...

[mlamouri (slow - plz ping), 2017-02-07 12:26:35]

On 2017/02/07 at 05:04:50, leon.han wrote:
> Hi, mlamouri@, WDYT about suggestions in 
https://codereview.chromium.org/2649823002/#msg19? Thanks.

Sorry, I completely missed this comment. I see that you uploaded a CL so I will
have a look there. You should land this in the meantime though.

[leonhsl(Using Gerrit), 2017-02-07 14:14:44]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2017-02-07 14:14:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2649823002/#ps160001
(title: "Address comments from mlamouri@")

[commit-bot: I haz the power, 2017-02-07 14:15:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 14:19:20]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486476884644250,
"parent_rev": "aa8ec6485091dda9862eb62960b881a60ef9e132", "commit_rev":
"e4db177a6f1e8cde2f3f85788161d42dab5cb8e0"}

[commit-bot: I haz the power, 2017-02-07 14:19:53]

Description was changed from

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

This CL also removes declarition of
content::RenderFrameHostDelegate::GetScreenOrientationProvider() because
no codes expect RenderFrameHostDelegate has such a method.

BUG=678545
TEST=content_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[ScreenOrientation] Merge ScreenOrientation into ScreenOrientationProvider.

ScreenOrientation has no actual functionality other than forwarding
lock/unlock requests to ScreenOrientationProvider, so this CL
merges it into ScreenOrientationProvider and removes it.

This CL also removes declarition of
content::RenderFrameHostDelegate::GetScreenOrientationProvider() because
no codes expect RenderFrameHostDelegate has such a method.

BUG=678545
TEST=content_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2649823002
Cr-Commit-Position: refs/heads/master@{#448617}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4db177a6f1e8cde2f3f85788161...
==========

[commit-bot: I haz the power, 2017-02-07 14:19:55]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/e4db177a6f1e8cde2f3f85788161...