chromeos: Block system suspend while uploading files to Drive

Make content::PowerSaveBlocker public

BUG=152207
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176806

[hashimoto, 2013-01-07 10:13:24]

satorux,
Is it OK to add this line for chrome/browser/google_apis/DEPS?

[Randy Smith (Not in Mondays), 2013-01-07 16:34:22]

CCing PSB folks (Avi, Mike M) in case they want to comment.

[satorux1, 2013-01-08 03:34:45]

I think it'd be cleaner to split this patch into 1) just move the
PowerSaveBlocker to content/public and 2) make changes 	
chrome/browser/google_apis.

I think you can reuse this patch for 1).


As for 2), would it be possible to write a test?

[Avi (use Gerrit), 2013-01-08 03:38:04]

jam@ likes double-checking new stuff going into public, but it's probably fine.

[hashimoto, 2013-01-08 04:02:08]

On 2013/01/08 03:34:45, satorux1 wrote:
> I think it'd be cleaner to split this patch into 1) just move the
> PowerSaveBlocker to content/public and 2) make changes 	
> chrome/browser/google_apis.
> 
> I think you can reuse this patch for 1).
Reverted changes for chrome/browser.
Now this patch simply moves power_save_blocker.h to content/public.
> 
> 
> As for 2), would it be possible to write a test?
I think writing a test would be somehow difficult because PowerSaveBlocker is
implemented differently in each platform, and PowerSaveBlocker itself lacks
tests.

One possible way of testing would be adding a static method to PowerSaveBlocker
which returns the number of PowerSaveBlocker instances currently active.
With this method, we can test it that there is at least one active
PowerSaveBlocker instance during file upload.

[hashimoto, 2013-01-09 08:59:39]

jam, avi
Could you review this change as owners of content?
Now this change only moves the header power_save_blocker.h to
content/public/browser.

[jam, 2013-01-09 16:54:33]

On 2013/01/09 08:59:39, hashimoto wrote:
> jam, avi
> Could you review this change as owners of content?
> Now this change only moves the header power_save_blocker.h to
> content/public/browser.

content/public has pure interfaces or structs. i.e. it's not just moving a
header of a class out to public. see the other files there for examples, and
http://www.chromium.org/developers/content-module/content-api

since PowerSaveBlocker is a utility class, and is now used by two modules, it
seems that this should move to base. That's what we did for other similar
classes once they were used by more than one module.

[hashimoto, 2013-01-10 08:40:30]

On 2013/01/09 16:54:33, John Abd-El-Malek wrote:
> On 2013/01/09 08:59:39, hashimoto wrote:
> > jam, avi
> > Could you review this change as owners of content?
> > Now this change only moves the header power_save_blocker.h to
> > content/public/browser.
> 
> content/public has pure interfaces or structs. i.e. it's not just moving a
> header of a class out to public. see the other files there for examples, and
> http://www.chromium.org/developers/content-module/content-api
Done. PowerSaveBlocker is now a pure interface.
> 
> since PowerSaveBlocker is a utility class, and is now used by two modules, it
> seems that this should move to base. That's what we did for other similar
> classes once they were used by more than one module.
Moving PowerSaveBlocker to base would be difficult because its Linux and Chrome
OS implementations depend on src/dbus and src/chromeos, and both src/dbus and
src/chromeos depend on src/base.
I'm happy to move PowerSaveBlocker to somewhere else if there is any directory
which can depend on both dbus and chromeos, but I could not come up with
appropriate one except src/content.

[jam, 2013-01-11 01:24:30]

https://codereview.chromium.org/11784016/diff/28020/content/browser/power_sav...
File content/browser/power_save_blocker_win.cc (right):

https://codereview.chromium.org/11784016/diff/28020/content/browser/power_sav...
content/browser/power_save_blocker_win.cc:116: // A second object with different
lifetime than the RAII container.
why do this ref counted stuff in every implementation instead of making the base
interface be refcounted?

https://codereview.chromium.org/11784016/diff/28020/content/public/browser/po...
File content/public/browser/power_save_blocker.h (right):

https://codereview.chromium.org/11784016/diff/28020/content/public/browser/po...
content/public/browser/power_save_blocker.h:42: 
nit: everything below this line isn't necessary for interfaces, so remove (see
other interfaces in content/public for example)

[hashimoto, 2013-01-11 08:39:44]

https://codereview.chromium.org/11784016/diff/28020/content/browser/power_sav...
File content/browser/power_save_blocker_win.cc (right):

https://codereview.chromium.org/11784016/diff/28020/content/browser/power_sav...
content/browser/power_save_blocker_win.cc:116: // A second object with different
lifetime than the RAII container.
On 2013/01/11 01:24:30, John Abd-El-Malek wrote:
> why do this ref counted stuff in every implementation instead of making the
base
> interface be refcounted?

Introduced PowerSaveBlockerImpl to avoid code duplication between platforms.

https://codereview.chromium.org/11784016/diff/28020/content/public/browser/po...
File content/public/browser/power_save_blocker.h (right):

https://codereview.chromium.org/11784016/diff/28020/content/public/browser/po...
content/public/browser/power_save_blocker.h:42: 
On 2013/01/11 01:24:30, John Abd-El-Malek wrote:
> nit: everything below this line isn't necessary for interfaces, so remove (see
> other interfaces in content/public for example)

Done.

[hashimoto, 2013-01-11 10:48:57]

https://codereview.chromium.org/11784016/diff/28020/content/public/browser/po...
File content/public/browser/power_save_blocker.h (right):

https://codereview.chromium.org/11784016/diff/28020/content/public/browser/po...
content/public/browser/power_save_blocker.h:42: 
On 2013/01/11 08:39:44, hashimoto wrote:
> On 2013/01/11 01:24:30, John Abd-El-Malek wrote:
> > nit: everything below this line isn't necessary for interfaces, so remove
(see
> > other interfaces in content/public for example)
> 
> Done.

Btw, to prohibit instantiating the interface class PowerSaveBlocker and force
using the Create method, the dtor must be marked as pure virtual (=0), or the
ctor must be protected.
So I marked the dtor as pure virtual.
The sad thing is that we cannot provide a method body inline when marking it as
pure virtual.
This is the reason why I placed the dtor's method body at
power_save_blocker_impl.cc

[jam, 2013-01-11 17:28:38]

https://codereview.chromium.org/11784016/diff/28020/content/browser/power_sav...
File content/browser/power_save_blocker_win.cc (right):

https://codereview.chromium.org/11784016/diff/28020/content/browser/power_sav...
content/browser/power_save_blocker_win.cc:116: // A second object with different
lifetime than the RAII container.
On 2013/01/11 08:39:44, hashimoto wrote:
> On 2013/01/11 01:24:30, John Abd-El-Malek wrote:
> > why do this ref counted stuff in every implementation instead of making the
> base
> > interface be refcounted?
> 
> Introduced PowerSaveBlockerImpl to avoid code duplication between platforms.

this is different than what i was wondering about. if the interface is
refcounted, you wouldn't need the Impl class or the Delegate inner class?

[Mike Mammarella, 2013-01-13 00:30:10]

On 2013/01/11 17:28:38, John Abd-El-Malek wrote:
> this is different than what i was wondering about. if the interface is
> refcounted, you wouldn't need the Impl class or the Delegate inner class?

The delegate is supposed to be an implementation detail; the power save blocker
is supposed to have a single owner, and not be refcounted. We don't want to
muddy that interface and make it not clear who owns it and thus who's
responsible for deleting it to clear the power save block.

The original reason that each implementation did its own version of the
refcounted delegate is that it wasn't clear initially that all platforms would
need to do that. In retrospect it turned out that all platforms needed to do the
work on other threads than the ones instantiating the power save blocker object,
and so all platforms ended up with refcounted delegates.

I'm not opposed to fixing that now, but we should keep the delegate separate so
there is a clear owner of the main object.

[jam, 2013-01-14 06:55:06]

ok, lgtm

[commit-bot: I haz the power, 2013-01-15 01:45:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/11784016/50001

[commit-bot: I haz the power, 2013-01-15 06:25:18]

Change committed as 176806