Introducing ResumeLibrary to track system resume signal

Introducing ResumeLibrary to track system resume signal
Currently used for updating clock view when system resumed.

BUG=chromium-os:11121
TEST=manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74951

[glotov, 2011-02-10 20:22:18]

Hi guys! Please have a look.

[Charlie Lee, 2011-02-10 20:30:45]

LGTM as long as the tests pass.

[Charlie Lee, 2011-02-10 20:32:01]

Looks like you need to rev the cros DEPS before this.

On 2011/02/10 20:30:45, Charlie Lee wrote:
> LGTM as long as the tests pass.

[glotov, 2011-02-10 20:35:16]

I have not committed yet my CLs on Chromeos side. This must be the reason.
Should not have started Trybots.

[Nikita (slow), 2011-02-11 11:54:51]

LGTM once trybots are fine.

May be it makes sense to extend PowerLibrary instead?

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
File chrome/browser/chromeos/cros/resume_library.cc (right):

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
chrome/browser/chromeos/cros/resume_library.cc:58: FOR_EACH_OBSERVER(Observer,
nit: could fit into 1 line?

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
chrome/browser/chromeos/cros/resume_library.cc:88: // Needed for
NewRunnableMethod() call above.
Should mention that this class is Singleton.

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
File chrome/browser/chromeos/cros/resume_library.h (right):

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
chrome/browser/chromeos/cros/resume_library.h:11: class ResumeLibrary {
Please add comment.

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
chrome/browser/chromeos/cros/resume_library.h:15: virtual void SystemResumed() =
0;
May be it makes sense to make this callback a part of PowerLibrary?

[Charlie Lee, 2011-02-11 18:08:27]

I second that idea too. PowerLibrary seems like an ideal place for the
resume logic.

- Charlie


On Fri, Feb 11, 2011 at 3:54 AM, <nkostylev@chromium.org> wrote:

> LGTM once trybots are fine.
>
> May be it makes sense to extend PowerLibrary instead?
>
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> File chrome/browser/chromeos/cros/resume_library.cc (right):
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> chrome/browser/chromeos/cros/resume_library.cc:58:
> FOR_EACH_OBSERVER(Observer,
> nit: could fit into 1 line?
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> chrome/browser/chromeos/cros/resume_library.cc:88: // Needed for
> NewRunnableMethod() call above.
> Should mention that this class is Singleton.
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> File chrome/browser/chromeos/cros/resume_library.h (right):
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> chrome/browser/chromeos/cros/resume_library.h:11: class ResumeLibrary {
> Please add comment.
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> chrome/browser/chromeos/cros/resume_library.h:15: virtual void
> SystemResumed() = 0;
> May be it makes sense to make this callback a part of PowerLibrary?
>
>
> http://codereview.chromium.org/6486007/
>

[glotov, 2011-02-11 18:20:56]

Please see the last reply about power_library idea. Pleas tell me what you think
then.

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
File chrome/browser/chromeos/cros/resume_library.cc (right):

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
chrome/browser/chromeos/cros/resume_library.cc:58: FOR_EACH_OBSERVER(Observer,
On 2011/02/11 11:54:51, Nikita Kostylev wrote:
> nit: could fit into 1 line?

Done.

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
chrome/browser/chromeos/cros/resume_library.cc:88: // Needed for
NewRunnableMethod() call above.
On 2011/02/11 11:54:51, Nikita Kostylev wrote:
> Should mention that this class is Singleton.

Done.

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
File chrome/browser/chromeos/cros/resume_library.h (right):

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
chrome/browser/chromeos/cros/resume_library.h:11: class ResumeLibrary {
On 2011/02/11 11:54:51, Nikita Kostylev wrote:
> Please add comment.

Done.

http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
chrome/browser/chromeos/cros/resume_library.h:15: virtual void SystemResumed() =
0;
I thought about it and cons were that callbacks have different semantics and
there are no clients that use both power and suspend notifications. So I decided
to make small standalone component.

On 2011/02/11 11:54:51, Nikita Kostylev wrote:
> May be it makes sense to make this callback a part of PowerLibrary?

[Charlie Lee, 2011-02-11 18:34:54]

Another option is to put it in the ScreenLockLibrary. I think we should try
to not create another library unless it's really that different.

- Charlie


On Fri, Feb 11, 2011 at 10:20 AM, <glotov@chromium.org> wrote:

> Please see the last reply about power_library idea. Pleas tell me what you
> think
> then.
>
>
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> File chrome/browser/chromeos/cros/resume_library.cc (right):
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> chrome/browser/chromeos/cros/resume_library.cc:58:
> FOR_EACH_OBSERVER(Observer,
> On 2011/02/11 11:54:51, Nikita Kostylev wrote:
>
>> nit: could fit into 1 line?
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> chrome/browser/chromeos/cros/resume_library.cc:88: // Needed for
> NewRunnableMethod() call above.
> On 2011/02/11 11:54:51, Nikita Kostylev wrote:
>
>> Should mention that this class is Singleton.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> File chrome/browser/chromeos/cros/resume_library.h (right):
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> chrome/browser/chromeos/cros/resume_library.h:11: class ResumeLibrary {
> On 2011/02/11 11:54:51, Nikita Kostylev wrote:
>
>> Please add comment.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6486007/diff/1/chrome/browser/chromeos/cros/re...
> chrome/browser/chromeos/cros/resume_library.h:15: virtual void
> SystemResumed() = 0;
> I thought about it and cons were that callbacks have different semantics
> and there are no clients that use both power and suspend notifications.
> So I decided to make small standalone component.
>
>
> On 2011/02/11 11:54:51, Nikita Kostylev wrote:
>
>> May be it makes sense to make this callback a part of PowerLibrary?
>>
>
> http://codereview.chromium.org/6486007/
>

[Nikita (slow), 2011-02-11 18:51:07]

On Fri, Feb 11, 2011 at 9:34 PM, Charlie Lee <chocobo@chromium.org> wrote:

> Another option is to put it in the ScreenLockLibrary. I think we should try
> to not create another library unless it's really that different.
>

Agree. I'm fine with both reusing ScreenLock / PowerLibrary since resume
state is close to them by context.

-- 
Nikita

[glotov, 2011-02-15 14:13:29]

All done, please have a look.

[Nikita (slow), 2011-02-15 15:23:19]

LGTM
unit_tests hang seems like a flake

http://codereview.chromium.org/6486007/diff/13001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/6486007/diff/13001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:27:
chromeos::DisconnectPowerStatus(power_status_connection_);
set power_status_connection_ to NULL?

[glotov, 2011-02-15 15:35:49]

unit_tests work fine locally.

http://codereview.chromium.org/6486007/diff/13001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/6486007/diff/13001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:27:
chromeos::DisconnectPowerStatus(power_status_connection_);
On 2011/02/15 15:23:20, Nikita Kostylev wrote:
> set power_status_connection_ to NULL?

Done.

[Nikita (slow), 2011-02-15 16:40:17]

browser_tests have multiple test failures and ui_tests are still running for
linux_chromeos