Auto-format style pass over files.

Auto-format style pass over files.

This is a refactor (actually reformat) only - no behavior change in place.

BUG=NONE
TEST=net_unittests.exe

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=46349

[cbentzel, 2010-04-28 16:13:12]

So, this is a bit of a strange review.

I wrote a quick batch script which auto-indents files using google-c-style.el
(and removes trailing whitespace and CRLF style line endings). The current
patchset shows the results from it running on the net/http directory. Before I
commit this CL, I would need to do some manual cleanup (such as fixing the
80-char line issues the new indentation caused).

What I'd really like is feedback on whether this works correctly on a few corner
cases. If we have to do manual edits each time the auto-indenter does it's work,
it's not going to be very useful. I marked up a few representative examples of
where it may not be doing the correct thing.

I will make a few manual edits and re-run the auto indent batch script so you
can see the results after a little massaging.

http://codereview.chromium.org/1800003/diff/1/13
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/1800003/diff/1/13#newcode905
net/http/http_cache_transaction.cc:905: STATE_CACHE_WRITE_RESPONSE;
This feels like it should line up under STATE_CACHE_WRITE_TRUNCATED_RESPONSE.

http://codereview.chromium.org/1800003/diff/1/14
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/1800003/diff/1/14#newcode335
net/http/http_cache_transaction.h:335: write_headers_callback_;
Even if this is valid, it seems less readable.

http://codereview.chromium.org/1800003/diff/1/18
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1800003/diff/1/18#newcode3726
net/http/http_network_transaction_unittest.cc:3726: //    {
This could be handled by #if 0'ing this block rather than commenting it out. Or,
once the comments are removed this will indent correctly.

http://codereview.chromium.org/1800003/diff/1/20
File net/http/http_response_headers_unittest.cc (left):

http://codereview.chromium.org/1800003/diff/1/20#oldcode533
net/http/http_response_headers_unittest.cc:533: "Content-type: text/html\n"
String continuations seemed to be indented before, rather than lined up like
post-auto-indent.

http://codereview.chromium.org/1800003/diff/1/23
File net/http/http_transaction_unittest.h (right):

http://codereview.chromium.org/1800003/diff/1/23#newcode39
net/http/http_transaction_unittest.h:39: TEST_MODE_SYNC_CACHE_START |
TEST_MODE_SYNC_CACHE_READ |
This feels incorrect as well - but can be worked around by wrapping in
parentheses.

[willchan no longer on Chromium, 2010-04-28 19:27:47]

I got a bit tired reading through it.  I think the false positive rate is high
enough that we shouldn't do this.  It causes a lot of 80 column violations.

I'd say this is a good tool to flag potential errors that a person needs to
manually go through and pick.  And in cases where the style guide doesn't
specify anything and it's not clear if the change is a win, I would go with the
change because it's automatic for emacs here (even though I use the vim settings
which are slightly different).

http://codereview.chromium.org/1800003/diff/1/3
File net/http/http_alternate_protocols_unittest.cc (right):

http://codereview.chromium.org/1800003/diff/1/3#newcode45
net/http/http_alternate_protocols_unittest.cc:45:
HttpAlternateProtocols::NPN_SPDY_1),
Wtf, this is a bug.  We shouldn't be using the comma operator, it should be a
semicolon.  Haha.  Can you fix that here?

Also, this autostyle thing isn't respecting 80 columns.

http://codereview.chromium.org/1800003/diff/1/7
File net/http/http_auth_handler_digest_unittest.cc (right):

http://codereview.chromium.org/1800003/diff/1/7#newcode260
net/http/http_auth_handler_digest_unittest.cc:260: tests[i].req_path,
tests[i].username, tests[i].password,
80 columns is broken here.

http://codereview.chromium.org/1800003/diff/1/8
File net/http/http_auth_handler_ntlm_portable.cc (right):

http://codereview.chromium.org/1800003/diff/1/8#newcode125
net/http/http_auth_handler_ntlm_portable.cc:125: NTLM_NegotiateUnicode |
I don't know if this is right?  I think subsequent lines in a statement should
be indented.

http://codereview.chromium.org/1800003/diff/1/12
File net/http/http_cache.cc (right):

http://codereview.chromium.org/1800003/diff/1/12#newcode800
net/http/http_cache.cc:800:
task_factory_.NewRunnableMethod(&HttpCache::OnProcessPendingQueue,
80 columns.

http://codereview.chromium.org/1800003/diff/1/13
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/1800003/diff/1/13#newcode905
net/http/http_cache_transaction.cc:905: STATE_CACHE_WRITE_RESPONSE;
On 2010/04/28 16:13:12, cbentzel wrote:
> This feels like it should line up under STATE_CACHE_WRITE_TRUNCATED_RESPONSE.

I think it's unclear according to the style guide.  Whatever is more readable to
you.  I agree your suggestion is more readable.

http://codereview.chromium.org/1800003/diff/1/14
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/1800003/diff/1/14#newcode335
net/http/http_cache_transaction.h:335: write_headers_callback_;
On 2010/04/28 16:13:12, cbentzel wrote:
> Even if this is valid, it seems less readable.

I agree, it's less readable.

http://codereview.chromium.org/1800003/diff/1/15
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/1800003/diff/1/15#newcode304
net/http/http_cache_unittest.cc:304: &MockDiskEntry::RunCallback, callback,
result));
80 columns

http://codereview.chromium.org/1800003/diff/1/17
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1800003/diff/1/17#newcode63
net/http/http_network_transaction.cc:63:
HttpUtil::SpecForRequest(request_info->url) :
Not clear to me that this is correct.  I think it's unspecified in the style
guide.  I guess this is more readable though.

http://codereview.chromium.org/1800003/diff/1/17#newcode1429
net/http/http_network_transaction.cc:1429: base::TimeDelta::FromMilliseconds(1),
base::TimeDelta::FromMinutes(10),
80 columns

http://codereview.chromium.org/1800003/diff/1/18
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1800003/diff/1/18#newcode241
net/http/http_network_transaction_unittest.cc:241:
CaptureGroupNameTCPSocketPool;
I'm not sure if this is good.  It becomes hard to tell where one statement ends
and a new one begins, at least for me.  Again, unspecified in style guide.

http://codereview.chromium.org/1800003/diff/1/18#newcode5063
net/http/http_network_transaction_unittest.cc:5063:
MockAuthHandlerCanonical::RESOLVE_SYNC :
80 columns

[wtc, 2010-04-28 22:39:57]

Chris,

I didn't read the entire CL.  Feel free to check in the
changes that you're sure are correct and doublecheck with
cpplint.py.

http://codereview.chromium.org/1800003/diff/1/2
File net/http/des.cc (left):

http://codereview.chromium.org/1800003/diff/1/2#oldcode134
net/http/des.cc:134: done:
cpplint.py says labels should be indented by 1.  (I did not
indent labels before, either.)

[cbentzel, 2010-04-29 13:53:41]

Thanks for looking.

I can fix the 80-char line issues manually, and re-running the script will
preserve the fixed indentation.

I really wish google-c-style.el, cpplint.py, and the vim module agreed on
indentation rules. I was hoping we could avoid spending any developer time
on reviewing and fixing indentation and formatting issues. Alas, it doesn't
look like we're there yet.

On Wed, Apr 28, 2010 at 6:39 PM, <wtc@chromium.org> wrote:

> Chris,
>
> I didn't read the entire CL.  Feel free to check in the
> changes that you're sure are correct and doublecheck with
> cpplint.py.
>
>
> http://codereview.chromium.org/1800003/diff/1/2
> File net/http/des.cc (left):
>
> http://codereview.chromium.org/1800003/diff/1/2#oldcode134
> net/http/des.cc:134: done:
> cpplint.py says labels should be indented by 1.  (I did not
> indent labels before, either.)
>
>
> http://codereview.chromium.org/1800003/show
>

[eroman, 2010-04-29 19:11:13]

> I really wish google-c-style.el, cpplint.py, and the vim module agreed on
> indentation rules. I was hoping we could avoid spending any developer time
> on reviewing and fixing indentation and formatting issues.

+1000000000

It is absolutely absurd how much of each review session we spend on triviality
like indentation, whose detection and fixing should be 100% automated.