Also scan .m and .mm files in checkdeps.py

Also scan .m and .mm files in checkdeps.py

- add necessary DEPS rules
- fix skia includes in test_shell to contain full path (third_party/skia/include/core/SkBitmap.h instead of just SkBitmap.h)
- remove forbidden chrome/common include from app/resource_bundle_mac.mm

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

[Paweł Hajdan Jr., 2009-05-08 15:07:37]

Please review:

brettw: checkdeps.py changes
pinkerton: I'm not sure about .m files. Please comment from Mac perspective.

[pink (ping after 24hrs), 2009-05-08 15:28:37]

Seems like we want both .m and .mm but i have no idea of the impact of this
patch or why it's needed.

[Mark Mentovai, 2009-05-08 15:41:24]

LGTM.  Yes, we do want this.  Be careful, though: it's possible that we have
existing #includes and #imports that don't comply, and checkdeps would trip on
them.

Also, checkdeps should look for #import as well as #include if it doesn't
already.

[TVL, 2009-05-08 15:45:49]

http://codereview.chromium.org/113147/diff/1/2
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/113147/diff/1/2#newcode73
Line 73: INCLUDE_PREFIX = "#include"
if you really want to check .m/.mm you need to also support #import

[Mark Mentovai, 2009-05-08 15:47:23]

Sorry, the LGTM covered the concept, not the code.

http://codereview.chromium.org/113147/diff/1/2
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/113147/diff/1/2#newcode73
Line 73: INCLUDE_PREFIX = "#include"
Support #import too.

http://codereview.chromium.org/113147/diff/1/2#newcode285
Line 285: for extension in checked_extensions:
return extension in checked_extensions

http://codereview.chromium.org/113147/diff/1/2#newcode295
Line 295: if line[0:8] != "#include":
Support #import too.

[TVL, 2009-05-08 16:00:09]

http://codereview.chromium.org/113147/diff/1/2
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/113147/diff/1/2#newcode87
Line 87: EXTRACT_INCLUDE_FILENAME = re.compile(INCLUDE_PREFIX + ' *"(.*)"')
fyi - to really match the preprocessor you need something like:

  [ \t]*#[ \t]*(include|import)[ \t]+"(.*)"

[Paweł Hajdan Jr., 2009-05-08 16:49:27]

Patch updated.

Few more comments about "why?". I'm working on removing dependency on libviews
on Linux (http://crbug.com/11066). I was looking at checkdeps.py as a related
tool, and just noticed that .mm files are not scanned.

After also scanning for #imports, this is what pops up:

Using base directory: /b/slave/linux/build/src
Checking: /b/slave/linux/build/src
ERROR in /b/slave/linux/build/src/webkit/tools/test_shell/test_shell_mac.mm
Illegal include: "skia/include/SkBitmap.h"
    Because of no rule applying
Illegal include: "mac/DumpRenderTreePasteboard.h"
    Because of no rule applying

ERROR in /b/slave/linux/build/src/chrome/browser/shell_integration_mac.mm
Illegal include: "third_party/mozilla/include/NSWorkspace+Utils.h"
    Because of no rule applying

ERROR in /b/slave/linux/build/src/chrome/browser/cocoa/status_bubble_mac.mm
Illegal include: "third_party/GTM/AppKit/GTMNSBezierPath+RoundRect.h"
    Because of no rule applying

ERROR in /b/slave/linux/build/src/chrome/browser/cocoa/tab_cell.mm
Illegal include: "third_party/GTM/AppKit/GTMTheme.h"
    Because of no rule applying
Illegal include: "third_party/GTM/AppKit/GTMNSColor+Luminance.h"
    Because of no rule applying

What should I do? This probably means that some rules would have to be added,
but let's first check that these dependencies are valid (and whether they should
be really #imports, for example for Skia).

[Mark Mentovai, 2009-05-08 20:21:16]

http://codereview.chromium.org/113147/diff/8/9
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/113147/diff/8/9#newcode87
Line 87: EXTRACT_INCLUDE_FILENAME = re.compile('(?:' +
'|'.join(INCLUDE_PREFIXES) +
We should probably go whole-hog and accept any whitespace (including tabs) as
TVL suggested.

Since INCLUDE_PREFIXES isn't used anywhere else, I think it's fine to just write
the RE as a string right here.  REs are already unreadable enough for most
people, no sense in making it worse by interspersing a Python join.

[Paweł Hajdan Jr., 2009-05-11 17:05:50]

Updated regex. Can someone review the list of "Illegal includes" in my previous
comment and tell what should be done? Now that checkdeps.py catches #imports
there should be probably some rules for them (of course if these dependencies
get accepted).

[Paweł Hajdan Jr., 2009-05-14 17:33:46]

ping

[brettw, 2009-05-14 19:17:46]

LGTM, sorry for the delay.

[Paweł Hajdan Jr., 2009-05-14 19:20:17]

brettw: no problem, still one problem with DEPS. I'll ask pink.

pink: Please comment about checkdeps failures in .mm and #import. I posted a
list of newly discovered "Illegal includes" in one of my earlier comments, also
with some ideas what to do with them.

[pink (ping after 24hrs), 2009-05-14 19:21:45]

This is Mark and Tom's territory. They have much more understanding of what's
going on here than I do, and I defer to their reviews.

[Paweł Hajdan Jr., 2009-05-15 11:00:49]

DEPS rules added, skia includes fixed.

Please review carefully. Things I have comments about:
- can we tighten the rule for GTM just to browser/cocoa dir?
- should we accept all of third_party/mozilla includes in all of chrome?
- should we allow direct inclusion of WebKit headers in Mac test_shell?
- should we add any TODOs, bugs, or comments to review these dependencies later
and maybe remove or further restrict them?

[Mark Mentovai, 2009-05-15 20:48:23]

lgtm

http://codereview.chromium.org/113147/diff/16/18
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/113147/diff/16/18#newcode274
Line 274: if len(file_name) < 2:
This <2 check should come out now that this function has been rewritten.

http://codereview.chromium.org/113147/diff/16/19
File webkit/tools/DEPS (right):

http://codereview.chromium.org/113147/diff/16/19#newcode6
Line 6: "+mac",
Do we really want to do this long-term?  If not, leave a note behind about doing
cleanup, or file a bug listing the files that currently need this.

[Paweł Hajdan Jr., 2009-05-18 11:43:18]

Done. I also removed one forbidden include from resource_bundle_mac.mm.

Filed a bug about the mac dep.