Use callbacks in SessionRestore unit tests

Use callbacks in SessionRestore unit tests

BUG=chromium:268984
TEST=All tests pass in SessionRestoreBrowserTest and
BetterSessionRestoreBrowserTest

Signed-off-by: Simon Que <sque@chromium.org>;

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

[Simon Que, 2015-01-23 20:31:50]

sque@chromium.org changed reviewers:
+ marja@chromium.org, sky@chromium.org

[Simon Que, 2015-01-23 20:33:33]

On 2015/01/23 20:31:51, Simon Que wrote:

When I run all the SessionRestoreTests at once, the last one
(SessionStorageAfterTabReplace) fails without a clear explanation.

However, when I explicitly run only
SessionRestoreTests.SessionStorageAfterTabReplace, it passes.

[Simon Que, 2015-01-23 20:35:34]

On 2015/01/23 20:33:33, Simon Que wrote:
> On 2015/01/23 20:31:51, Simon Que wrote:
> 
> When I run all the SessionRestoreTests at once, the last one
> (SessionStorageAfterTabReplace) fails without a clear explanation.
> 
> However, when I explicitly run only
> SessionRestoreTests.SessionStorageAfterTabReplace, it passes.

Hmm now it's passing when I run all tests at once.... must be a flake or
something.

[sky, 2015-01-23 23:36:24]

On 2015/01/23 20:35:34, Simon Que wrote:
> On 2015/01/23 20:33:33, Simon Que wrote:
> > On 2015/01/23 20:31:51, Simon Que wrote:
> > 
> > When I run all the SessionRestoreTests at once, the last one
> > (SessionStorageAfterTabReplace) fails without a clear explanation.
> > 
> > However, when I explicitly run only
> > SessionRestoreTests.SessionStorageAfterTabReplace, it passes.
> 
> Hmm now it's passing when I run all tests at once.... must be a flake or
> something.

That makes me worried. Are there pre tests that aren't running if you run one by
itself?

[Simon Que, 2015-01-24 01:22:08]

On 2015/01/23 23:36:24, sky wrote:
> On 2015/01/23 20:35:34, Simon Que wrote:
> > On 2015/01/23 20:33:33, Simon Que wrote:
> > > On 2015/01/23 20:31:51, Simon Que wrote:
> > > 
> > > When I run all the SessionRestoreTests at once, the last one
> > > (SessionStorageAfterTabReplace) fails without a clear explanation.
> > > 
> > > However, when I explicitly run only
> > > SessionRestoreTests.SessionStorageAfterTabReplace, it passes.
> > 
> > Hmm now it's passing when I run all tests at once.... must be a flake or
> > something.
> 
> That makes me worried. Are there pre tests that aren't running if you run one
by
> itself?

It was probably a flake. It's working fine now. I ran all the
SessionRestoreTests at once, multiple times.

[Simon Que, 2015-01-25 03:37:42]

On 2015/01/24 01:22:08, Simon Que wrote:
> On 2015/01/23 23:36:24, sky wrote:
> > On 2015/01/23 20:35:34, Simon Que wrote:
> > > On 2015/01/23 20:33:33, Simon Que wrote:
> > > > On 2015/01/23 20:31:51, Simon Que wrote:
> > > > 
> > > > When I run all the SessionRestoreTests at once, the last one
> > > > (SessionStorageAfterTabReplace) fails without a clear explanation.
> > > > 
> > > > However, when I explicitly run only
> > > > SessionRestoreTests.SessionStorageAfterTabReplace, it passes.
> > > 
> > > Hmm now it's passing when I run all tests at once.... must be a flake or
> > > something.
> > 
> > That makes me worried. Are there pre tests that aren't running if you run
one
> by
> > itself?
> 
> It was probably a flake. It's working fine now. I ran all the
> SessionRestoreTests at once, multiple times.

sky: I sent out an email proposing an alternative approach. I can do that in
this CL.

[sky, 2015-01-26 16:50:01]

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
File chrome/browser/sessions/session_restore_test_helper.cc (right):

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
2015 and no copyright.

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.cc:15:
base::Unretained(this)));
As it's entirely possible for this to be destroyed before you get the callback
you shouldn't use unretained here.

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
File chrome/browser/sessions/session_restore_test_helper.h (right):

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
2015 and no copyright.

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.h:21:
SessionRestoreTestHelper();
Add destructor.

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.h:40: void
OnSessionRestoreDone();
functions before members.

[Simon Que, 2015-01-26 18:40:50]

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
File chrome/browser/sessions/session_restore_test_helper.cc (right):

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2015/01/26 16:50:01, sky wrote:
> 2015 and no copyright.

Done.

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.cc:15:
base::Unretained(this)));
On 2015/01/26 16:50:01, sky wrote:
> As it's entirely possible for this to be destroyed before you get the callback
> you shouldn't use unretained here.

Done.

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
File chrome/browser/sessions/session_restore_test_helper.h (right):

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2015/01/26 16:50:01, sky wrote:
> 2015 and no copyright.

Done.

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.h:21:
SessionRestoreTestHelper();
On 2015/01/26 16:50:01, sky wrote:
> Add destructor.

Why is it necessary?

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.h:40: void
OnSessionRestoreDone();
On 2015/01/26 16:50:01, sky wrote:
> functions before members.

Done.

[sky, 2015-01-26 20:07:09]

LGTM

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
File chrome/browser/sessions/session_restore_test_helper.h (right):

https://codereview.chromium.org/871013006/diff/40001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore_test_helper.h:21:
SessionRestoreTestHelper();
On 2015/01/26 18:40:49, Simon Que wrote:
> On 2015/01/26 16:50:01, sky wrote:
> > Add destructor.
> 
> Why is it necessary?

So that it doesn't get inlined in all call sites. It's in the chrome style
guide.

[Simon Que, 2015-01-26 21:16:58]

> So that it doesn't get inlined in all call sites. It's in the chrome style
> guide.


Done

[Simon Que, 2015-01-26 21:17:09]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2015-01-26 21:18:35]

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

[commit-bot: I haz the power, 2015-01-26 22:14:17]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-26 22:15:58]

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