Cleanup, no functional change.

Cleanup, no functional change.

* Function declarations should either list all arguments on the same line as the function name, or one per line, indented even, starting on the same line as the function name if they fit.
* '*' is adjacent to type name, not variable name.
* Eliminate some code duplication.
* No else after return.
* Use vector::empty() and string16::clear() where appropriate.
* Various other tiny cleanups for shorter code, parallel structure, consistent indenting with other Chrome code, etc.

BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=167306

[dmazzoni, 2012-11-13 03:54:02]

lgtm

Be sure to fix the one functional error below before committing, thanks!

https://codereview.chromium.org/11359177/diff/1/content/browser/accessibility...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/11359177/diff/1/content/browser/accessibility...
content/browser/accessibility/browser_accessibility_win.cc:246: STDMETHODIMP
BrowserAccessibilityWin::accLocation(LONG* x_left,
The style guide used to allow this, oh well.

https://codereview.chromium.org/11359177/diff/1/content/browser/accessibility...
content/browser/accessibility/browser_accessibility_win.cc:2688: DCHECK(table);
This isn't safe; it's possible for a web author to create a cell that isn't a
descendant of a table.

[Peter Kasting, 2012-11-13 04:02:08]

https://codereview.chromium.org/11359177/diff/1/content/browser/accessibility...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/11359177/diff/1/content/browser/accessibility...
content/browser/accessibility/browser_accessibility_win.cc:2688: DCHECK(table);
On 2012/11/13 03:54:02, Dominic Mazzoni wrote:
> This isn't safe; it's possible for a web author to create a cell that isn't a
> descendant of a table.

Then the old code was wrong to NOTREACHED().  NOTREACHED() and DCHECK() both
mean "this absolutely cannot happen".

I can restore the conditional and simply eliminate the "else" arm, if that's the
correct answer.

[dmazzoni, 2012-11-13 04:12:12]

https://codereview.chromium.org/11359177/diff/1/content/browser/accessibility...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/11359177/diff/1/content/browser/accessibility...
content/browser/accessibility/browser_accessibility_win.cc:2688: DCHECK(table);
On 2012/11/13 04:02:08, Peter Kasting wrote:
> On 2012/11/13 03:54:02, Dominic Mazzoni wrote:
> > This isn't safe; it's possible for a web author to create a cell that isn't
a
> > descendant of a table.
> 
> Then the old code was wrong to NOTREACHED().  NOTREACHED() and DCHECK() both
> mean "this absolutely cannot happen".
> 
> I can restore the conditional and simply eliminate the "else" arm, if that's
the
> correct answer.

You're right, that was a poor choice. Please eliminate the "else" arm, thanks.

[jam, 2012-11-13 04:41:15]

On Mon, Nov 12, 2012 at 7:16 PM, <pkasting@chromium.org> wrote:

> Reviewers: Dominic Mazzoni,
>
> Description:
> Cleanup, no functional change.
>
> * Function declarations should either list all arguments on the same line
> as the
> function name, or one per line, indented even, starting on the same line
> as the
> function name if they fit.
>

Where is this coming from? The Google C++ style guide doesn't say that this
should be the case:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

i..e the second example has

ReturnType ClassName::ReallyLongFunctionName(Type par_name1, Type par_name2,
                                             Type par_name3) {

I don't see the benefit for changing code to fit one parameter per line. If
you're writing new code, feel free to write it in the style that you
prefer. But no need to change existing code. Otherwise someone else can
come and decide to put multiple parameters in each line and the code will
keep changing.

* '*' is adjacent to type name, not variable name.
> * Eliminate some code duplication.
> * No else after return.
> * Use vector::empty() and string16::clear() where appropriate.
> * Various other tiny cleanups for shorter code, parallel structure,
> consistent
> indenting with other Chrome code, etc.
>
> BUG=none
> TEST=none
>
> Please review this at
https://codereview.chromium.**org/11359177/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>   M     content/browser/accessibility/**browser_accessibility_win.h
>   M     content/browser/accessibility/**browser_accessibility_win.cc
>
>
>

[Peter Kasting, 2012-11-13 04:50:30]

This is coming from "be consistent within the file itself".

The file mostly uses this style (which is also the prevalent style in Chrome
code, though that's less important) but a small minority of declarations
randomly used a different style.

You are correct that this is not mandated by the Google style guide.  That was
not the motivating factor.

[jam, 2012-11-13 04:58:28]

On Mon, Nov 12, 2012 at 8:50 PM, <pkasting@chromium.org> wrote:

> This is coming from "be consistent within the file itself".
>

Why should the file be consistent with putting multiple parameters on one
line or not?


>
> The file mostly uses this style (which is also the prevalent style in
> Chrome
> code, though that's less important)


btw my reading of the code doesn't give me impression that this is the
majority.


> but a small minority of declarations
> randomly used a different style.
>
> You are correct that this is not mandated by the Google style guide.  That
> was
> not the motivating factor.
>

I think we should leave that part as is. fixing indentation is fine of
course, but I really prefer we don't have hysteresis by switching
formatting between alternate acceptable forms.


>
>
https://codereview.chromium.**org/11359177/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2012-11-13 05:12:58]

On 2012/11/13 04:58:28, John Abd-El-Malek wrote:
> Why should the file be consistent with putting multiple parameters on one
> line or not?

You don't see any problem with randomly alternating between different
linebreaking, wrapping, and indenting styles?

The primary rule in the style guide is consistency above all.  Do something one
way or another but not both.  We similarly ask people to pick a rule about
whether their conditionals have braces or not and then follow it consistently,
at least within a file if not within a whole module.  And the same with most
other style rules.

Maybe you don't ask this in your reviews, but you should.  See the "Parting
Words" section in the style guide, which I don't think could be much more
definitive on this topic, going so far as to ask people to be consistent on
things like the use of asterisks in comments.

> > The file mostly uses this style (which is also the prevalent style in
> > Chrome
> > code, though that's less important)
> 
> btw my reading of the code doesn't give me impression that this is the
> majority.

Not a critical point to the argument, so I don't care to debate it.

> > You are correct that this is not mandated by the Google style guide.  That
> > was
> > not the motivating factor.
> 
> I think we should leave that part as is. fixing indentation is fine of
> course, but I really prefer we don't have hysteresis by switching
> formatting between alternate acceptable forms.

I don't think I understand the use of "hysteresis" in this context.  This isn't
about changing one file's consistent and acceptable formatting to another
consistent and acceptable formatting style.  This is about fixing some outliers
within the file.

I agree with you that when something is consistent and correct, changing it just
to change it is harmful (you might introduce bugs).  And even if one part of the
codebase is inconsistent with another, if they're both locally consistent,
that's probably not enough reason to change one or the other.  So if you're
worried I'm going to go start changing the line wrapping of all function
declarations in Chrome you can stop worrying.  But this change does not fall
into those categories.

[jam, 2012-11-13 06:49:28]

I think some stuff like this is so minor that it's not worth stressing
about, or making changes to fix. I can think of other examples that would
be odd if a file wasn't consistent about, i.e. whether classes declared in
the file sometimes had the bodies inline and in other places had them
following the declaration. That would be jarring to read. Likewise, I'm
sure we have hundreds of files that somewhere have a one line statement
with brace brackets, and another line without. At the end of the day, it's
up to developer and reviewer how much to care about something this trivial.
I'm just not comfortable with third parties going to change this code
because of the hysteresis issue. If you're not going to change other files,
I don't care much about this one. But I don't think doing this to
other files will be productive.


On Mon, Nov 12, 2012 at 9:12 PM, <pkasting@chromium.org> wrote:

> On 2012/11/13 04:58:28, John Abd-El-Malek wrote:
>
>> Why should the file be consistent with putting multiple parameters on one
>> line or not?
>>
>
> You don't see any problem with randomly alternating between different
> linebreaking, wrapping, and indenting styles?
>
> The primary rule in the style guide is consistency above all.  Do
> something one
> way or another but not both.  We similarly ask people to pick a rule about
> whether their conditionals have braces or not and then follow it
> consistently,
> at least within a file if not within a whole module.  And the same with
> most
> other style rules.
>
> Maybe you don't ask this in your reviews, but you should.  See the "Parting
> Words" section in the style guide, which I don't think could be much more
> definitive on this topic, going so far as to ask people to be consistent on
> things like the use of asterisks in comments.
>
>
>  > The file mostly uses this style (which is also the prevalent style in
>> > Chrome
>> > code, though that's less important)
>>
>
>  btw my reading of the code doesn't give me impression that this is the
>> majority.
>>
>
> Not a critical point to the argument, so I don't care to debate it.
>
>
>  > You are correct that this is not mandated by the Google style guide.
>>  That
>> > was
>> > not the motivating factor.
>>
>
>  I think we should leave that part as is. fixing indentation is fine of
>> course, but I really prefer we don't have hysteresis by switching
>> formatting between alternate acceptable forms.
>>
>
> I don't think I understand the use of "hysteresis" in this context.  This
> isn't
> about changing one file's consistent and acceptable formatting to another
> consistent and acceptable formatting style.  This is about fixing some
> outliers
> within the file.
>
> I agree with you that when something is consistent and correct, changing
> it just
> to change it is harmful (you might introduce bugs).  And even if one part
> of the
> codebase is inconsistent with another, if they're both locally consistent,
> that's probably not enough reason to change one or the other.  So if you're
> worried I'm going to go start changing the line wrapping of all function
> declarations in Chrome you can stop worrying.  But this change does not
> fall
> into those categories.
>
>
https://codereview.chromium.**org/11359177/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2012-11-13 07:07:15]

On 2012/11/13 06:49:28, John Abd-El-Malek wrote:
> I'm just not comfortable with third parties going to change this code
> because of the hysteresis issue. If you're not going to change other files,
> I don't care much about this one. But I don't think doing this to
> other files will be productive.

I don't have any plans to go do this to other files.  The reason I started
touching this file to begin with was because I was trying to find and read all
the code that uses BSTRs (to convert to ScopedBstr where possible) and reading
this file was bugging me.

I was in the process of writing a follow-up note here anyway, though, and this
provides a good segue to it.

While I do think the consistency argument is true, and I think the change
overall is an improvement that should stand -- if admittedly highly trivial in
places -- that argument was not my complete original motivation.

Part of why the file was bugging me was that that first bullet in the
description is in fact what the Chrome style guide used to say about function
declarations/definitions.  It said that for years, and I enforced it in my
reviews for years before that, though not everyone on the team knew/agreed
with/followed the rule.  So that pattern is just ingrained by force of habit.  I
totally forgot that we rewrote the style guide to omit this directive (because
it went beyond the requirements of the Google style guide).

It would not have been hard to say that originally: "I admit I forgot we don't
require this anymore.  However, I still think this change is a good idea because
it makes the file style consistent." etc.  But I didn't do that.  Instead I just
stuck with the arguments that most strongly supported what I was doing.  It
wasn't an explicit conscious decision -- I think I was instinctively defensive,
for reasons that I'd rather not discuss in public (if they even need to be
discussed at all).  In retrospect, I regret that.  It's important to admit
mistakes, even minor ones; that's what's best from both a technical and an
interpersonal perspective.

So, I'm sorry I didn't say that up front.  And I'll try to remember in the
future not to enforce style rules we no longer have.