Make the net logging code more compact.

Make the net logging code more compact.

The generation of the lookup table between errors and strings for
the net log expanded to 37 KB, due to the repeated code. Rewriting
the code to a loop and it shrank by roughly 75%.

Total change: -32087 bytes
==========================
  3 added, totalling +960 bytes across 1 sources
  60 removed, totalling -1143 bytes across 1 sources
  1 shrunk, for a net change of -31904 bytes (36336 bytes before, 4432 bytes after) across 1 sources

BUG=
R=mmenke@chromium.org

Committed: https://crrev.com/3795272d194b346ef1f066936cb20e67bd4b5dba
Cr-Commit-Position: refs/heads/master@{#295338}

[Daniel Bratell, 2014-09-15 12:31:16]

bratell@opera.com changed reviewers:
+ mmenke@chromium.org

[mmenke, 2014-09-15 14:06:08]

Only realized you hadn't sent this out for review yet after making comments,
sorry about that.  Pretty much just nits.

Great idea to shrink this code, by the way!

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:83: };
nit:  -2 indent, and blank line after struct definition.

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:90: { #label, (value) },
Any reason value is in parentheses here, but not elsewhere?

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:90: { #label, (value) },
Can these defines all fit on one row?  (Except perhaps the LOAD_STATE one)

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:91: static const StringToConstantTable
certStatusFlags[] = {
constants should use kConstantNamingScheme

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:91: static const StringToConstantTable
certStatusFlags[] = {
Don't use static:  It makes this code potentially non-thread-safe, I believe.

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:94: #undef CERT_STATUS_FLAG
optional:  I think these are little more readable if you put the #ifdef/#undef
immediately abody/below the include statement - that way, the array definition
stands out a bit more, and I think it makes the indentation a bit less
confusing.

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:112: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(loadFlags); i++)
Looks like we aren't including the header for ARRAYSIZE_UNSAFE.  Could you add
base/macros.h to net_log_logger.h (Which should already have it, for
DISALLOW_COPY_AND_ASSIGN, but does not).

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:124: { # label, net::LOAD_STATE_ ## label },
Believe this should use a 2-space indent, like the others.

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:127: }; // End loadStateTable.
optional nit:  I don't think this comment adds anything.  If you disagree, feel
free to keep them, but add one space before the comment (Should be two spaces
between code and comments).

[mmenke, 2014-09-15 14:37:42]

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:124: { # label, net::LOAD_STATE_ ## label },
On 2014/09/15 14:06:08, mmenke wrote:
> Believe this should use a 2-space indent, like the others.

Sorry, that should have been 4-space indent.

[Daniel Bratell, 2014-09-15 16:11:51]

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:90: { #label, (value) },
On 2014/09/15 14:06:08, mmenke wrote:
> Any reason value is in parentheses here, but not elsewhere?

I had a compiler error I thought was because of the lack of () but it was
because of the semi colons and then I forgot about it. I will remove them again.

https://codereview.chromium.org/574433002/diff/1/net/base/net_log_logger.cc#n...
net/base/net_log_logger.cc:94: #undef CERT_STATUS_FLAG
On 2014/09/15 14:06:08, mmenke wrote:
> optional:  I think these are little more readable if you put the #ifdef/#undef
> immediately abody/below the include statement - that way, the array definition
> stands out a bit more, and I think it makes the indentation a bit less
> confusing.

Good suggestion! It did make it more readable!

[mmenke, 2014-09-15 16:17:29]

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:70: struct StringToConstantTable {
nit:  This should be in an anonymous namespace at the top of the file, so it's
not at global scope.  Doesn't matter if it's nested in the net namespace or not.

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:70: struct StringToConstantTable {
nit:  This is more of a StringToConstantTableEntry than a StringToConstantTable

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:75: static const StringToConstantTable
kCertStatusFlags[] = {
nit:  It's more common not to use the static, and to put these in the anonymous
namespace with StringToConstantTable instead.

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:114: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kCertStatusFlags); i++)
Should include macros.h for ARRAYSIZE_UNSAFE.  It should be in the header, since
the header also depends on it.

[eroman, 2014-09-15 21:57:36]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2014-09-15 21:57:37]

sounds like a worthwhile change, thanks!

[Daniel Bratell, 2014-09-16 09:32:20]

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:70: struct StringToConstantTable {
On 2014/09/15 16:17:29, mmenke wrote:
> nit:  This should be in an anonymous namespace at the top of the file, so it's
> not at global scope.  Doesn't matter if it's nested in the net namespace or
not.

Done.

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:70: struct StringToConstantTable {
On 2014/09/15 16:17:29, mmenke wrote:
> nit:  This should be in an anonymous namespace at the top of the file, so it's
> not at global scope.  Doesn't matter if it's nested in the net namespace or
not.

Done.

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:75: static const StringToConstantTable
kCertStatusFlags[] = {
On 2014/09/15 16:17:29, mmenke wrote:
> nit:  It's more common not to use the static, and to put these in the
anonymous
> namespace with StringToConstantTable instead.

Done.

https://codereview.chromium.org/574433002/diff/20001/net/base/net_log_logger....
net/base/net_log_logger.cc:114: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kCertStatusFlags); i++)
On 2014/09/15 16:17:29, mmenke wrote:
> Should include macros.h for ARRAYSIZE_UNSAFE.  It should be in the header,
since
> the header also depends on it.

Done.

[mmenke, 2014-09-16 14:24:12]

LGTM, just a tiny nit.  No need to publish another message to ask for a
re-review - publishing your responses is enough.

[mmenke, 2014-09-16 14:24:29]

Oops, forgot to publish my nit.

https://codereview.chromium.org/574433002/diff/40001/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/574433002/diff/40001/net/base/net_log_logger....
net/base/net_log_logger.cc:51: }
nit:

}  // namespace

[Daniel Bratell, 2014-09-17 19:31:58]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-09-17 19:33:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/574433002/60001

[commit-bot: I haz the power, 2014-09-17 20:32:59]

Committed patchset #4 (id:60001) as 05aa513761eb0cc013e41711d4139073a9ce56e3

[commit-bot: I haz the power, 2014-09-17 20:33:34]

Patchset 4 (id:??) landed as
https://crrev.com/3795272d194b346ef1f066936cb20e67bd4b5dba
Cr-Commit-Position: refs/heads/master@{#295338}