sync: avoid creating a HistoryModelWorker unless it is needed.

sync: avoid creating a HistoryModelWorker unless it is needed. 

We typically add all known workers when Initializing the SyncBackendHost,
but this one is causing crashes and isn't used by default.


BUG=53916, 69561
TEST=none

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

[tim (not reviewing), 2011-01-12 21:55:06]

+ brettw FYI

[lipalani, 2011-01-12 22:02:52]

Just one comment. If you have a reason for not doing it that way then LGTM!!

http://codereview.chromium.org/6111012/diff/1/chrome/browser/sync/glue/sync_b...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/6111012/diff/1/chrome/browser/sync/glue/sync_b...
chrome/browser/sync/glue/sync_backend_host.cc:110: }
may be HasSwitch check alone is sufficient. The types comes from the registered
data types. And it would not be registered unless the command line was passed. 
The problem with the types check is that when somebody fixes the typed urls to
be default, this code would need no modification, so they would not look for it
and it would stay on forever until some tester tests by disabling typed url and
then reenables and finds things crashing.
Does that make sense?

[lipalani, 2011-01-12 22:06:51]

http://codereview.chromium.org/6111012/diff/1/chrome/browser/sync/glue/sync_b...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/6111012/diff/1/chrome/browser/sync/glue/sync_b...
chrome/browser/sync/glue/sync_backend_host.cc:104:
switches::kEnableSyncTypedUrls || types.count(syncable::TYPED_URLS))) {
Also is this code compiling. It looks like you need to have a closing parantheis
after switches::kEnableSyncTypedUrls.

[tim (not reviewing), 2011-01-12 23:41:27]

http://codereview.chromium.org/6111012/diff/1/chrome/browser/sync/glue/sync_b...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/6111012/diff/1/chrome/browser/sync/glue/sync_b...
chrome/browser/sync/glue/sync_backend_host.cc:104:
switches::kEnableSyncTypedUrls || types.count(syncable::TYPED_URLS))) {
On 2011/01/12 22:06:51, lipalani wrote:
> Also is this code compiling. It looks like you need to have a closing
parantheis
> after switches::kEnableSyncTypedUrls.

Doh. Hadn't uploaded the patch set with this fixed :) Did now.

http://codereview.chromium.org/6111012/diff/1/chrome/browser/sync/glue/sync_b...
chrome/browser/sync/glue/sync_backend_host.cc:110: }
On 2011/01/12 22:02:53, lipalani wrote:
> may be HasSwitch check alone is sufficient. The types comes from the
registered
> data types. And it would not be registered unless the command line was passed.

> The problem with the types check is that when somebody fixes the typed urls to
> be default, this code would need no modification, so they would not look for
it
> and it would stay on forever until some tester tests by disabling typed url
and
> then reenables and finds things crashing.
> Does that make sense?

The reason for the types check is unittests. See the case below with password
store.

[lipalani, 2011-01-13 00:31:46]

Sounds good about the unit test. 
And it is not possible to only have the types test and leave out the command
line flag, as it is possible user disabled typed urls and can choose to
re-enable later.

so LGTM!!

http://codereview.chromium.org/6111012/diff/1/chrome/browser/sync/glue/sync_b...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/6111012/diff/1/chrome/browser/sync/glue/sync_b...
chrome/browser/sync/glue/sync_backend_host.cc:110: }
sounds good.
On 2011/01/12 23:41:28, timsteele wrote:
> On 2011/01/12 22:02:53, lipalani wrote:
> > may be HasSwitch check alone is sufficient. The types comes from the
> registered
> > data types. And it would not be registered unless the command line was
passed.
> 
> > The problem with the types check is that when somebody fixes the typed urls
to
> > be default, this code would need no modification, so they would not look for
> it
> > and it would stay on forever until some tester tests by disabling typed url
> and
> > then reenables and finds things crashing.
> > Does that make sense?
> 
> The reason for the types check is unittests. See the case below with password
> store.

[brettw, 2011-01-13 03:31:53]

http://codereview.chromium.org/6111012/diff/10001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/6111012/diff/10001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:105: // TODO(tim): Bug 53916. 
HistoryModelWorker crashes, so avoid adding it
You may want to make a new bug in the sync component that you guys can track
progress on the underlying crash. Then we can close the current bug which is
blocking the release, which I think will make the release tracking & merging
easier.

[tim (not reviewing), 2011-01-13 17:54:01]

On 2011/01/13 03:31:53, brettw wrote:
>
http://codereview.chromium.org/6111012/diff/10001/chrome/browser/sync/glue/sy...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
http://codereview.chromium.org/6111012/diff/10001/chrome/browser/sync/glue/sy...
> chrome/browser/sync/glue/sync_backend_host.cc:105: // TODO(tim): Bug 53916. 
> HistoryModelWorker crashes, so avoid adding it
> You may want to make a new bug in the sync component that you guys can track
> progress on the underlying crash. Then we can close the current bug which is
> blocking the release, which I think will make the release tracking & merging
> easier.

Filed 69561!