NTP: clang-format

NTP: clang-format

Formatted paths:
  //chrome/browser/android/ntp/…/*.{h,cc}
  //chrome/browser/ntp_snippets/…/*.{h,cc}
  //chrome/browser/ntp_tiles/…/*.{h,cc}
  //components/ntp_snippets/…/*.{h,cc}
  //components/ntp_tiles/…/*.{h,cc}

Review-Url: https://codereview.chromium.org/2744253004
Cr-Commit-Position: refs/heads/master@{#459678}
Committed: https://chromium.googlesource.com/chromium/src/+/e8969bcc1d3a7841c604f451798492b5e2172013

[sfiera, 2017-03-14 15:59:23]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 16:00:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2017-03-14 16:05:00]

Description was changed from

==========
NTP: clang-format

Formatted paths:
  //chrome/browser/android/ntp/…/*.{h,cc}
  //chrome/browser/ntp_snippets/…/*.{h,cc}
  //chrome/browser/ntp_tiles/…/*.{h,cc}
  //components/ntp_snippets/…/*.{h,cc}
  //components/ntp_tiles/…/*.{h,cc}

If you get merge conflicts rebasing past this CL, you should be able to
recover with:
  $ git rebase --abort  # cancel in-progress rebase
  $ git diff --stat @{u}  # list files you've changed
  $ clang-format $THOSE_FILES  # fully format files in your CL
  $ git add -A && git commit -m clang-format  # commit all of that
  $ git branch tmp  # save that in a temporary branch
  $ git rebase  # start the rebase again (you'll get conflicts)
  $ git checkout tmp $THOSE_FILES  # get the clang-formatted versions
  $ git rebase --continue  # finish the rebase
  $ git branch -D tmp  # get rid of the temporary branch
==========

to

==========
NTP: clang-format

Formatted paths:
  //chrome/browser/android/ntp/…/*.{h,cc}
  //chrome/browser/ntp_snippets/…/*.{h,cc}
  //chrome/browser/ntp_tiles/…/*.{h,cc}
  //components/ntp_snippets/…/*.{h,cc}
  //components/ntp_tiles/…/*.{h,cc}
==========

[sfiera, 2017-03-14 16:10:13]

sfiera@chromium.org changed reviewers:
+ bauerb@chromium.org

[sfiera, 2017-03-14 16:10:14]

Since presubmits now check formatting, format everything. I'll announce this to
the list and reformat from head before submitting.

My interest in this is because I have clang-format integration set up in my
editor, but it only works at the file level, so I can't use it without causing
incidental changes until everything's formatted.

[vitaliii, 2017-03-14 16:33:04]

On 2017/03/14 16:10:14, sfiera wrote:
> Since presubmits now check formatting, format everything. I'll announce this
to
> the list and reformat from head before submitting.
> 
> My interest in this is because I have clang-format integration set up in my
> editor, but it only works at the file level, so I can't use it without causing
> incidental changes until everything's formatted.

Could you write in the description which command exactly you use to do
formatting? 
(just want to make sure that others are using the same)

[Bernhard Bauer, 2017-03-14 16:43:08]

On 2017/03/14 16:10:14, sfiera wrote:
> Since presubmits now check formatting, format everything. I'll announce this
to
> the list and reformat from head before submitting.
> 
> My interest in this is because I have clang-format integration set up in my
> editor, but it only works at the file level, so I can't use it without causing
> incidental changes until everything's formatted.

TBH, that seems a bit like it's a problem with your local tooling... `git cl
format` works for me to only format changed lines, and I've found that quite
nice.

[commit-bot: I haz the power, 2017-03-14 16:48:36]

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

[commit-bot: I haz the power, 2017-03-14 16:48:37]

Dry run: This issue passed the CQ dry run.

[sfiera, 2017-03-14 17:05:32]

On 2017/03/14 16:43:08, Bernhard Bauer wrote:
> On 2017/03/14 16:10:14, sfiera wrote:
> > Since presubmits now check formatting, format everything. I'll announce this
> to
> > the list and reformat from head before submitting.
> > 
> > My interest in this is because I have clang-format integration set up in my
> > editor, but it only works at the file level, so I can't use it without
causing
> > incidental changes until everything's formatted.
> 
> TBH, that seems a bit like it's a problem with your local tooling... `git cl
> format` works for me to only format changed lines, and I've found that quite
> nice.

I'm aware of git cl format. There is probably a way to integrate it into my
editor, but I don't think that's worth the time. Everything is going to end up
formatted anyway because of the new presubmits. (I think that the repo should
have been formatted before adding the presubmit, and said as much in the
annoucement, but we have the presubmit now and our code might as well be in line
with it)

vitaliii, the command is just clang-format $FILE (add -i for in-place
formatting). clang-format is in depot_tools, so it should already be in your
$PATH.

[vitaliii, 2017-03-14 17:07:37]

On 2017/03/14 17:05:32, sfiera wrote:
> On 2017/03/14 16:43:08, Bernhard Bauer wrote:
> > On 2017/03/14 16:10:14, sfiera wrote:
> > > Since presubmits now check formatting, format everything. I'll announce
this
> > to
> > > the list and reformat from head before submitting.
> > > 
> > > My interest in this is because I have clang-format integration set up in
my
> > > editor, but it only works at the file level, so I can't use it without
> causing
> > > incidental changes until everything's formatted.
> > 
> > TBH, that seems a bit like it's a problem with your local tooling... `git cl
> > format` works for me to only format changed lines, and I've found that quite
> > nice.
> 
> I'm aware of git cl format. There is probably a way to integrate it into my
> editor, but I don't think that's worth the time. Everything is going to end up
> formatted anyway because of the new presubmits. (I think that the repo should
> have been formatted before adding the presubmit, and said as much in the
> annoucement, but we have the presubmit now and our code might as well be in
line
> with it)
> 
> vitaliii, the command is just clang-format $FILE (add -i for in-place
> formatting). clang-format is in depot_tools, so it should already be in your
> $PATH.

Are you sure that the results are the same as 'git cl format'?

[sfiera, 2017-03-14 17:11:10]

On 2017/03/14 17:07:37, vitaliii wrote:
> On 2017/03/14 17:05:32, sfiera wrote:
> > vitaliii, the command is just clang-format $FILE (add -i for in-place
> > formatting). clang-format is in depot_tools, so it should already be in your
> > $PATH.
> 
> Are you sure that the results are the same as 'git cl format'?

If you're using the version from depot_tools, it should be the same. If you've
got it installed through apt, no, depot_tools has a more recent version.

[Bernhard Bauer, 2017-03-14 17:15:33]

On 2017/03/14 17:05:32, sfiera wrote:
> On 2017/03/14 16:43:08, Bernhard Bauer wrote:
> > On 2017/03/14 16:10:14, sfiera wrote:
> > > Since presubmits now check formatting, format everything. I'll announce
this
> > to
> > > the list and reformat from head before submitting.
> > > 
> > > My interest in this is because I have clang-format integration set up in
my
> > > editor, but it only works at the file level, so I can't use it without
> causing
> > > incidental changes until everything's formatted.
> > 
> > TBH, that seems a bit like it's a problem with your local tooling... `git cl
> > format` works for me to only format changed lines, and I've found that quite
> > nice.
> 
> I'm aware of git cl format. There is probably a way to integrate it into my
> editor, but I don't think that's worth the time. Everything is going to end up
> formatted anyway because of the new presubmits. (I think that the repo should
> have been formatted before adding the presubmit, and said as much in the
> annoucement, but we have the presubmit now and our code might as well be in
line
> with it)

Right, but using `git cl format` will satisfy the presubmit without modifying
unchanged code. Think about this way -- if you were to upload a CL that formats
all of //src, would that be accepted? If not, why should we treat a certain
subset of directories that we happen to own differently?

[sfiera, 2017-03-14 17:41:30]

On 2017/03/14 17:15:33, Bernhard Bauer wrote:
> On 2017/03/14 17:05:32, sfiera wrote:
> > On 2017/03/14 16:43:08, Bernhard Bauer wrote:
> > > On 2017/03/14 16:10:14, sfiera wrote:
> > > > Since presubmits now check formatting, format everything. I'll announce
> this
> > > to
> > > > the list and reformat from head before submitting.
> > > > 
> > > > My interest in this is because I have clang-format integration set up in
> my
> > > > editor, but it only works at the file level, so I can't use it without
> > causing
> > > > incidental changes until everything's formatted.
> > > 
> > > TBH, that seems a bit like it's a problem with your local tooling... `git
cl
> > > format` works for me to only format changed lines, and I've found that
quite
> > > nice.
> > 
> > I'm aware of git cl format. There is probably a way to integrate it into my
> > editor, but I don't think that's worth the time. Everything is going to end
up
> > formatted anyway because of the new presubmits. (I think that the repo
should
> > have been formatted before adding the presubmit, and said as much in the
> > annoucement, but we have the presubmit now and our code might as well be in
> line
> > with it)
> 
> Right, but using `git cl format` will satisfy the presubmit without modifying
> unchanged code. Think about this way -- if you were to upload a CL that
formats
> all of //src, would that be accepted? If not, why should we treat a certain
> subset of directories that we happen to own differently?

If I were changing //src, I'd be touching tens of thousands of files and
hundreds of developers, and there's no way I could check the patch over before
submitting it. This one is small enough to scan through—most of it is just
matter-of-taste stuff, but there are a couple of actual indentation fixes too.

[sfiera, 2017-03-15 09:56:49]

To expand further, the problems I would see getting flagged on //src are:

- clang-format sometimes does the wrong thing (destroying hand-formatted tables,
etc.). It doesn't here; I've checked.
- You end up in the blame history for a lot of files you don't own. Here,
though, I'm not a bad first point of contact for NTP stuff.
- It'll cause merge conflicts for in-flight CLs. I'm assuming probably ~5 for
just our tree.
- Don't do it on a weekday. I'm fine doing it on a weekend to have fewer
conflicts.

The benefits I see are:
- Editor integration of clang-format is better than git-cl-format because a)
it's faster (doesn't have to talk to our huge git repo) and b) it already exists
:)
- Some of the changes in the CL are real fixes that we should apply and the rest
will happen eventually anyway.
- Future changes would be more focused; as far as I know, git-cl-format just
operates on changed lines, so you can change an argument from false to true and
it'll go reformat a whole function call.

I think if I were going to reformat //src, the thing I'd want to do is write a
sort of inverse git-cl-format: format all lines that *other* CLs *aren't*
changing. That gets rid of the merge conflicts, and those other CLs can handle
formatting the lines they touch. Our part of the tree is small enough that I
guess I could do that manually.

[Bernhard Bauer, 2017-03-15 11:56:55]

Can we mark this CL so that git hyper-blame will skip it?

https://codereview.chromium.org/2744253004/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category.cc (right):

https://codereview.chromium.org/2744253004/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category.cc:26: << " is not a valid category ID. This
may "
Rewrap

https://codereview.chromium.org/2744253004/diff/1/components/ntp_snippets/fea...
File components/ntp_snippets/features.h (right):

https://codereview.chromium.org/2744253004/diff/1/components/ntp_snippets/fea...
components/ntp_snippets/features.h:28: ;
Remove the semicolon.

[sfiera, 2017-03-23 16:32:53]

Rebased.

After this CL is submitted, we can add its hash to src/.git-blame-ignore-revs to
have hyper-blame skip it.

[sfiera, 2017-03-24 13:14:15]

sfiera@chromium.org changed reviewers:
+ dgn@chromium.org, jkrcal@chromium.org

[sfiera, 2017-03-24 13:14:15]

+dgn,jkrcal since bauerb is OOO (as are treib and markusheintz, the other two
that could cover the whole CL)

[jkrcal, 2017-03-24 13:39:49]

lgtm

[dgn, 2017-03-24 15:52:58]

lgtm

[sfiera, 2017-03-24 16:18:20]

Thanks! I'll rebase and reformat tomorrow before submitting this.

[sfiera, 2017-03-26 17:04:56]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-03-26 17:04:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org,
dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2744253004/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-26 17:05:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-26 18:38:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490547896876460,
"parent_rev": "5388a131f6f63314ef21f730f3e6151c6ff8db56", "commit_rev":
"e8969bcc1d3a7841c604f451798492b5e2172013"}

[commit-bot: I haz the power, 2017-03-26 18:40:02]

Description was changed from

==========
NTP: clang-format

Formatted paths:
  //chrome/browser/android/ntp/…/*.{h,cc}
  //chrome/browser/ntp_snippets/…/*.{h,cc}
  //chrome/browser/ntp_tiles/…/*.{h,cc}
  //components/ntp_snippets/…/*.{h,cc}
  //components/ntp_tiles/…/*.{h,cc}
==========

to

==========
NTP: clang-format

Formatted paths:
  //chrome/browser/android/ntp/…/*.{h,cc}
  //chrome/browser/ntp_snippets/…/*.{h,cc}
  //chrome/browser/ntp_tiles/…/*.{h,cc}
  //components/ntp_snippets/…/*.{h,cc}
  //components/ntp_tiles/…/*.{h,cc}

Review-Url: https://codereview.chromium.org/2744253004
Cr-Commit-Position: refs/heads/master@{#459678}
Committed:
https://chromium.googlesource.com/chromium/src/+/e8969bcc1d3a7841c604f4517984...
==========

[commit-bot: I haz the power, 2017-03-26 18:40:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e8969bcc1d3a7841c604f4517984...