Clean up dependencies for disk_cache histogram_macros.h.

Clean up dependencies for disk_cache histogram_macros.h.

The current mechanism by which these convenience macros find their
backend is confusing, at best; the macro has the form of our include
guard macros, but describes a CC file. Embedded within the
histogram_macros.h then is an abstraction violation: it just knows
which object has backend information depending on which file it was
included from.

Some downstream reviews have made this even more confusing; rather
than continue this, let's parameterize the backend object to the
convenience macros, so each translation unit is responsible for
knowing its own context.

R=mmenke@chromium.org, rvargas@chromium.org
TBR=mmenke@chromium.org
BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251116

[gavinp, 2014-02-06 18:28:10]

mmenke, rvargas: PTAL.

This review is upstream of the net/disk_cache directory re-orgs. In the first of
those reviews https://codereview.chromium.org/121643003/ , there were review
comments to change the macro that controlled histogram macros to being a macro
that looks like an include guard, but refers to a file that doesn't even exist.
That seemed... one step too confusing for me.

Instead, let's remove the abstraction violation and just explicitly parameterise
this at each call site.

[rvargas (doing something else), 2014-02-08 03:57:37]

Sorry for the delay, I'll try to go over it on Monday.

[rvargas (doing something else), 2014-02-11 02:17:15]

https://codereview.chromium.org/140893013/diff/260001/chrome/browser/net/http...
File chrome/browser/net/http_pipelining_compatibility_client.cc (right):

https://codereview.chromium.org/140893013/diff/260001/chrome/browser/net/http...
chrome/browser/net/http_pipelining_compatibility_client.cc:30: //
TODO(gavinp,rvargas): Clean up this dependency by moving the needed
TODOs reference the person that added the line, not the person who is in charge
of fixing it (aka, only one person).

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/backend_...
File net/disk_cache/backend_impl.cc (left):

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/backend_...
net/disk_cache/backend_impl.cc:31: // This has to be defined before including
histogram_macros.h from this file.
The comment is still needed.

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:37: #define
CACHE_HISTOGRAM_MACROS_BACKEND_IMPL_OBJ backend_
needs a comment (on every file)

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/histogra...
File net/disk_cache/histogram_macros.h (right):

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/histogra...
net/disk_cache/histogram_macros.h:87: #define BACKEND_OBJ
CACHE_HISTOGRAM_MACROS_BACKEND_IMPL_OBJ
Why not go all the way and let the caller define BACKEND_OBJ (aka, remove this
line)? The fact that the caller is including this file means that BACKEND_OBJ
gets defined anyway for that compilation unit.

The only weird case could be the user from chrome/net, but this file was never
intended to be used outside of disk_cache to start with, and it is rather
unfortunate that it was.

[gavinp, 2014-02-11 16:12:43]

PTAL.

https://codereview.chromium.org/140893013/diff/260001/chrome/browser/net/http...
File chrome/browser/net/http_pipelining_compatibility_client.cc (right):

https://codereview.chromium.org/140893013/diff/260001/chrome/browser/net/http...
chrome/browser/net/http_pipelining_compatibility_client.cc:30: //
TODO(gavinp,rvargas): Clean up this dependency by moving the needed
On 2014/02/11 02:17:15, rvargas wrote:
> TODOs reference the person that added the line, not the person who is in
charge
> of fixing it (aka, only one person).

Done.

However, do note that
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#TODO_Comments
says it's for whoever can provide context, which I thought was both of us. About
1% of TODOs in our codebase have commas right now.

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/backend_...
File net/disk_cache/backend_impl.cc (left):

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/backend_...
net/disk_cache/backend_impl.cc:31: // This has to be defined before including
histogram_macros.h from this file.
On 2014/02/11 02:17:15, rvargas wrote:
> The comment is still needed.

Done. I've updated the comment to be both narrower and more explanatory.

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/histogra...
File net/disk_cache/histogram_macros.h (right):

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/histogra...
net/disk_cache/histogram_macros.h:87: #define BACKEND_OBJ
CACHE_HISTOGRAM_MACROS_BACKEND_IMPL_OBJ
On 2014/02/11 02:17:15, rvargas wrote:
> Why not go all the way and let the caller define BACKEND_OBJ (aka, remove this
> line)? The fact that the caller is including this file means that BACKEND_OBJ
> gets defined anyway for that compilation unit.
> 
> The only weird case could be the user from chrome/net, but this file was never
> intended to be used outside of disk_cache to start with, and it is rather
> unfortunate that it was.

Done. Went with BLOCKFILE_BACKEND_IMPL_OBJ, since this is blockfile cache
specific.

[rvargas (doing something else), 2014-02-11 19:41:48]

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/histogra...
File net/disk_cache/histogram_macros.h (right):

https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/histogra...
net/disk_cache/histogram_macros.h:87: #define BACKEND_OBJ
CACHE_HISTOGRAM_MACROS_BACKEND_IMPL_OBJ
On 2014/02/11 16:12:45, gavinp wrote:
> On 2014/02/11 02:17:15, rvargas wrote:
> > Why not go all the way and let the caller define BACKEND_OBJ (aka, remove
this
> > line)? The fact that the caller is including this file means that
BACKEND_OBJ
> > gets defined anyway for that compilation unit.
> > 
> > The only weird case could be the user from chrome/net, but this file was
never
> > intended to be used outside of disk_cache to start with, and it is rather
> > unfortunate that it was.
> 
> Done. Went with BLOCKFILE_BACKEND_IMPL_OBJ, since this is blockfile cache
> specific.

Lots of redundancy on that name IMO (BLOCKFILE is implied given that this will
live under a folder with that name (and not being a public api), and there is no
way to have an object of something that is not the implementation of a backend).
But the difference is small enough to not matter much.

https://codereview.chromium.org/140893013/diff/700001/net/disk_cache/backend_...
File net/disk_cache/backend_impl.cc (right):

https://codereview.chromium.org/140893013/diff/700001/net/disk_cache/backend_...
net/disk_cache/backend_impl.cc:31: // Define BLOCKFILE_BACKEND_IMPL_OBJ to be a
disk_cache::BackendImpl* in order
nit: I much prefer the original comment. It explain _why_ the include is here
rather than as part of the regular list of includes, without re-spelling what
the code already says.

The new comment "reads aloud" the code, mentions a symbol that is not evident
here (what is it, where is it, why it matters), and leaves the important part
out (this define is needed for the header that follows).

[rvargas (doing something else), 2014-02-11 19:42:29]

On 2014/02/11 19:41:48, rvargas wrote:
>
https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/histogra...
> File net/disk_cache/histogram_macros.h (right):
> 
>
https://codereview.chromium.org/140893013/diff/260001/net/disk_cache/histogra...
> net/disk_cache/histogram_macros.h:87: #define BACKEND_OBJ
> CACHE_HISTOGRAM_MACROS_BACKEND_IMPL_OBJ
> On 2014/02/11 16:12:45, gavinp wrote:
> > On 2014/02/11 02:17:15, rvargas wrote:
> > > Why not go all the way and let the caller define BACKEND_OBJ (aka, remove
> this
> > > line)? The fact that the caller is including this file means that
> BACKEND_OBJ
> > > gets defined anyway for that compilation unit.
> > > 
> > > The only weird case could be the user from chrome/net, but this file was
> never
> > > intended to be used outside of disk_cache to start with, and it is rather
> > > unfortunate that it was.
> > 
> > Done. Went with BLOCKFILE_BACKEND_IMPL_OBJ, since this is blockfile cache
> > specific.
> 
> Lots of redundancy on that name IMO (BLOCKFILE is implied given that this will
> live under a folder with that name (and not being a public api), and there is
no
> way to have an object of something that is not the implementation of a
backend).
> But the difference is small enough to not matter much.
> 
>
https://codereview.chromium.org/140893013/diff/700001/net/disk_cache/backend_...
> File net/disk_cache/backend_impl.cc (right):
> 
>
https://codereview.chromium.org/140893013/diff/700001/net/disk_cache/backend_...
> net/disk_cache/backend_impl.cc:31: // Define BLOCKFILE_BACKEND_IMPL_OBJ to be
a
> disk_cache::BackendImpl* in order
> nit: I much prefer the original comment. It explain _why_ the include is here
> rather than as part of the regular list of includes, without re-spelling what
> the code already says.
> 
> The new comment "reads aloud" the code, mentions a symbol that is not evident
> here (what is it, where is it, why it matters), and leaves the important part
> out (this define is needed for the header that follows).

forgot to mention: the comment matters, the name not so much.

[gavinp, 2014-02-12 21:09:52]

PTAL.

I've updated the macro name, and the comment.

[rvargas (doing something else), 2014-02-12 22:27:39]

LGTM

[gavinp, 2014-02-13 13:51:10]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-02-13 13:51:20]

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

[commit-bot: I haz the power, 2014-02-13 14:16:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 14:16:06]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[mmenke, 2014-02-13 16:44:09]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-02-13 16:44:28]

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

[commit-bot: I haz the power, 2014-02-13 17:12:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 17:12:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[gavinp, 2014-02-13 18:53:07]

Adding mmenke to TBR for http_pipeline_compatibility_client.cc.

[gavinp, 2014-02-13 18:53:12]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-02-13 18:54:18]

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

[mmenke, 2014-02-13 18:55:13]

On 2014/02/13 18:53:12, gavinp wrote:
> The CQ bit was checked by mailto:gavinp@chromium.org

Pipelining stuff LGTM.  I'm thinking we should probably just remove it, anyways.
 It's not huge, but getting rid of it will reduce binary size a bit, which is
nice from a networking library standpoint.

[commit-bot: I haz the power, 2014-02-13 19:13:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 19:13:51]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[gavinp, 2014-02-13 19:37:15]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-02-13 20:00:28]

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

[commit-bot: I haz the power, 2014-02-13 20:40:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 20:40:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[gavinp, 2014-02-13 20:46:58]

Committed patchset #12 manually as r251116.