sync: Refactor StatusChangeChecker hierarchy

sync: Refactor StatusChangeChecker hierarchy

Refactors the StatusChangeChecker class hierarchy to allow for more code
sharing.  Allows children of StatusChangeChecker to share logic around
performing a blocking wait, checking exit conditions, and timing out.

As before, the sharing is acheived through inheritance.  We should move
to a delegation pattern if this becomes too complicated, but I think the
use of inheritance still makes sense for now.

This is a pre-requisite for a future patch that adds a new kind of
StatusChangeChecker that does not listen to ProfileSyncService changes.
I expect that there will be many more kinds of StatusChangeCheckers
defined in the future.  This refactoring will help us share code between
them.

This is attached to issue 97780 and 95742 because it will enable us to
write more StatusChangeCheckers that we can use to refactor tests that
currently listen for snapshots and self-notifications.

BUG=97780, 95742
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272711

[rlarocque, 2014-05-22 20:20:19]

This was originally part of a CL to refactor the autofill integration tests. 
This chunk of it can stand on its own, so I decided to split it out so it can be
reviewed separately.

Adding pvalenzuela as the main reviewer, with zea as a backup in case Patrick's
approval isn't sufficient to get past the CQ.

Please review.

[pval...(no longer on Chromium), 2014-05-22 21:07:12]

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/status_change_checker.cc (right):

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/status_change_checker.cc:26:
DCHECK(!wait_started_) << "This class is intended for one use only.";
document this one-time use restriction in the class comment

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/status_change_checker.h (right):

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/status_change_checker.h:38: // Timeout
length when blocking.  Default is 45s.
"Default is 45s." doesn't seem very future proof. Maybe just remove and let
readers view the default in the cc file?

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/status_change_checker.h:50: void Unblock();
is StopBlockingWait() or StopWaiting() a better name for this?

[rlarocque, 2014-05-22 21:29:30]

Patch updated.  PTAL.

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/status_change_checker.cc (right):

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/status_change_checker.cc:26:
DCHECK(!wait_started_) << "This class is intended for one use only.";
On 2014/05/22 21:07:12, pvalenzuela wrote:
> document this one-time use restriction in the class comment

The old class comment was stale.  Fixed it and added a note about this.

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/status_change_checker.h (right):

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/status_change_checker.h:38: // Timeout
length when blocking.  Default is 45s.
On 2014/05/22 21:07:12, pvalenzuela wrote:
> "Default is 45s." doesn't seem very future proof. Maybe just remove and let
> readers view the default in the cc file?

Done.

https://codereview.chromium.org/299843007/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/status_change_checker.h:50: void Unblock();
On 2014/05/22 21:07:12, pvalenzuela wrote:
> is StopBlockingWait() or StopWaiting() a better name for this?

Done.  Changed name to StopWaiting().

[pval...(no longer on Chromium), 2014-05-22 21:33:37]

lgtm

[rlarocque, 2014-05-22 21:34:12]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-22 21:35:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/299843007/60001

[commit-bot: I haz the power, 2014-05-22 21:35:36]

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

[commit-bot: I haz the power, 2014-05-22 21:35:36]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[rlarocque, 2014-05-22 22:09:27]

On 2014/05/22 21:35:36, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

I figured that might happen.

Nicolas, PTAL.

[Nicolas Zea, 2014-05-23 20:22:43]

On 2014/05/22 22:09:27, rlarocque wrote:
> On 2014/05/22 21:35:36, I haz the power (commit-bot) wrote:
> > No LGTM from a valid reviewer yet. Only full committers are accepted.
> > Even if an LGTM may have been provided, it was from a non-committer or
> > a provisional committer, _not_ a full super star committer.
> > See http://www.chromium.org/getting-involved/become-a-committer
> > Note that this has nothing to do with OWNERS files.
> 
> I figured that might happen.
> 
> Nicolas, PTAL.

"if it LGTM to pvalenzuela, it LGTMe"

[Nicolas Zea, 2014-05-23 20:52:17]

On 2014/05/23 20:22:43, Nicolas Zea wrote:
> On 2014/05/22 22:09:27, rlarocque wrote:
> > On 2014/05/22 21:35:36, I haz the power (commit-bot) wrote:
> > > No LGTM from a valid reviewer yet. Only full committers are accepted.
> > > Even if an LGTM may have been provided, it was from a non-committer or
> > > a provisional committer, _not_ a full super star committer.
> > > See http://www.chromium.org/getting-involved/become-a-committer
> > > Note that this has nothing to do with OWNERS files.
> > 
> > I figured that might happen.
> > 
> > Nicolas, PTAL.
> 
> "if it LGTM to pvalenzuela, it LGTMe"

I just realized I got that phrase wrong. Should be "if it LGTpvalenzuela, it
LGTMe".

Clearly I'm not as talented a wordsmith as Tim :-(

[rlarocque, 2014-05-23 22:43:10]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-23 22:47:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/299843007/60001

[commit-bot: I haz the power, 2014-05-24 06:09:40]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-24 12:19:10]

Change committed as 272711