[ScreenOrientation] Fix a possible crash point in ScreenOrientationProvider.

[ScreenOrientation] Fix a possible crash point in ScreenOrientationProvider.

ScreenOrientation does not need to maintain pending lock request,
just let ScreenOrientationProvider do that.

BUG=678545
Review-Url: https://codereview.chromium.org/2630323002
Cr-Commit-Position: refs/heads/master@{#445321}
Committed: https://chromium.googlesource.com/chromium/src/+/9aad73908ef510572cfe4504c36c2ece69438d1e

[leonhsl(Using Gerrit), 2017-01-16 10:50:35]

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

[commit-bot: I haz the power, 2017-01-16 10:50:49]

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-16 11:00:18]

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

[leonhsl(Using Gerrit), 2017-01-16 11:00:19]

I found this while investigating for IPCs refactor of screen_orientation, PTAL,
Thanks.

https://codereview.chromium.org/2630323002/diff/1/content/public/browser/scre...
File content/public/browser/screen_orientation_provider.cc (left):

https://codereview.chromium.org/2630323002/diff/1/content/public/browser/scre...
content/public/browser/screen_orientation_provider.cc:32:
DCHECK(on_result_callback_.is_null());
Although ScreenOrientation have cancelled the pending request, it can't clear
|on_result_callback_| here, |on_result_callback_| could still be non-null, we
should just reset all the pending status.

[commit-bot: I haz the power, 2017-01-16 11:37:04]

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

[commit-bot: I haz the power, 2017-01-16 11:37:05]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-01-16 13:02:03]

Thanks for noticing this! Do you think it would be more clear to add a
ScreenOrientationProvider::CancelLockRequest() method, which just calls its
callback (if it has one) with CANCELED and then clears its state? Then
ScreenOrientation would call that instead of directly calling its own
NotifyLockOrientation() method, and we could maintain the DCHECKs in the top of
ScreenOrientationProvider::LockOrientation().

[leonhsl(Using Gerrit), 2017-01-17 06:14:33]

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

[commit-bot: I haz the power, 2017-01-17 06:14:41]

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-17 06:22:30]

After further investigation I think we do not need to maintain the pending
request in both ScreenOrientation and ScreenOrientationProvider, we could just
put all those pending/cancel logic into ScreenOrientationProvider :)
Uploaded ps#2, PTAnL, Thanks.

[commit-bot: I haz the power, 2017-01-17 07:00:08]

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

[commit-bot: I haz the power, 2017-01-17 07:00:09]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-01-17 12:27:07]

lgtm, thanks!

[leonhsl(Using Gerrit), 2017-01-18 10:14:39]

Friendly ping mlamouri@ for OWNER review, Thanks.

[mlamouri (slow - plz ping), 2017-01-18 10:25:25]

lgtm but can you add tests? :)

[leonhsl(Using Gerrit), 2017-01-19 04:40:31]

On 2017/01/18 10:25:25, mlamouri wrote:
> lgtm but can you add tests? :)

No problem, I'll try to figure out one test for this :)

[leonhsl(Using Gerrit), 2017-01-22 06:39:15]

Description was changed from

==========
[ScreenOrientation] Fix a possible crash point in ScreenOrientationProvider.

BUG=
==========

to

==========
[ScreenOrientation] Fix a possible crash point in ScreenOrientationProvider.

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

[leonhsl(Using Gerrit), 2017-01-22 08:22:05]

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

[commit-bot: I haz the power, 2017-01-22 08:22: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-01-22 08:26:06]

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

[commit-bot: I haz the power, 2017-01-22 08:26:07]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[leonhsl(Using Gerrit), 2017-01-22 08:27:57]

Description was changed from

==========
[ScreenOrientation] Fix a possible crash point in ScreenOrientationProvider.

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
==========

[leonhsl(Using Gerrit), 2017-01-22 08:31:47]

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

[commit-bot: I haz the power, 2017-01-22 08:31:55]

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-22 08:31:59]

Patchset #3 (id:40001) has been deleted

[leonhsl(Using Gerrit), 2017-01-22 09:35:34]

Uploaded ps#3 to merge ScreenOrientation into ScreenOrientationProvider, PTAnL
firstly while I'm considering how to add some tests, Thanks.

[commit-bot: I haz the power, 2017-01-22 09:36:19]

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

[commit-bot: I haz the power, 2017-01-22 09:36:20]

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

[leonhsl(Using Gerrit), 2017-01-22 09:47:44]

Patchset #4 (id:80001) has been deleted

[leonhsl(Using Gerrit), 2017-01-22 09:47:51]

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

[commit-bot: I haz the power, 2017-01-22 09:47:59]

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-22 09:48:14]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-01-22 10:41:53]

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

[commit-bot: I haz the power, 2017-01-22 10:41:54]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-01-23 01:14:20]

On 2017/01/22 at 09:35:34, leon.han wrote:
> Uploaded ps#3 to merge ScreenOrientation into ScreenOrientationProvider, PTAnL
firstly while I'm considering how to add some tests, Thanks.

I would recommend to upload this in another CL instead. Uploading a largely
different CL after it was reviewed and approved isn't a good idea in general
because reviewer might easily forget to look at it. I clicked out of curiosity
to check the tests ;)

[leonhsl(Using Gerrit), 2017-01-23 02:03:40]

On 2017/01/23 01:14:20, mlamouri wrote:
> On 2017/01/22 at 09:35:34, leon.han wrote:
> > Uploaded ps#3 to merge ScreenOrientation into ScreenOrientationProvider,
PTAnL
> firstly while I'm considering how to add some tests, Thanks.
> 
> I would recommend to upload this in another CL instead. Uploading a largely
> different CL after it was reviewed and approved isn't a good idea in general
> because reviewer might easily forget to look at it. I clicked out of curiosity
> to check the tests ;)

Oh, I see, I'll separate ps#3 to another CL, and could we just land ps#2 here
and add tests in that new CL? Because some codes are supposed to be deleted in
that new CL.

[mlamouri (slow - plz ping), 2017-01-23 02:13:51]

On 2017/01/23 at 02:03:40, leon.han wrote:
> On 2017/01/23 01:14:20, mlamouri wrote:
> > On 2017/01/22 at 09:35:34, leon.han wrote:
> > > Uploaded ps#3 to merge ScreenOrientation into ScreenOrientationProvider,
PTAnL
> > firstly while I'm considering how to add some tests, Thanks.
> > 
> > I would recommend to upload this in another CL instead. Uploading a largely
> > different CL after it was reviewed and approved isn't a good idea in general
> > because reviewer might easily forget to look at it. I clicked out of
curiosity
> > to check the tests ;)
> 
> Oh, I see, I'll separate ps#3 to another CL, and could we just land ps#2 here
and add tests in that new CL? Because some codes are supposed to be deleted in
that new CL.

Sure.

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

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] Fix a possible crash point in ScreenOrientationProvider.

ScreenOrientation does not need to maintain pending lock request,
just let ScreenOrientationProvider do that.

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

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

Patchset #3 (id:100001) has been deleted

[leonhsl(Using Gerrit), 2017-01-23 02:29:03]

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

[leonhsl(Using Gerrit), 2017-01-23 02:29:04]

+kinuko@, would you PTAL for OWNER review of content/public/browser/? Thanks.

[kinuko, 2017-01-23 02:38:17]

lgtm

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

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

[commit-bot: I haz the power, 2017-01-23 02:39:35]

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

[kinuko, 2017-01-23 02:39:50]

https://codereview.chromium.org/2630323002/diff/20001/content/public/browser/...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2630323002/diff/20001/content/public/browser/...
content/public/browser/screen_orientation_provider.cc:111: }
nit: no need of block for one-line body

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

https://codereview.chromium.org/2630323002/diff/20001/content/public/browser/...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2630323002/diff/20001/content/public/browser/...
content/public/browser/screen_orientation_provider.cc:111: }
On 2017/01/23 02:39:50, kinuko wrote:
> nit: no need of block for one-line body

Got it, as I've sent this CL to CQ now so I will address this in the follow-up
CL which will also change this file :) Thanks!

[commit-bot: I haz the power, 2017-01-23 04:22:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485139163397460,
"parent_rev": "72b3d43cd95b330a4056e06371399af57b07bacd", "commit_rev":
"9aad73908ef510572cfe4504c36c2ece69438d1e"}

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

Description was changed from

==========
[ScreenOrientation] Fix a possible crash point in ScreenOrientationProvider.

ScreenOrientation does not need to maintain pending lock request,
just let ScreenOrientationProvider do that.

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

to

==========
[ScreenOrientation] Fix a possible crash point in ScreenOrientationProvider.

ScreenOrientation does not need to maintain pending lock request,
just let ScreenOrientationProvider do that.

BUG=678545

Review-Url: https://codereview.chromium.org/2630323002
Cr-Commit-Position: refs/heads/master@{#445321}
Committed:
https://chromium.googlesource.com/chromium/src/+/9aad73908ef510572cfe4504c36c...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/9aad73908ef510572cfe4504c36c...