Decomped registration logic into its own class.

Decomped registration logic into its own class.

Handle registration events properly.

BUG=34647
TEST=new unittests

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

[akalin, 2010-06-29 07:15:43]

+nick and +ghc for review

[Paweł Hajdan Jr., 2010-06-29 12:52:15]

Drive-by with minor test comments.

http://codereview.chromium.org/2861034/diff/28001/29007
File chrome/browser/sync/notifier/registration_manager_unittest.cc (right):

http://codereview.chromium.org/2861034/diff/28001/29007#newcode7
chrome/browser/sync/notifier/registration_manager_unittest.cc:7: #include
<deque>
nit: Please keep alphabetical ordering.

http://codereview.chromium.org/2861034/diff/28001/29007#newcode52
chrome/browser/sync/notifier/registration_manager_unittest.cc:52: NOTREACHED();
How about FAIL() instead, to avoid crashing entire unit_tests binary?

http://codereview.chromium.org/2861034/diff/28001/29007#newcode57
chrome/browser/sync/notifier/registration_manager_unittest.cc:57: NOTREACHED();
Same here.

http://codereview.chromium.org/2861034/diff/28001/29007#newcode62
chrome/browser/sync/notifier/registration_manager_unittest.cc:62: NOTREACHED();
And here.

[ncarter (slow), 2010-06-29 15:09:41]

http://codereview.chromium.org/2861034/diff/28001/29006
File chrome/browser/sync/notifier/registration_manager.h (right):

http://codereview.chromium.org/2861034/diff/28001/29006#newcode57
chrome/browser/sync/notifier/registration_manager.h:57: REGISTERED,
What do these states mean?  What's the difference between being in the
UNREGISTERED state, and not being in the map?

http://codereview.chromium.org/2861034/diff/28001/29009
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2861034/diff/28001/29009#newcode1842
chrome/chrome_tests.gypi:1842: 'target_name': 'sync_notifier_unit_tests',
Why not just add registration_manager_unittest to sync_unit_tests?

http://codereview.chromium.org/2861034/diff/28001/29009#newcode1845
chrome/chrome_tests.gypi:1845: # TODO(akalin): Write our own test suite and
runner.
Why do we need a custom suite and runner?

[akalin, 2010-06-29 18:12:32]

Note that this patch depends on http://codereview.chromium.org/2861034/show .

Addressed all comments, please take another look.

http://codereview.chromium.org/2861034/diff/28001/29006
File chrome/browser/sync/notifier/registration_manager.h (right):

http://codereview.chromium.org/2861034/diff/28001/29006#newcode57
chrome/browser/sync/notifier/registration_manager.h:57: REGISTERED,
On 2010/06/29 15:09:41, ncarter wrote:
> What do these states mean?  What's the difference between being in the
> UNREGISTERED state, and not being in the map?

Added comments.  If it's not in the map, we don't want it registered.  If it's
in the map with UNREGISTERED, we want it registered, but we haven't gotten
around to starting the process yet.

http://codereview.chromium.org/2861034/diff/28001/29007
File chrome/browser/sync/notifier/registration_manager_unittest.cc (right):

http://codereview.chromium.org/2861034/diff/28001/29007#newcode7
chrome/browser/sync/notifier/registration_manager_unittest.cc:7: #include
<deque>
On 2010/06/29 12:52:15, Paweł Hajdan Jr. wrote:
> nit: Please keep alphabetical ordering.

Done.

http://codereview.chromium.org/2861034/diff/28001/29007#newcode52
chrome/browser/sync/notifier/registration_manager_unittest.cc:52: NOTREACHED();
On 2010/06/29 12:52:15, Paweł Hajdan Jr. wrote:
> How about FAIL() instead, to avoid crashing entire unit_tests binary?

Done.

http://codereview.chromium.org/2861034/diff/28001/29007#newcode57
chrome/browser/sync/notifier/registration_manager_unittest.cc:57: NOTREACHED();
On 2010/06/29 12:52:15, Paweł Hajdan Jr. wrote:
> Same here.

Done.

http://codereview.chromium.org/2861034/diff/28001/29007#newcode62
chrome/browser/sync/notifier/registration_manager_unittest.cc:62: NOTREACHED();
On 2010/06/29 12:52:15, Paweł Hajdan Jr. wrote:
> And here.

Done.

http://codereview.chromium.org/2861034/diff/28001/29009
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2861034/diff/28001/29009#newcode1842
chrome/chrome_tests.gypi:1842: 'target_name': 'sync_notifier_unit_tests',
On 2010/06/29 15:09:41, ncarter wrote:
> Why not just add registration_manager_unittest to sync_unit_tests?

sync_unit_tests depends on the world, and sync_notifier_unit_tests needs to
depend on much less.

http://codereview.chromium.org/2861034/diff/28001/29009#newcode1845
chrome/chrome_tests.gypi:1845: # TODO(akalin): Write our own test suite and
runner.
On 2010/06/29 15:09:41, ncarter wrote:
> Why do we need a custom suite and runner?

base/test/run_all_unittests.cc has some dependencies we don't need, like some
gtk stuff (see conditions block).

[Paweł Hajdan Jr., 2010-06-29 18:16:55]

Code I commented in the drive-by LGTM.

[ncarter (slow), 2010-06-29 20:08:55]

http://codereview.chromium.org/2861034/diff/28001/29009
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2861034/diff/28001/29009#newcode1842
chrome/chrome_tests.gypi:1842: 'target_name': 'sync_notifier_unit_tests',
On 2010/06/29 18:12:32, akalin wrote:
> On 2010/06/29 15:09:41, ncarter wrote:
> > Why not just add registration_manager_unittest to sync_unit_tests?
> 
> sync_unit_tests depends on the world, and sync_notifier_unit_tests needs to
> depend on much less.

If you take this approach to its logical conclusion, then each _unittest.cc file
would wind up in its own executable.  The downside to that situation is more
targets to keep in mind when trying to run manually, and a bigger list of
executables to run in the various trybot/buildbot/valgrind configs.  I'm not
saying that there's not a place for sharding the unit tests, but I am arguing
that this instance looks oversharded, and we should avoid incurring the
overhead.

Which dependencies are you eager to avoid?  You depend on 'sync' here, which in
my mind means that you pull in most of what 'sync_unit_tests' would, even if it
gets discarded at the link step.

[ghc, 2010-06-29 20:36:15]

http://codereview.chromium.org/2861034/diff/10002/36005
File chrome/browser/sync/notifier/registration_manager.cc (right):

http://codereview.chromium.org/2861034/diff/10002/36005#newcode112
chrome/browser/sync/notifier/registration_manager.cc:112: if
(result.status().code() != invalidation::Status::SUCCESS) {
do you leave it in the PENDING state in this case?

http://codereview.chromium.org/2861034/diff/10002/36006
File chrome/browser/sync/notifier/registration_manager.h (right):

http://codereview.chromium.org/2861034/diff/10002/36006#newcode39
chrome/browser/sync/notifier/registration_manager.h:39: // return true
immediately after calling RegisterType(model_type).
remove the second 'immediately'?

[akalin, 2010-06-29 21:37:50]

On 2010/06/29 20:08:55, ncarter wrote:
> http://codereview.chromium.org/2861034/diff/28001/29009
> File chrome/chrome_tests.gypi (right):
> 
> If you take this approach to its logical conclusion, then each _unittest.cc
file
> would wind up in its own executable.  The downside to that situation is more
> targets to keep in mind when trying to run manually, and a bigger list of
> executables to run in the various trybot/buildbot/valgrind configs.  I'm not
> saying that there's not a place for sharding the unit tests, but I am arguing
> that this instance looks oversharded, and we should avoid incurring the
> overhead.
> 
> Which dependencies are you eager to avoid?  You depend on 'sync' here, which
in
> my mind means that you pull in most of what 'sync_unit_tests' would, even if
it
> gets discarded at the link step.

sync_unit_tests depends on WebCore (?) at least on Mac, and most of chrome.  I
filed a bug ( http://code.google.com/p/chromium/issues/detail?id=47910 ) to
track and fix this.  (And assigned to Tim; if he's mad at you, this is probably
why. :) )

Although yeah, I think I'm convinced that it's oversharded in this case.  moved
it to sync_unit_tests.

[akalin, 2010-06-29 21:38:19]

Addressed all comments, please take another look.

http://codereview.chromium.org/2861034/diff/10002/36005
File chrome/browser/sync/notifier/registration_manager.cc (right):

http://codereview.chromium.org/2861034/diff/10002/36005#newcode112
chrome/browser/sync/notifier/registration_manager.cc:112: if
(result.status().code() != invalidation::Status::SUCCESS) {
On 2010/06/29 20:36:15, ghc wrote:
> do you leave it in the PENDING state in this case?

Per our discussion, not handing errors for now.  Added TODO.

http://codereview.chromium.org/2861034/diff/10002/36006
File chrome/browser/sync/notifier/registration_manager.h (right):

http://codereview.chromium.org/2861034/diff/10002/36006#newcode39
chrome/browser/sync/notifier/registration_manager.h:39: // return true
immediately after calling RegisterType(model_type).
On 2010/06/29 20:36:15, ghc wrote:
> remove the second 'immediately'?

Done.

[ghc, 2010-06-29 21:39:11]

LGTM

[ncarter (slow), 2010-06-30 04:08:18]

On 2010/06/29 21:37:50, akalin wrote:
> On 2010/06/29 20:08:55, ncarter wrote:
> > http://codereview.chromium.org/2861034/diff/28001/29009
> > File chrome/chrome_tests.gypi (right):
> > 
> > If you take this approach to its logical conclusion, then each _unittest.cc
> file
> > would wind up in its own executable.  The downside to that situation is more
> > targets to keep in mind when trying to run manually, and a bigger list of
> > executables to run in the various trybot/buildbot/valgrind configs.  I'm not
> > saying that there's not a place for sharding the unit tests, but I am
arguing
> > that this instance looks oversharded, and we should avoid incurring the
> > overhead.
> > 
> > Which dependencies are you eager to avoid?  You depend on 'sync' here, which
> in
> > my mind means that you pull in most of what 'sync_unit_tests' would, even if
> it
> > gets discarded at the link step.
> 
> sync_unit_tests depends on WebCore (?) at least on Mac, and most of chrome.  I
> filed a bug ( http://code.google.com/p/chromium/issues/detail?id=47910 ) to
> track and fix this.  (And assigned to Tim; if he's mad at you, this is
probably
> why. :) )
> 
> Although yeah, I think I'm convinced that it's oversharded in this case. 
moved
> it to sync_unit_tests.

LGTM.  Thanks for the compromise and thanks for filing the bug.  There is no
reason on earth that sync_unit_tests should depend on WebCore, and that's the
issue we should focus on.