ForceTLS: persist to disk

ForceTLS: persist to disk

[abarth-chromium, 2009-09-02 19:49:30]

Awesome!  Thanks for doing this.  This LGTM after you address the minor points
below.

http://codereview.chromium.org/186014/diff/1001/2001
File chrome/browser/force_tls_persister.cc (right):

http://codereview.chromium.org/186014/diff/1001/2001#newcode41
Line 41: // Runs on |file_thread_|
You can change this into a DCHECK using ChromeThread

http://codereview.chromium.org/186014/diff/1001/2006
File net/base/force_tls_state.cc (right):

http://codereview.chromium.org/186014/diff/1001/2006#newcode45
Line 45: if (callback_)
I'd factor this into a private helper (NotifyDirty or some-such).

http://codereview.chromium.org/186014/diff/1001/2006#newcode215
Line 215: }
We should probably filter out expired entries here and mark ourselves dirty. 
Otherwise the store will collect junk over time.

http://codereview.chromium.org/186014/diff/1001/2007
File net/base/force_tls_state.h (right):

http://codereview.chromium.org/186014/diff/1001/2007#newcode57
Line 57: void SetDirtyCallback(void (*callback) (void*), void* userdata);
You can make this take a Task for type safety.  You can then create the Task in
the persister using NewRunnableMethod.

[agl, 2009-09-02 20:47:44]

http://codereview.chromium.org/186014/diff/1001/2001
File chrome/browser/force_tls_persister.cc (right):

http://codereview.chromium.org/186014/diff/1001/2001#newcode41
Line 41: // Runs on |file_thread_|
On 2009/09/02 19:49:30, abarth wrote:
> You can change this into a DCHECK using ChromeThread

Done.

http://codereview.chromium.org/186014/diff/1001/2006
File net/base/force_tls_state.cc (right):

http://codereview.chromium.org/186014/diff/1001/2006#newcode215
Line 215: }
On 2009/09/02 19:49:30, abarth wrote:
> We should probably filter out expired entries here and mark ourselves dirty. 
> Otherwise the store will collect junk over time.

Done.

http://codereview.chromium.org/186014/diff/1001/2007
File net/base/force_tls_state.h (right):

http://codereview.chromium.org/186014/diff/1001/2007#newcode57
Line 57: void SetDirtyCallback(void (*callback) (void*), void* userdata);
On 2009/09/02 19:49:30, abarth wrote:
> You can make this take a Task for type safety.  You can then create the Task
in
> the persister using NewRunnableMethod.

So, the reason why I didn't do this is because Tasks take a reference to the
object which they're going to be calling. So ForceTLSState would have a
reference to ForceTLSPersister, but the persister also has a reference to
ForceTLSState. Thus we have a reference cycle and neither would ever be deleted.

I could override the templates to not take a reference in this case, but I feel
like it's a lot of code. I'm not bother either way if you really want to make it
a Task though.

[abarth-chromium, 2009-09-02 21:02:10]

We're into the tail of nit picking.  This is good as-is, or you could decide to
address the two comments below.

http://codereview.chromium.org/186014/diff/1001/2007
File net/base/force_tls_state.h (right):

http://codereview.chromium.org/186014/diff/1001/2007#newcode57
Line 57: void SetDirtyCallback(void (*callback) (void*), void* userdata);
On 2009/09/02 20:47:44, agl wrote:
> So, the reason why I didn't do this is because Tasks take a reference to the
> object which they're going to be calling. So ForceTLSState would have a
> reference to ForceTLSPersister, but the persister also has a reference to
> ForceTLSState. Thus we have a reference cycle and neither would ever be
deleted.

Ah, that makes sense.

> I could override the templates to not take a reference in this case, but I
feel
> like it's a lot of code. I'm not bother either way if you really want to make
it
> a Task though.

Please don't do that.  Overriding that template is dangerous because other
callsites don't expect it.

The really correct thing to do is make a delegate interface for ForceTLSState
with an OnStateChanged() virtual function.  You can decide if that's worth doing
to get rid of the reinterpret_cast.

http://codereview.chromium.org/186014/diff/1002/1008
File net/base/force_tls_state.cc (right):

http://codereview.chromium.org/186014/diff/1002/1008#newcode218
Line 218: continue;
You didn't mark us dirty, which means we won't flush the reduced set back to
disk unless something else randomly alters the memory version of the table.