Remove dependency of power_save_blocker_android on various content bits

Remove dependency of power_save_blocker_android on various content bits

BUG=612337, 612563
TEST=start/stop video on m.youtube.com, content_browsertests

Committed: https://crrev.com/a25943038ebc706537cdd5e6229924c53217ed31
Cr-Commit-Position: refs/heads/master@{#399549}

[scottmg, 2016-05-20 21:20:08]

Description was changed from

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
==========

to

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
==========

[scottmg, 2016-05-24 23:28:59]

scottmg@chromium.org changed reviewers:
+ boliu@chromium.org, michaelbai@chromium.org

[scottmg, 2016-05-26 23:05:21]

Apparently this doesn't work. I'm not too sure what I'm doing here, but I guess
only certain views implement that interface.

[scottmg, 2016-05-26 23:19:30]

Description was changed from

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
==========

to

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
TEST=start/stop video on m.youtube.com
==========

[scottmg, 2016-05-26 23:36:59]

scottmg@chromium.org changed reviewers:
+ nick@chromium.org

[scottmg, 2016-05-26 23:37:59]

+nick for content/

Do you feel able to review this (including the .java)?

(All of content/public/android/OWNERS are out until June-sometime.)

[scottmg, 2016-05-30 21:17:58]

Description was changed from

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
TEST=start/stop video on m.youtube.com
==========

to

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
TEST=start/stop video on m.youtube.com, content_browsertests
==========

[boliu, 2016-05-31 17:23:44]

hmm, code no longer handles the case of PowerSaveBlockerImpl::Delegate outliving
WebContents/ViewAndroid. previously this was done with WebContentsObserver
nulling out the web_contents() pointer.

For this CL, it's kinda hard to reason about the lifetime of
RenderFrameDevToolsAgentHost, since it's refcounted.

https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
File content/browser/power_save_blocker_android.cc (right):

https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
content/browser/power_save_blocker_android.cc:7: #include "base/location.h"
is this needed?

[ncarter (slow), 2016-05-31 17:43:09]

I can give an owners stamp but you'll need a primary reviewer (bo?) who is more
familiar with Android. (I have Java readability, etc, but am pretty shaky on the
JNI+Java stuff)

https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
File content/browser/power_save_blocker_android.cc (right):

https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
content/browser/power_save_blocker_android.cc:7: #include "base/location.h"
On 2016/05/31 17:23:43, boliu wrote:
> is this needed?

yes, for FROM_HERE. It was missing.

[scottmg, 2016-05-31 20:02:38]

On 2016/05/31 17:23:44, boliu wrote:
> hmm, code no longer handles the case of PowerSaveBlockerImpl::Delegate
outliving
> WebContents/ViewAndroid. previously this was done with WebContentsObserver
> nulling out the web_contents() pointer.

Thanks, I missed that subtlety.

Does it seem correct to pass a new interface into InitDisplaySleepBlocker() that
can then be used to call back to the holder of web_contents() to provide the
view if it's still around? That is, we're not worried about the lifetime of the
object that is the WebContentsObserver, just the View that it gave out, right?

> 
> For this CL, it's kinda hard to reason about the lifetime of
> RenderFrameDevToolsAgentHost, since it's refcounted.
> 
>
https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
> File content/browser/power_save_blocker_android.cc (right):
> 
>
https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
> content/browser/power_save_blocker_android.cc:7: #include "base/location.h"
> is this needed?

[scottmg, 2016-05-31 20:48:10]

On 2016/05/31 20:02:38, scottmg wrote:
> On 2016/05/31 17:23:44, boliu wrote:
> > hmm, code no longer handles the case of PowerSaveBlockerImpl::Delegate
> outliving
> > WebContents/ViewAndroid. previously this was done with WebContentsObserver
> > nulling out the web_contents() pointer.
> 
> Thanks, I missed that subtlety.
> 
> Does it seem correct to pass a new interface into InitDisplaySleepBlocker()
that
> can then be used to call back to the holder of web_contents() to provide the
> view if it's still around? That is, we're not worried about the lifetime of
the
> object that is the WebContentsObserver, just the View that it gave out, right?

Hmm, I guess not. It depends now on PowerSaveBlockerImpl::Delegate also being a
WebContentsObserver itself, so that gets nulled when the WebContents goes away,
if I understand.

I guess instead then, I could have three callers of InitDisplaySleepBlocker()
clear the provided view in WebContentsDestroyed. That seems relatively
straightforward. (?)

> 
> > 
> > For this CL, it's kinda hard to reason about the lifetime of
> > RenderFrameDevToolsAgentHost, since it's refcounted.
> > 
> >
>
https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
> > File content/browser/power_save_blocker_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
> > content/browser/power_save_blocker_android.cc:7: #include "base/location.h"
> > is this needed?

[boliu, 2016-05-31 20:56:29]

On 2016/05/31 20:48:10, scottmg wrote:
> On 2016/05/31 20:02:38, scottmg wrote:
> > On 2016/05/31 17:23:44, boliu wrote:
> > > hmm, code no longer handles the case of PowerSaveBlockerImpl::Delegate
> > outliving
> > > WebContents/ViewAndroid. previously this was done with WebContentsObserver
> > > nulling out the web_contents() pointer.
> > 
> > Thanks, I missed that subtlety.
> > 
> > Does it seem correct to pass a new interface into InitDisplaySleepBlocker()
> that
> > can then be used to call back to the holder of web_contents() to provide the
> > view if it's still around? That is, we're not worried about the lifetime of
> the
> > object that is the WebContentsObserver, just the View that it gave out,
right?
> 
> Hmm, I guess not. It depends now on PowerSaveBlockerImpl::Delegate also being
a
> WebContentsObserver itself, so that gets nulled when the WebContents goes
away,
> if I understand.
> 
> I guess instead then, I could have three callers of InitDisplaySleepBlocker()
> clear the provided view in WebContentsDestroyed. That seems relatively
> straightforward. (?)

Yeah that should work.

Or maybe InitDisplaySleepBlocker can take a WeakPtr<ui::ViewAndroid>.

> 
> > 
> > > 
> > > For this CL, it's kinda hard to reason about the lifetime of
> > > RenderFrameDevToolsAgentHost, since it's refcounted.
> > > 
> > >
> >
>
https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
> > > File content/browser/power_save_blocker_android.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
> > > content/browser/power_save_blocker_android.cc:7: #include
"base/location.h"
> > > is this needed?

[scottmg, 2016-05-31 22:15:36]

Patchset #9 (id:150001) has been deleted

[scottmg, 2016-05-31 23:14:17]

Patchset #10 (id:190001) has been deleted

[scottmg, 2016-05-31 23:14:24]

Patchset #9 (id:170001) has been deleted

[scottmg, 2016-06-01 17:30:11]

Patchset #10 (id:230001) has been deleted

[scottmg, 2016-06-01 19:31:17]

Patchset #11 (id:270001) has been deleted

[scottmg, 2016-06-01 20:36:13]

Patchset #11 (id:290001) has been deleted

[scottmg, 2016-06-01 21:11:43]

Patchset #10 (id:250001) has been deleted

[scottmg, 2016-06-01 21:13:10]

On 2016/05/31 20:56:29, boliu wrote:
> On 2016/05/31 20:48:10, scottmg wrote:
> > On 2016/05/31 20:02:38, scottmg wrote:
> > > On 2016/05/31 17:23:44, boliu wrote:
> > > > hmm, code no longer handles the case of PowerSaveBlockerImpl::Delegate
> > > outliving
> > > > WebContents/ViewAndroid. previously this was done with
WebContentsObserver
> > > > nulling out the web_contents() pointer.
> > > 
> > > Thanks, I missed that subtlety.
> > > 
> > > Does it seem correct to pass a new interface into
InitDisplaySleepBlocker()
> > that
> > > can then be used to call back to the holder of web_contents() to provide
the
> > > view if it's still around? That is, we're not worried about the lifetime
of
> > the
> > > object that is the WebContentsObserver, just the View that it gave out,
> right?
> > 
> > Hmm, I guess not. It depends now on PowerSaveBlockerImpl::Delegate also
being
> a
> > WebContentsObserver itself, so that gets nulled when the WebContents goes
> away,
> > if I understand.
> > 
> > I guess instead then, I could have three callers of
InitDisplaySleepBlocker()
> > clear the provided view in WebContentsDestroyed. That seems relatively
> > straightforward. (?)
> 
> Yeah that should work.
> 
> Or maybe InitDisplaySleepBlocker can take a WeakPtr<ui::ViewAndroid>.

I tried WeakPtr (see ps#9) as it seemed a bit nicer, but the WebView tests
didn't work, it seems they want to have the PowerSave disabled on shutdown. So
I've switched to a Clear function called from WebContentsDestroyed().

> 
> > 
> > > 
> > > > 
> > > > For this CL, it's kinda hard to reason about the lifetime of
> > > > RenderFrameDevToolsAgentHost, since it's refcounted.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
> > > > File content/browser/power_save_blocker_android.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2003803002/diff/130001/content/browser/power_...
> > > > content/browser/power_save_blocker_android.cc:7: #include
> "base/location.h"
> > > > is this needed?

[boliu, 2016-06-01 21:37:07]

https://codereview.chromium.org/2003803002/diff/310001/content/browser/power_...
File content/browser/power_save_blocker_android.cc (right):

https://codereview.chromium.org/2003803002/diff/310001/content/browser/power_...
content/browser/power_save_blocker_android.cc:59: if (view_android_ &&
!view_android_->GetViewAndroidDelegate().is_null()) {
the is_null check isn't actually "safe". in theory gc could run between this
line and the body, and collect the object. So to do this safely, need to save
the ScopedJavaLocalRef as a local variable, check is_null, and then call obj()
to use it

on the other hand, it's safe to assume GetViewAndroidDelegate() never returns
null object, so maybe just skip this check altogether?

[scottmg, 2016-06-01 21:44:42]

https://codereview.chromium.org/2003803002/diff/310001/content/browser/power_...
File content/browser/power_save_blocker_android.cc (right):

https://codereview.chromium.org/2003803002/diff/310001/content/browser/power_...
content/browser/power_save_blocker_android.cc:59: if (view_android_ &&
!view_android_->GetViewAndroidDelegate().is_null()) {
On 2016/06/01 21:37:07, boliu wrote:
> the is_null check isn't actually "safe". in theory gc could run between this
> line and the body, and collect the object. So to do this safely, need to save
> the ScopedJavaLocalRef as a local variable, check is_null, and then call obj()
> to use it
> 
> on the other hand, it's safe to assume GetViewAndroidDelegate() never returns
> null object, so maybe just skip this check altogether?

Thanks, removed.

[boliu, 2016-06-01 21:45:21]

non-owner lgtm

[ncarter (slow), 2016-06-01 22:01:11]

lgtm

https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_service_context.cc:44:
->ClearViewForSleepBlocker();
Now that there are 2 downcasts, consider making wake_lock_ the Impl type, so
that you only have to do the static_cast at CreatePowerSaveBlocker time (where
it's also easier to verify that the cast is safe).

[ncarter (slow), 2016-06-01 22:02:56]

https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_service_context.cc:44:
->ClearViewForSleepBlocker();
On 2016/06/01 22:01:11, ncarter wrote:
> Now that there are 2 downcasts, consider making wake_lock_ the Impl type, so
> that you only have to do the static_cast at CreatePowerSaveBlocker time (where
> it's also easier to verify that the cast is safe).

Or don't -- downcasting while assigning a unique_ptr can be a PITA, and it's
probably worse than 2 static_casts.

[ncarter (slow), 2016-06-01 22:02:57]

https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_service_context.cc:44:
->ClearViewForSleepBlocker();
On 2016/06/01 22:01:11, ncarter wrote:
> Now that there are 2 downcasts, consider making wake_lock_ the Impl type, so
> that you only have to do the static_cast at CreatePowerSaveBlocker time (where
> it's also easier to verify that the cast is safe).

Or don't -- downcasting while assigning a unique_ptr can be a PITA, and it's
probably worse than 2 static_casts.

[scottmg, 2016-06-02 02:11:31]

Patchset #12 (id:350001) has been deleted

[scottmg, 2016-06-02 04:18:58]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 04:19:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003803002/390001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003803002/390001

[commit-bot: I haz the power, 2016-06-02 04:38:18]

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

[commit-bot: I haz the power, 2016-06-02 04:38:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scottmg, 2016-06-02 15:32:25]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 15:33:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003803002/390001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003803002/390001

[commit-bot: I haz the power, 2016-06-02 17:14:12]

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

[commit-bot: I haz the power, 2016-06-02 17:14:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[scottmg, 2016-06-02 21:25:00]

Patchset #12 (id:370001) has been deleted

[scottmg, 2016-06-02 21:25:09]

Patchset #12 (id:390001) has been deleted

[scottmg, 2016-06-03 01:33:07]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 01:34:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003803002/450001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003803002/450001

[commit-bot: I haz the power, 2016-06-03 02:53:56]

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

[commit-bot: I haz the power, 2016-06-03 02:53:58]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[scottmg, 2016-06-09 16:03:33]

Patchset #12 (id:410001) has been deleted

[scottmg, 2016-06-09 16:03:44]

Patchset #13 (id:450001) has been deleted

[scottmg, 2016-06-09 16:03:52]

Patchset #12 (id:430001) has been deleted

[scottmg, 2016-06-11 20:21:51]

Patchset #12 (id:470001) has been deleted

[scottmg, 2016-06-11 23:21:29]

Hi, finally got back to this and had to end up switching back to a WeakPtr
because of some subtlety in the shutdown order that didn't work when just
clearing the View during WebContentsDestroyed().

Sorry for the back and forth, but could you please look again?

[boliu, 2016-06-13 14:56:43]

lgtm

let actual owner decide if my comments have merit :p

https://codereview.chromium.org/2003803002/diff/490001/content/browser/devtoo...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2003803002/diff/490001/content/browser/devtoo...
content/browser/devtools/render_frame_devtools_agent_host.cc:763: if
(view_weak_factory_)
view_weak_factory_.reset(); everywhere?

https://codereview.chromium.org/2003803002/diff/490001/content/browser/power_...
File content/browser/power_save_blocker_android.cc (right):

https://codereview.chromium.org/2003803002/diff/490001/content/browser/power_...
content/browser/power_save_blocker_android.cc:92: base::WeakPtr<ui::ViewAndroid>
view_android) {
should parameter be const&?

[ncarter (slow), 2016-06-13 16:56:20]

lgtm

https://codereview.chromium.org/2003803002/diff/490001/content/browser/devtoo...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2003803002/diff/490001/content/browser/devtoo...
content/browser/devtools/render_frame_devtools_agent_host.cc:763: if
(view_weak_factory_)
On 2016/06/13 14:56:43, boliu wrote:
> view_weak_factory_.reset(); everywhere?

I agree with boliu; otherwise you have a dangling NativeView ptr inside the weak
factory.

https://codereview.chromium.org/2003803002/diff/490001/content/browser/power_...
File content/browser/power_save_blocker_android.cc (right):

https://codereview.chromium.org/2003803002/diff/490001/content/browser/power_...
content/browser/power_save_blocker_android.cc:92: base::WeakPtr<ui::ViewAndroid>
view_android) {
On 2016/06/13 14:56:43, boliu wrote:
> should parameter be const&?

I imagine const& is better, since copying a WeakPtr involves a refcount bump.

[scottmg, 2016-06-13 18:52:56]

Thanks!

https://codereview.chromium.org/2003803002/diff/490001/content/browser/devtoo...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2003803002/diff/490001/content/browser/devtoo...
content/browser/devtools/render_frame_devtools_agent_host.cc:763: if
(view_weak_factory_)
On 2016/06/13 14:56:43, boliu wrote:
> view_weak_factory_.reset(); everywhere?

Done.

https://codereview.chromium.org/2003803002/diff/490001/content/browser/power_...
File content/browser/power_save_blocker_android.cc (right):

https://codereview.chromium.org/2003803002/diff/490001/content/browser/power_...
content/browser/power_save_blocker_android.cc:92: base::WeakPtr<ui::ViewAndroid>
view_android) {
On 2016/06/13 14:56:43, boliu wrote:
> should parameter be const&?

Done.

[boliu, 2016-06-13 18:54:45]

https://codereview.chromium.org/2003803002/diff/510001/content/browser/devtoo...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2003803002/diff/510001/content/browser/devtoo...
content/browser/devtools/render_frame_devtools_agent_host.cc:766: }
err, just reset, no need for if or invalidate?

[scottmg, 2016-06-13 19:10:10]

https://codereview.chromium.org/2003803002/diff/510001/content/browser/devtoo...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2003803002/diff/510001/content/browser/devtoo...
content/browser/devtools/render_frame_devtools_agent_host.cc:766: }
On 2016/06/13 18:54:45, boliu wrote:
> err, just reset, no need for if or invalidate?

Oops, done.

[scottmg, 2016-06-13 19:10:17]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-06-13 19:10:17]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2003803002/#ps530001
(title: "only .reset()")

[commit-bot: I haz the power, 2016-06-13 19:10:56]

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

[commit-bot: I haz the power, 2016-06-13 21:37:28]

Description was changed from

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
TEST=start/stop video on m.youtube.com, content_browsertests
==========

to

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
TEST=start/stop video on m.youtube.com, content_browsertests
==========

[commit-bot: I haz the power, 2016-06-13 21:37:30]

Committed patchset #14 (id:530001)

[commit-bot: I haz the power, 2016-06-13 21:37:44]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-13 21:39:02]

Description was changed from

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
TEST=start/stop video on m.youtube.com, content_browsertests
==========

to

==========
Remove dependency of power_save_blocker_android on various content bits

BUG=612337,612563
TEST=start/stop video on m.youtube.com, content_browsertests

Committed: https://crrev.com/a25943038ebc706537cdd5e6229924c53217ed31
Cr-Commit-Position: refs/heads/master@{#399549}
==========

[commit-bot: I haz the power, 2016-06-13 21:39:03]

Patchset 14 (id:??) landed as
https://crrev.com/a25943038ebc706537cdd5e6229924c53217ed31
Cr-Commit-Position: refs/heads/master@{#399549}

[scottmg, 2016-06-20 22:07:45]

On 2016/06/01 22:02:57, ncarter wrote:
>
https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
> File content/browser/wake_lock/wake_lock_service_context.cc (right):
> 
>
https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
> content/browser/wake_lock/wake_lock_service_context.cc:44:
> ->ClearViewForSleepBlocker();
> On 2016/06/01 22:01:11, ncarter wrote:
> > Now that there are 2 downcasts, consider making wake_lock_ the Impl type, so
> > that you only have to do the static_cast at CreatePowerSaveBlocker time
(where
> > it's also easier to verify that the cast is safe).
> 
> Or don't -- downcasting while assigning a unique_ptr can be a PITA, and it's
> probably worse than 2 static_casts.

[ Thread necromancy, but just FYI: after moving this all to //device, the split
of PowerSaveBlocker and PowerSaveBlockerImpl is no longer necessary, so the
downcast is gone. \o/ ]

[ncarter (slow), 2016-06-20 22:18:12]

Hooray.

On Mon, Jun 20, 2016 at 3:07 PM, <scottmg@chromium.org> wrote:

> On 2016/06/01 22:02:57, ncarter wrote:
> >
>
>
https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
> > File content/browser/wake_lock/wake_lock_service_context.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2003803002/diff/330001/content/browser/wake_l...
> > content/browser/wake_lock/wake_lock_service_context.cc:44:
> > ->ClearViewForSleepBlocker();
> > On 2016/06/01 22:01:11, ncarter wrote:
> > > Now that there are 2 downcasts, consider making wake_lock_ the Impl
> type, so
> > > that you only have to do the static_cast at CreatePowerSaveBlocker time
> (where
> > > it's also easier to verify that the cast is safe).
> >
> > Or don't -- downcasting while assigning a unique_ptr can be a PITA, and
> it's
> > probably worse than 2 static_casts.
>
> [ Thread necromancy, but just FYI: after moving this all to //device, the
> split
> of PowerSaveBlocker and PowerSaveBlockerImpl is no longer necessary, so the
> downcast is gone. \o/ ]
>
> https://codereview.chromium.org/2003803002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.