Subject .mm files to the header sorting presubmit

Subject .mm files to the header sorting presubmit

The presubmit is already robust to handle Objective-C .h files. Not
checking .mm files just adds extra review burden. Plus I can't just
press F5 in Sublime to sort lines in .mm files due to import vs include,
so it actually needs some brainpower when adding something and I keep
forgetting to do that.

This adds .mm to the list of file extensions the header sorting
presubmit checks for, and fixes the current mistakes to avoid annoying
people on presubmit with this change.

BUG=None
TBR=keybuk@chromium.org,jamescook@chromium.org,stuartmorgan@chromium.org,xhwang@chromium.org,rsesek@chromium.org,zea@chromium.org
R=thakis@chromium.org,jochen@chromium.org

Committed: https://crrev.com/574f09c11f5a366b059d0992c180543697666c8a
Cr-Commit-Position: refs/heads/master@{#330509}

[tapted, 2015-05-19 03:09:42]

tapted@chromium.org changed reviewers:
+ thakis@chromium.org

[tapted, 2015-05-19 03:10:30]

tapted@chromium.org changed reviewers:
+ jochen@chromium.org

[tapted, 2015-05-19 03:31:10]

Nico, please take a look. To ensure this is generally a good idea, and for
OWNERS in base,ui,chrome

jochen for PRESUBMIT.py and src/content OWNERS

There's a smattering of other files (device,extensions,ios,media,sandbox,sync)
[plan: TBR for OWNERS there].

Happy to split this up, but these files are pretty quiet - no conflicts cropped
up after a few days around a branch point - and this gives some data to see if
we like the sorting for Mac.

It was mostly mechanical with ./tools/sort-headers.py `git ls-files -- '*.mm'`,
but a few files were missing a gap between "their" header and the rest, or had
stuff included twice. Fixed them up.

Thanks!

https://codereview.chromium.org/1133713009/diff/80001/ios/chrome/browser/snap...
File ios/chrome/browser/snapshots/snapshot_cache.mm (right):

https://codereview.chromium.org/1133713009/diff/80001/ios/chrome/browser/snap...
ios/chrome/browser/snapshots/snapshot_cache.mm:16: #include
"base/mac/bind_objc_block.h"
heh. I guess I can delete this one. aaaand a few others.

[Nico, 2015-05-19 04:31:47]

lgtm

Mixing framework headers with regular system headers looks a bit strange, but
we'll get used to it I suppose.

https://codereview.chromium.org/1133713009/diff/100001/testing/gtest_mac.mm
File testing/gtest_mac.mm (right):

https://codereview.chromium.org/1133713009/diff/100001/testing/gtest_mac.mm#n...
testing/gtest_mac.mm:9: #include <gtest/gtest.h>
(should these be "testing/gtest/include/gtest.h" etc includes?)

[tapted, 2015-05-19 05:32:59]

https://codereview.chromium.org/1133713009/diff/100001/testing/gtest_mac.mm
File testing/gtest_mac.mm (right):

https://codereview.chromium.org/1133713009/diff/100001/testing/gtest_mac.mm#n...
testing/gtest_mac.mm:9: #include <gtest/gtest.h>
On 2015/05/19 04:31:47, Nico wrote:
> (should these be "testing/gtest/include/gtest.h" etc includes?)

Hm. testing/gtest.gyp is missing 'include_dirs' += [ '..' ] to pick up includes
rooted at src/ . So it would be a build error to change it (output follows).

testing/platform_test_mac.mm relies on a relative include (#include
"platform_test.h").

From what I remember about #include <> vs #include "" using <> here should
actually be bad, since it would prefer a system include (e.g.
/usr/include/gtest/gtest.h) over a relative one, but I think that preference is
getting overridden by -I../../testing/gtest/include in the compile line (I tried
putting a gtest.h in various places and couldn't break things).

And I think it's impossible to take out the 'include_dirs': = [ 'gtest/include'
] to enforce this, otherwise the gtest headers wouldn't find themselves (they
use ""-style, but all start with gtest/ so are not relative :|).

<>-style currently matches stuff in gtest_mac.h, so we should change them
together. So.. to fix, I think it should be a follow-up to this. i.e.
 1) replace 'gtest' in gtest.gyp's include_dirs with '..' (keep 'gtest/include'
since its needed for gtest headers to find themselves),
 2) change all the <> to "", and
 3) fix relative includes, e.g., in platform_test_mac.mm

[1/103] OBJCXX obj/testing/gtest.gtest_mac.o
FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ /*snip*/ -Igen
-I../../testing/gtest -I../../testing/gtest/include -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk
/*snip*/ -c ../../testing/gtest_mac.mm -o obj/testing/gtest.gtest_mac.o
../../testing/gtest_mac.mm:9:10: fatal error:
'testing/gtest/include/gtest/gtest.h' file not found
#include "testing/gtest/include/gtest/gtest.h"

[Nico, 2015-05-19 07:03:21]

Looks like it's intentional then. Don't worry about it :-)
On May 18, 2015 10:33 PM, <tapted@chromium.org> wrote:

>
> https://codereview.chromium.org/1133713009/diff/100001/testing/gtest_mac.mm
> File testing/gtest_mac.mm (right):
>
>
>
https://codereview.chromium.org/1133713009/diff/100001/testing/gtest_mac.mm#n...
> testing/gtest_mac.mm:9: #include <gtest/gtest.h>
> On 2015/05/19 04:31:47, Nico wrote:
>
>> (should these be "testing/gtest/include/gtest.h" etc includes?)
>>
>
> Hm. testing/gtest.gyp is missing 'include_dirs' += [ '..' ] to pick up
> includes rooted at src/ . So it would be a build error to change it
> (output follows).
>
> testing/platform_test_mac.mm relies on a relative include (#include
> "platform_test.h").
>
> From what I remember about #include <> vs #include "" using <> here
> should actually be bad, since it would prefer a system include (e.g.
> /usr/include/gtest/gtest.h) over a relative one, but I think that
> preference is getting overridden by -I../../testing/gtest/include in the
> compile line (I tried putting a gtest.h in various places and couldn't
> break things).
>
> And I think it's impossible to take out the 'include_dirs': = [
> 'gtest/include' ] to enforce this, otherwise the gtest headers wouldn't
> find themselves (they use ""-style, but all start with gtest/ so are not
> relative :|).
>
> <>-style currently matches stuff in gtest_mac.h, so we should change
> them together. So.. to fix, I think it should be a follow-up to this.
> i.e.
>  1) replace 'gtest' in gtest.gyp's include_dirs with '..' (keep
> 'gtest/include' since its needed for gtest headers to find themselves),
>  2) change all the <> to "", and
>  3) fix relative includes, e.g., in platform_test_mac.mm
>
> [1/103] OBJCXX obj/testing/gtest.gtest_mac.o
> FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++
> /*snip*/ -Igen -I../../testing/gtest -I../../testing/gtest/include
> -isysroot
>
>
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk
> /*snip*/ -c ../../testing/gtest_mac.mm -o obj/testing/gtest.gtest_mac.o
> ../../testing/gtest_mac.mm:9:10: fatal error:
> 'testing/gtest/include/gtest/gtest.h' file not found
> #include "testing/gtest/include/gtest/gtest.h"
>
> https://codereview.chromium.org/1133713009/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2015-05-19 11:24:57]

lgtm

[tapted, 2015-05-19 11:52:33]

tapted@chromium.org changed reviewers:
+ jamescook@chromium.org, keybuk@chromium.org, rsesek@chromium.org,
stuartmorgan@chromium.org, xhwang@chromium.org, zea@chromium.org

[tapted, 2015-05-19 11:52:34]

+OWNERS TBR (just sorting #includes now to avoid presubmit surprises later)

keybuk: device/bluetooth
jamescook: extensions/shell
stuartmorgan: ios/{chrome,web}
xhwang: media
rsesek: sandbox/mac
zea: sync

[tapted, 2015-05-19 11:54:18]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-05-19 11:54:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133713009/100001

[commit-bot: I haz the power, 2015-05-19 12:01:17]

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

[commit-bot: I haz the power, 2015-05-19 12:01:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tapted, 2015-05-19 12:10:22]

The CQ bit was checked by tapted@chromium.org

[tapted, 2015-05-19 12:10:23]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1133713009/#ps120001
(title: "rebase - of course... it would be one of my patches I conflict with
:|")

[commit-bot: I haz the power, 2015-05-19 12:10:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133713009/120001

[commit-bot: I haz the power, 2015-05-19 13:08:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-19 13:09:14]

Patchset 7 (id:??) landed as
https://crrev.com/574f09c11f5a366b059d0992c180543697666c8a
Cr-Commit-Position: refs/heads/master@{#330509}

[stuartmorgan, 2015-05-19 13:23:11]

ios LGTM

[Robert Sesek, 2015-05-19 14:17:45]

sandbox/mac lgtm