Created notification for when a session is saved....

Created notification for when a session is saved.

Created a notification for when a session is saved,
so that later we can observe this notification and 
sync a session.
BUG=30519
TEST=session_service_unittest.cc

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

[jerrica, 2010-06-15 20:39:26]

Hello Tim,

Please review this for me :-)

[tim (not reviewing), 2010-06-15 23:22:25]

Looking pretty good! No major issues, just some nits.

http://codereview.chromium.org/2858005/diff/1/2
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/2858005/diff/1/2#newcode436
chrome/browser/sessions/session_service.cc:436:
NotificationService::current()->Notify(
might as well wrap this in the if (had_commands) block as well.

http://codereview.chromium.org/2858005/diff/1/2#newcode437
chrome/browser/sessions/session_service.cc:437:
NotificationType::SESSION_SERVICE_SAVED,
these need one more space of indent, or move them back to 4 spaces from previous
line start.

http://codereview.chromium.org/2858005/diff/1/3
File chrome/browser/sessions/session_service_unittest.cc (right):

http://codereview.chromium.org/2858005/diff/1/3#newcode50
chrome/browser/sessions/session_service_unittest.cc:50: void
SessionServiceTest::Observe(NotificationType type,
shouldn't have the 'SessionServiceTest' prefix here

http://codereview.chromium.org/2858005/diff/1/3#newcode127
chrome/browser/sessions/session_service_unittest.cc:127: int sync_save_count;
probably a good idea to init in constructor. safety first :)

hmm.. this should have a trailing underscore too.  I don't know why the two
params above it dont :\

http://codereview.chromium.org/2858005/diff/1/3#newcode635
chrome/browser/sessions/session_service_unittest.cc:635: if (type ==
NotificationType::SESSION_SERVICE_SAVED)
does the test expect other types?  could we do ASSERT_EQ(type.value(),
NotificationType::SESSION_SERVICE_SAVED) instead of the if?

http://codereview.chromium.org/2858005/diff/1/3#newcode646
chrome/browser/sessions/session_service_unittest.cc:646: AddTab(browser(),
GURL("http://foo/1"));
if we're just calling Save() directly now, is there any need for all this code?

Also I'm still a little confused why Save doesn't get called automagically when
some of these actions are performed... don't other tests in this file rely on
Save getting called?

http://codereview.chromium.org/2858005/diff/1/3#newcode654
chrome/browser/sessions/session_service_unittest.cc:654:
&GetCurrentSessionCallbackHandler::OnGotSession));
nit - indent is off.  in thsi case, you probably want to indent all 3 params 4
spaces from the previous line start (the word service())

http://codereview.chromium.org/2858005/diff/1/4
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/2858005/diff/1/4#newcode984
chrome/common/notification_type.h:984: // The session service has been saved.
maybe add '... Only sent if there were new SessionService commands to save, and
not for no-op save operations.'

[jerrica, 2010-06-17 00:14:25]

i made the changes :-)

http://codereview.chromium.org/2858005/diff/1/2
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/2858005/diff/1/2#newcode436
chrome/browser/sessions/session_service.cc:436:
NotificationService::current()->Notify(
On 2010/06/15 23:22:25, timsteele wrote:
> might as well wrap this in the if (had_commands) block as well.

Done.

http://codereview.chromium.org/2858005/diff/1/2#newcode437
chrome/browser/sessions/session_service.cc:437:
NotificationType::SESSION_SERVICE_SAVED,
On 2010/06/15 23:22:25, timsteele wrote:
> these need one more space of indent, or move them back to 4 spaces from
previous
> line start.

Done.

http://codereview.chromium.org/2858005/diff/1/3
File chrome/browser/sessions/session_service_unittest.cc (right):

http://codereview.chromium.org/2858005/diff/1/3#newcode50
chrome/browser/sessions/session_service_unittest.cc:50: void
SessionServiceTest::Observe(NotificationType type,
On 2010/06/15 23:22:25, timsteele wrote:
> shouldn't have the 'SessionServiceTest' prefix here

Done.

http://codereview.chromium.org/2858005/diff/1/3#newcode646
chrome/browser/sessions/session_service_unittest.cc:646: AddTab(browser(),
GURL("http://foo/1"));
one does, but as it doesn't call it directly, it takes a while.  I'll remove
that stuff though :)

On 2010/06/15 23:22:25, timsteele wrote:
> if we're just calling Save() directly now, is there any need for all this
code?
> 
> Also I'm still a little confused why Save doesn't get called automagically
when
> some of these actions are performed... don't other tests in this file rely on
> Save getting called?

http://codereview.chromium.org/2858005/diff/1/3#newcode646
chrome/browser/sessions/session_service_unittest.cc:646: AddTab(browser(),
GURL("http://foo/1"));
On 2010/06/15 23:22:25, timsteele wrote:
> if we're just calling Save() directly now, is there any need for all this
code?
> 
> Also I'm still a little confused why Save doesn't get called automagically
when
> some of these actions are performed... don't other tests in this file rely on
> Save getting called?

Done.

http://codereview.chromium.org/2858005/diff/1/3#newcode654
chrome/browser/sessions/session_service_unittest.cc:654:
&GetCurrentSessionCallbackHandler::OnGotSession));
On 2010/06/15 23:22:25, timsteele wrote:
> nit - indent is off.  in thsi case, you probably want to indent all 3 params 4
> spaces from the previous line start (the word service())

Done.

http://codereview.chromium.org/2858005/diff/1/4
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/2858005/diff/1/4#newcode984
chrome/common/notification_type.h:984: // The session service has been saved.
On 2010/06/15 23:22:25, timsteele wrote:
> maybe add '... Only sent if there were new SessionService commands to save,
and
> not for no-op save operations.'

Done.

[tim (not reviewing), 2010-06-17 01:31:56]

LGTM, with two small things you should fix up before checking in.

http://codereview.chromium.org/2858005/diff/7001/8002
File chrome/browser/sessions/session_service_unittest.cc (right):

http://codereview.chromium.org/2858005/diff/7001/8002#newcode49
chrome/browser/sessions/session_service_unittest.cc:49: sync_save_count_ = 0;
this should really go in the initializer list on line 30

e.g. 
SessionServiceTest() : window_bounds(0, 1, 2, 3), sync_save_count_(0) {}

http://codereview.chromium.org/2858005/diff/7001/8002#newcode643
chrome/browser/sessions/session_service_unittest.cc:643:
ASSERT_EQ(sync_save_count_, 1);
use EXPECT_EQ here.