Per the Chromium style guide/dos-and-donts doc, don't inline non-"cheap

Per the Chromium style guide/dos-and-donts doc, don't inline non-"cheap
accessor" functions in non-test code.

This also attempts to put function declarations into some kind of consistent
order that's also compliant with the Google style guide:
* Constructors/destructors,
* Statics,
* Virtuals,
* Accessors,
* Other non-virtuals

There are a couple of minor changes to the names of some functions to try and be
consistent about "inlined functions are named unix_hacker()-style, out-of-line
functions aren't", and to make a few functions static that weren't; otherwise,
no code change.

BUG=none
TEST=none
R=isherman@chromium.org, pfeldman@chromium.org, xians@chromium.org

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

[Peter Kasting, 2013-07-23 22:40:48]

isherman: autofill, password_manager
pfeldman: devtools
xians: media

[Ilya Sherman, 2013-07-23 22:48:33]

I honestly don't see the value of the changes to the autofill/ and
password_manager/ test code.  The existing style is clearly in use in many parts
of the Chromium codebase for tests, and this isn't code that you maintain. 
Unless you're planning to fix this everywhere in the code base (which seems nigh
infeasible to me without an automated tool), it doesn't seem worth doing for
these two isolated files.  (It doesn't help that Rietveld makes it hard to see
what's actually changed when large swaths of code are moved around but otherwise
unmodified.)

[Peter Kasting, 2013-07-23 22:53:41]

On 2013/07/23 22:48:33, Ilya Sherman wrote:
> I honestly don't see the value of the changes to the autofill/ and
> password_manager/ test code.  The existing style is clearly in use in many
parts
> of the Chromium codebase for tests, and this isn't code that you maintain.

Part of the value is that my upcoming changes to the infobar code in these files
-- which I do maintain -- will be clearer when the definitions aren't mixed into
the declarations.

The other part of the value, and frankly the part that seems unarguable to me,
is that this is what our style guide says to do.  If one is going to be making
significant changes to a file anyway, it is not only reasonable but recommended
to make the touched code be style-guide-compliant at that point.  If you don't
think there is value in this style rule, then by all means post a message
advocating for its change, but until the rule is removed, we should follow it.
 
> Unless you're planning to fix this everywhere in the code base (which seems
nigh
> infeasible to me without an automated tool), it doesn't seem worth doing for
> these two isolated files.

This CL is five files out of a large number I've already done this with in the
past.

> (It doesn't help that Rietveld makes it hard to see
> what's actually changed when large swaths of code are moved around but
otherwise
> unmodified.)

As I said, nothing should be changed other than de-inlining.  I very
purposefully separated all other code changes out so that this could be a
mechanical review.

[Ilya Sherman, 2013-07-23 23:08:36]

On 2013/07/23 22:53:41, Peter Kasting wrote:
> On 2013/07/23 22:48:33, Ilya Sherman wrote:
> > I honestly don't see the value of the changes to the autofill/ and
> > password_manager/ test code.  The existing style is clearly in use in many
> parts
> > of the Chromium codebase for tests, and this isn't code that you maintain.
> 
> Part of the value is that my upcoming changes to the infobar code in these
files
> -- which I do maintain -- will be clearer when the definitions aren't mixed
into
> the declarations.
> 
> The other part of the value, and frankly the part that seems unarguable to me,
> is that this is what our style guide says to do.  If one is going to be making
> significant changes to a file anyway, it is not only reasonable but
recommended
> to make the touched code be style-guide-compliant at that point.  If you don't
> think there is value in this style rule, then by all means post a message
> advocating for its change, but until the rule is removed, we should follow it.

Very well, I've sent an email to chromium-dev.

> > Unless you're planning to fix this everywhere in the code base (which seems
> nigh
> > infeasible to me without an automated tool), it doesn't seem worth doing for
> > these two isolated files.
> 
> This CL is five files out of a large number I've already done this with in the
> past.
> 
> > (It doesn't help that Rietveld makes it hard to see
> > what's actually changed when large swaths of code are moved around but
> otherwise
> > unmodified.)
> 
> As I said, nothing should be changed other than de-inlining.  I very
> purposefully separated all other code changes out so that this could be a
> mechanical review.

Thanks for splitting this off -- that definitely makes the primary CL easier to
review.  However, it's still a pain to *actually* review *this* CL in any
meaningful sense.  If you're asking me to just trust that you've (manually?)
de-inlined everything correctly without me really looking at it, then it seems
like you may as well commit this CL without my review, since the review at that
point is just a formality.

[Peter Kasting, 2013-07-23 23:23:07]

On 2013/07/23 23:08:36, Ilya Sherman wrote:
> Very well, I've sent an email to chromium-dev.

Thanks.  Would you like me to put this CL on hold pending the outcome of that
thread?  Or should I proceed with this?

> Thanks for splitting this off -- that definitely makes the primary CL easier
to
> review.  However, it's still a pain to *actually* review *this* CL in any
> meaningful sense.

Yes :(.  My local diff tool does a better job of showing moved lines, but given
that de-inlining generally involves moving _and_ re-indenting/re-wrapping code,
I'm not aware of a good way to review.

> If you're asking me to just trust that you've (manually?)
> de-inlined everything correctly without me really looking at it, then it seems
> like you may as well commit this CL without my review, since the review at
that
> point is just a formality.

Well, I thought about sending the whole review to e.g. sky for a single
rubber-stamp.  But I decided I'd rather have OWNERs of the affected areas at
least know this was happening :).  I suppose I could have marked this TBR, but I
think the thread on chromium-dev is a positive outcome of not having done so.

[Ilya Sherman, 2013-07-23 23:28:51]

On 2013/07/23 23:23:07, Peter Kasting wrote:
> On 2013/07/23 23:08:36, Ilya Sherman wrote:
> > Very well, I've sent an email to chromium-dev.
> 
> Thanks.  Would you like me to put this CL on hold pending the outcome of that
> thread?  Or should I proceed with this?

I'd prefer that you put this CL on hold for now, since I personally prefer the
preexisting style (I find it much less boilerplate-y).  Thanks :)

[Peter Kasting, 2013-07-24 22:41:43]

Based on the discussion thread, I have removed all de-inlining from test files
in this change.

This still does do some minor re-ordering/re-naming, and a bit of de-inlining in
the non-test code.  All other cleanup remains postponed to a subsequent change.

Hopefully, this is ready for re-review.

[Ilya Sherman, 2013-07-25 01:13:34]

lgtm

[pfeldman, 2013-07-25 05:02:44]

Thanks for doing that - I was originally skeptical about the change, but now I
see that readability and source size were not affected much.

The way changes in devtools_window are structured are -everything +everything in
terms of diff. It makes it pretty much impossible to review properly. How do we
handle this? Could you extract the changes into a separate CL (CLs) and migrate
classes one by one (or let us do that). Alternatively, you could temporarily
violate the order of definitions for the sake of a reasonable diff and then move
things in place?

[no longer working on chromium, 2013-07-25 08:52:03]

lgtm to the media.

[Peter Kasting, 2013-07-25 18:50:00]

On 2013/07/25 05:02:44, pfeldman wrote:
> The way changes in devtools_window are structured are -everything +everything
in
> terms of diff. It makes it pretty much impossible to review properly. How do
we
> handle this? Could you extract the changes into a separate CL (CLs) and
migrate
> classes one by one (or let us do that). Alternatively, you could temporarily
> violate the order of definitions for the sake of a reasonable diff and then
move
> things in place?

I'm not sure I understand your comments completely.  The only three things
happening to the devtools code in this change are:

* Deinline DevToolsConfirmInfoBarDelegate
* Adjust order of rest of code so definitions match declarations and
declarations are in some kind of consistent order
* Made GetMinimumWidth/Height/GetMinimizedHeight static (so I wouldn't have to
reorder them again if I did this in a future change)

There are no functional changes or other stylistic changes, and I'm not sure how
to break the diff down any more since 99% of the changes are to the
DevToolsWindow class (everything else is basically in a single block at the top
of the file).  So the only thing to review is whether I got the order in the .h
file right.  If a different ordering would make more sense I am happy to change
to that.

If you want to confirm that in moving code around I didn't change anything else,
you can download and apply the patch and then use some better diff tool that can
indicate this, e.g. p4merge.

[Peter Kasting, 2013-07-29 21:58:55]

ping pfeldman.

[pfeldman, 2013-07-31 07:56:17]

devtools rs lgtm

[Peter Kasting, 2013-07-31 18:27:38]

Committed patchset #3 manually as r214781 (presubmit successful).