Code quality and standards in SimpleBackend.

Code quality and standards in SimpleBackend.

In the review of https://codereview.chromium.org/13880016/ , rvargas
pointed out that we could use WeakPtr safely in the SimpleEntryImpl,
even though it's destructed on any thread, since the WeakPtr can be
destructed on any thread.

As well, he asked for raw pointer function arguments, and some grammar
fixes.

While I was here, I delinted.

R=pliard,rvargas
BUG=233103
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194873

[gavinp, 2013-04-18 09:13:32]

pliard, can you please take a look at this fairly quickly? It's going to create
merge hell for the team, so we should land it quickly.

rvargas, please take a look over to make sure I've addressed your concerns.

[Philippe, 2013-04-18 09:30:10]

Thanks Gavin for the cleanup! Mostly LG but see my comments about TaskRunner.

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.h:40: base::TaskRunner* cache_thread,
What kind of guarantees do you need from this TaskRunner (and others)? I suspect
that you actually need a SingleThreadTaskRunner. For example, TaskRunner does
not guarantee the order in which posted tasks are run. It's a very high level
abstraction. In practice it will work as expected since you will provide an
instance of the correct implementation but I would suggest better expressing
your expectations here with a SingleThreadTaskRunner.

This would significantly increase the diff though :/

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:32: 
Nit: I am not sure we have this blank line usually.

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:120: const
scoped_refptr<base::MessageLoopProxy> constructor_thread_;
While you are here you might consider making this a const
scoped_refptr<base::SingleThreadTaskRunner>. The migration will be seamless
since MessageLoopProxy is a SingleThreadTaskRunner. Sorry for not pointing this
out earlier.

[gavinp, 2013-04-18 09:46:18]

OK Philippe, here we are.

This is... going to be hard to sync to head... It's there now...

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.h:40: base::TaskRunner* cache_thread,
On 2013/04/18 09:30:10, Philippe wrote:
> What kind of guarantees do you need from this TaskRunner (and others)? I
suspect
> that you actually need a SingleThreadTaskRunner. For example, TaskRunner does
> not guarantee the order in which posted tasks are run. It's a very high level
> abstraction. In practice it will work as expected since you will provide an
> instance of the correct implementation but I would suggest better expressing
> your expectations here with a SingleThreadTaskRunner.
> 
> This would significantly increase the diff though :/

Done.

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:32: 
On 2013/04/18 09:30:10, Philippe wrote:
> Nit: I am not sure we have this blank line usually.

I just rebased to HEAD, and it was there.

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:120: const
scoped_refptr<base::MessageLoopProxy> constructor_thread_;
On 2013/04/18 09:30:10, Philippe wrote:
> While you are here you might consider making this a const
> scoped_refptr<base::SingleThreadTaskRunner>. The migration will be seamless
> since MessageLoopProxy is a SingleThreadTaskRunner. Sorry for not pointing
this
> out earlier.

Done.

https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:120: const
scoped_refptr<base::MessageLoopProxy> constructor_thread_;
On 2013/04/18 09:30:10, Philippe wrote:
> While you are here you might consider making this a const
> scoped_refptr<base::SingleThreadTaskRunner>. The migration will be seamless
> since MessageLoopProxy is a SingleThreadTaskRunner. Sorry for not pointing
this
> out earlier.

Done.

[Philippe, 2013-04-18 10:07:16]

On 2013/04/18 09:46:18, gavinp wrote:
> OK Philippe, here we are.
> 
> This is... going to be hard to sync to head... It's there now...
> 
>
https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
> File net/disk_cache/simple/simple_backend_impl.h (right):
> 
>
https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_backend_impl.h:40: base::TaskRunner*
cache_thread,
> On 2013/04/18 09:30:10, Philippe wrote:
> > What kind of guarantees do you need from this TaskRunner (and others)? I
> suspect
> > that you actually need a SingleThreadTaskRunner. For example, TaskRunner
does
> > not guarantee the order in which posted tasks are run. It's a very high
level
> > abstraction. In practice it will work as expected since you will provide an
> > instance of the correct implementation but I would suggest better expressing
> > your expectations here with a SingleThreadTaskRunner.
> > 
> > This would significantly increase the diff though :/
> 
> Done.
> 
>
https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
> File net/disk_cache/simple/simple_entry_impl.h (right):
> 
>
https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_entry_impl.h:32: 
> On 2013/04/18 09:30:10, Philippe wrote:
> > Nit: I am not sure we have this blank line usually.
> 
> I just rebased to HEAD, and it was there.
> 
>
https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_entry_impl.h:120: const
> scoped_refptr<base::MessageLoopProxy> constructor_thread_;
> On 2013/04/18 09:30:10, Philippe wrote:
> > While you are here you might consider making this a const
> > scoped_refptr<base::SingleThreadTaskRunner>. The migration will be seamless
> > since MessageLoopProxy is a SingleThreadTaskRunner. Sorry for not pointing
> this
> > out earlier.
> 
> Done.
> 
>
https://codereview.chromium.org/14022012/diff/11002/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_entry_impl.h:120: const
> scoped_refptr<base::MessageLoopProxy> constructor_thread_;
> On 2013/04/18 09:30:10, Philippe wrote:
> > While you are here you might consider making this a const
> > scoped_refptr<base::SingleThreadTaskRunner>. The migration will be seamless
> > since MessageLoopProxy is a SingleThreadTaskRunner. Sorry for not pointing
> this
> > out earlier.
> 
> Done.

lgtm, thanks Gavin!

[commit-bot: I haz the power, 2013-04-18 10:11:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14022012/21001

[gavinp, 2013-04-18 10:21:10]

Committed patchset #5 manually as r194873 (presubmit successful).

[rvargas (doing something else), 2013-04-18 17:31:50]

lgtm

https://codereview.chromium.org/14022012/diff/21001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/14022012/diff/21001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:32: 
nit: extra line

https://codereview.chromium.org/14022012/diff/21001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14022012/diff/21001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.cc:24: EntryMetadata::EntryMetadata() :
hash_key_(0),
nit: this initializer list has to start on the next line.