Add GROUP_FILE ModelSafeGroup

Add GROUP_FILE ModelSafeGroup

BUG=
TEST=Added FileModelWorkerUnittest

[not at google - send to devlin, 2011-09-14 18:03:19]

FileModelWorker and its unit test are just copied verbatim from
DatabaseModelWorker with db/database renamed to file etc.

[not at google - send to devlin, 2011-09-14 18:03:57]

(Forgot to add Fred as a reviewer.)

[akalin, 2011-09-14 18:11:06]

LGTM after comments

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/engine/mock...
File chrome/browser/sync/engine/mock_model_safe_workers.h (right):

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/engine/mock...
chrome/browser/sync/engine/mock_model_safe_workers.h:27: class
MockFILEModelWorker : public ModelSafeWorker {
FILE -> File

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
File chrome/browser/sync/glue/file_model_worker.h (right):

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker.h:15: 
Add a TODO(sync) somewhere to combine ModelSafeWorker implementations that
differ only in which browser thread it's run on

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker.h:22: explicit FileModelWorker() {}
no explicit on non-one-arg constructor

in fact, no explicit constructor needed

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker.h:23: 
add virtual destructor (with non-inline body)

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker.h:32:
DISALLOW_COPY_AND_ASSIGN(FileModelWorker);
#include "base/basictypes.h" for this

[Paweł Hajdan Jr., 2011-09-14 18:33:01]

Drive-by with testing comment. I'd like to take another look before this gets
committed.

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
File chrome/browser/sync/glue/file_model_worker_unittest.cc (right):

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker_unittest.cc:41:
timer()->Start(FROM_HERE, TimeDelta::FromSeconds(10),
Do not hardcode timeouts. Use base/test_timeouts. Or maybe just put the thing
inside browser_tests which has a reliable out-of-proc timeout logic.

[not at google - send to devlin, 2011-09-14 19:09:18]

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/engine/mock...
File chrome/browser/sync/engine/mock_model_safe_workers.h (right):

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/engine/mock...
chrome/browser/sync/engine/mock_model_safe_workers.h:27: class
MockFILEModelWorker : public ModelSafeWorker {
On 2011/09/14 18:11:11, akalin wrote:
> FILE -> File

Done.

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
File chrome/browser/sync/glue/file_model_worker.h (right):

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker.h:15: 
On 2011/09/14 18:11:11, akalin wrote:
> Add a TODO(sync) somewhere to combine ModelSafeWorker implementations that
> differ only in which browser thread it's run on

The TODO would probably never get done, so I've done it.  Simpler.

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker.h:22: explicit FileModelWorker() {}
On 2011/09/14 18:11:11, akalin wrote:
> no explicit on non-one-arg constructor
> 
> in fact, no explicit constructor needed

Constructor needed now, but un-explicited.

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker.h:23: 
On 2011/09/14 18:11:11, akalin wrote:
> add virtual destructor (with non-inline body)

Done.

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker.h:32:
DISALLOW_COPY_AND_ASSIGN(FileModelWorker);
On 2011/09/14 18:11:11, akalin wrote:
> #include "base/basictypes.h" for this

Done.

[akalin, 2011-09-14 19:16:32]

LGTM after comments

Pawel?

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
File chrome/browser/sync/glue/browser_thread_model_worker.cc (right):

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
chrome/browser/sync/glue/browser_thread_model_worker.cc:22: DLOG(WARNING) <<
"Already on the appropriate thread.";
log the thread ID, i.e.

DLOG(WARNING) << "Already on thread " << thread_;

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
chrome/browser/sync/glue/browser_thread_model_worker.cc:35: NOTREACHED() <<
"Failed to post task to the appropriate thread.";
same here

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
File chrome/browser/sync/glue/browser_thread_model_worker.h (right):

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
chrome/browser/sync/glue/browser_thread_model_worker.h:28: // ModelSafeWorker
implementation. Called on syncapi SyncerThread.
"syncapi SyncerThread" -> "the sync thread"

[not at google - send to devlin, 2011-09-14 19:21:59]

btw

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
File chrome/browser/sync/glue/file_model_worker_unittest.cc (right):

http://codereview.chromium.org/7891054/diff/1/chrome/browser/sync/glue/file_m...
chrome/browser/sync/glue/file_model_worker_unittest.cc:41:
timer()->Start(FROM_HERE, TimeDelta::FromSeconds(10),
On 2011/09/14 18:33:01, Paweł Hajdan Jr. wrote:
> Do not hardcode timeouts. Use base/test_timeouts. Or maybe just put the thing
> inside browser_tests which has a reliable out-of-proc timeout logic.

Done (in the other test file that I kinda moved this to).

[not at google - send to devlin, 2011-09-14 20:39:02]

I'll put through the submit queue now, any more comments I can do in a follow
up.

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
File chrome/browser/sync/glue/browser_thread_model_worker.cc (right):

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
chrome/browser/sync/glue/browser_thread_model_worker.cc:22: DLOG(WARNING) <<
"Already on the appropriate thread.";
On 2011/09/14 19:16:32, akalin wrote:
> log the thread ID, i.e.
> 
> DLOG(WARNING) << "Already on thread " << thread_;

It would be nice if I could print out "FILE" instead of "3" but superficial
digging around can't find a utility like that.

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
chrome/browser/sync/glue/browser_thread_model_worker.cc:35: NOTREACHED() <<
"Failed to post task to the appropriate thread.";
On 2011/09/14 19:16:32, akalin wrote:
> same here

Done.

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
File chrome/browser/sync/glue/browser_thread_model_worker.h (right):

http://codereview.chromium.org/7891054/diff/6001/chrome/browser/sync/glue/bro...
chrome/browser/sync/glue/browser_thread_model_worker.h:28: // ModelSafeWorker
implementation. Called on syncapi SyncerThread.
On 2011/09/14 19:16:32, akalin wrote:
> "syncapi SyncerThread" -> "the sync thread"

Done.

[Paweł Hajdan Jr., 2011-09-14 20:41:14]

LGTM

http://codereview.chromium.org/7891054/diff/7001/chrome/browser/sync/glue/bro...
File chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc (right):

http://codereview.chromium.org/7891054/diff/7001/chrome/browser/sync/glue/bro...
chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc:39: // DoWork
hasn't executed within action_timeout_ms() ms
nit: Dot at the end.

[not at google - send to devlin, 2011-09-14 21:18:06]

Commit queue didn't work, it seems not to like moving files.  I'll try a git try
but I might need somebody to commit it for me (I'm not a committer).

http://codereview.chromium.org/7891054/diff/7001/chrome/browser/sync/glue/bro...
File chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc (right):

http://codereview.chromium.org/7891054/diff/7001/chrome/browser/sync/glue/bro...
chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc:39: // DoWork
hasn't executed within action_timeout_ms() ms
On 2011/09/14 20:41:14, Paweł Hajdan Jr. wrote:
> nit: Dot at the end.

Done.

[akalin, 2011-09-15 00:05:11]

How did you try the commit queue?  I think just checking the box here should
suffice; I don't see any messages from commit-bot.

On 2011/09/14 21:18:06, kalman1 wrote:
> Commit queue didn't work, it seems not to like moving files.  I'll try a git
try
> but I might need somebody to commit it for me (I'm not a committer).
> 
>
http://codereview.chromium.org/7891054/diff/7001/chrome/browser/sync/glue/bro...
> File chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc (right):
> 
>
http://codereview.chromium.org/7891054/diff/7001/chrome/browser/sync/glue/bro...
> chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc:39: // DoWork
> hasn't executed within action_timeout_ms() ms
> On 2011/09/14 20:41:14, Paweł Hajdan Jr. wrote:
> > nit: Dot at the end.
> 
> Done.

[not at google - send to devlin, 2011-09-15 00:08:22]

On 2011/09/15 00:05:11, akalin wrote:
> How did you try the commit queue?  I think just checking the box here should
> suffice; I don't see any messages from commit-bot.
> 
> On 2011/09/14 21:18:06, kalman1 wrote:
> > Commit queue didn't work, it seems not to like moving files.  I'll try a git
> try
> > but I might need somebody to commit it for me (I'm not a committer).
> > 
> >
>
http://codereview.chromium.org/7891054/diff/7001/chrome/browser/sync/glue/bro...
> > File chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc
(right):
> > 
> >
>
http://codereview.chromium.org/7891054/diff/7001/chrome/browser/sync/glue/bro...
> > chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc:39: //
DoWork
> > hasn't executed within action_timeout_ms() ms
> > On 2011/09/14 20:41:14, Paweł Hajdan Jr. wrote:
> > > nit: Dot at the end.
> > 
> > Done.

Commit bot apparently doesn't work when you move files, it won't compile.  The
try bot is fine though.

[not at google - send to devlin, 2011-09-15 00:24:20]

I.e. look at this:

http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/84...

The missing file is definitely there.  And I found a Chromium bug somewhere
which says that the commit bots don't work with moved files.

Anyway, the point is that I need this to be committed manually by someone, would
you mind doing it Fred?  I ran a git try which failed due to me not changing
there from being 3 workers to 4, but I've fixed that now (and verified that the
tests pass now).

[akalin, 2011-09-15 00:30:35]

On 2011/09/15 00:24:20, kalman1 wrote:
> I.e. look at this:
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/84...
> 
> The missing file is definitely there.  And I found a Chromium bug somewhere
> which says that the commit bots don't work with moved files.
> 
> Anyway, the point is that I need this to be committed manually by someone,
would
> you mind doing it Fred?  I ran a git try which failed due to me not changing
> there from being 3 workers to 4, but I've fixed that now (and verified that
the
> tests pass now).

Okay, I can commit this.  Can you run another round of tryjobs?

[not at google - send to devlin, 2011-09-15 00:39:06]

Submitted as trunk3_group_file#ae2eba

[commit-bot: I haz the power, 2011-09-15 00:51:42]

Try job failure for 7891054-3021 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[not at google - send to devlin, 2011-09-15 16:26:46]

On 2011/09/15 00:51:42, I haz the power (commit-bot) wrote:
> Try job failure for 7891054-3021 (retry) on win for step "compile" (clobber
> build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Lots of green.

[not at google - send to devlin, 2011-09-15 16:27:09]

Replying to the failing buildbot gave the wrong impression...

Trybot is green.

[akalin, 2011-09-15 16:52:13]

On 2011/09/15 16:27:09, kalman1 wrote:
> Replying to the failing buildbot gave the wrong impression...
> 
> Trybot is green.

Sorry, I didn't get around to this last night.  I'll try landing this today.

[akalin, 2011-09-15 18:54:44]

On 2011/09/15 16:52:13, akalin wrote:
> On 2011/09/15 16:27:09, kalman1 wrote:
> > Replying to the failing buildbot gave the wrong impression...
> > 
> > Trybot is green.
> 
> Sorry, I didn't get around to this last night.  I'll try landing this today.

Landed as http://src.chromium.org/viewvc/chrome?view=rev&revision=101346,
closing.

[tim (not reviewing), 2011-09-23 14:16:35]

On 2011/09/15 18:54:44, akalin wrote:
> On 2011/09/15 16:52:13, akalin wrote:
> > On 2011/09/15 16:27:09, kalman1 wrote:
> > > Replying to the failing buildbot gave the wrong impression...
> > > 
> > > Trybot is green.
> > 
> > Sorry, I didn't get around to this last night.  I'll try landing this today.
> 
> Landed as http://src.chromium.org/viewvc/chrome?view=rev&revision=101346,
> closing.

Why does this add a 'BrowserThreadModelWorker'?  Isn't it just for the file
thread?
I haven't been following this too closely, but how come we changed our minds
from simply using GROUP_UI for extensions settings?

I'm additionally concerned about http://crbug.com/97271.
And the recent perf test timeouts..

[akalin, 2011-09-23 17:29:18]

On Fri, Sep 23, 2011 at 7:16 AM,  <tim@chromium.org> wrote:
> Why does this add a 'BrowserThreadModelWorker'?  Isn't it just for the file
> thread?

Because otherwise it's duplicated code.

> I haven't been following this too closely, but how come we changed our minds
> from simply using GROUP_UI for extensions settings?

Because extension settings lives on the FILE thread.

>
> I'm additionally concerned about http://crbug.com/97271.
> And the recent perf test timeouts..

I don't see how this can affect any timings, unless work is actually
being done on the FILE thread.