app_list: Enable level 1 of cpplint, be stricker.

app_list: Enable level 1 of cpplint, be stricker.

Otherwise we are not checking the following, because depot_tool's
presubmit_canned_checks.py:CheckChangeLintsClean filters them:

* build/include
* build/include_order
* build/namespace
* readability/casting
* runtime/int
* runtime/virtual
* whitespace/braces
* readability/inheritance

Entries were found with the following command line:

$ for f in $(g ls-files ui/app_list | grep ".*\cc$" | grep -v cocoa); do
cpplint.py $f; done

And

$ for f in $(g ls-files ui/app_list | grep ".*\h$" | grep -v cocoa); do
cpplint.py $f; done

They were cleaned up until no errors were reported.

BUG=None
TEST=see above
R=tapted@chromium.org

Committed: https://crrev.com/7fb9084142c40eca614f2fda14223489cbc06eb9
Cr-Commit-Position: refs/heads/master@{#307685}

[tfarina, 2014-12-06 21:18:07]

Trent, PTAL. (This is how cc/ is linting their code as well).

Dana, Hendrik, fyi.

[tapted, 2014-12-07 22:31:37]

I'm fine with the code changes, but I don't like the PRESUBMIT change.
CheckChangeLintsClean has justifications for each of the filters it imposes:
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/pres...

Some of them are marked ~"too hard to fix in Chrome right now", if we can mark
of areas of the codebase that are fixed then that's really awesome, but
re-implementing CheckChangeLintsClean each time isn't the way to do it. Can we
add a `lint_filters=None` argument to CheckChangeLintsClean. Then something like


if lint_filters != None:
  cpplint._SetFilters(lint_filters)
else
  cpplint.SetFilters(...

depot tools owners might have a different opinion.


A bigger concern is filters like the casting one. The ObjC files are not
currently filtered out by the presubmit, but you did filter them out according
to your CL description. Will changes to ObjC files give false positives now?

[tfarina, 2014-12-08 00:35:13]

On Sunday, December 7, 2014, <tapted@chromium.org> wrote:

> I'm fine with the code changes, but I don't like the PRESUBMIT change.
> CheckChangeLintsClean has justifications for each of the filters it
> imposes:
> https://code.google.com/p/chromium/codesearch#chromium/
> tools/depot_tools/presubmit_canned_checks.py&l=103
>
> Some of them are marked ~"too hard to fix in Chrome right now", if we can
> mark
> of areas of the codebase that are fixed then that's really awesome,

That should not be my job, don't you agree? Chromium is a multi-million
lines of code.

What I'm trying to do here is for the benefit of app_list code, which is
almost clean.

I was the one that first added PRESUBMIT to app_list.

> but
> re-implementing CheckChangeLintsClean each time isn't the way to do it.

What do you mean?


> Can we
> add a `lint_filters=None` argument to CheckChangeLintsClean. Then
> something like
>
>
> if lint_filters != None:
>   cpplint._SetFilters(lint_filters)
> else
>   cpplint.SetFilters(...
>
> depot tools owners might have a different opinion.

I can try sending that to depot_tools owners.


>
>
> A bigger concern is filters like the casting one. The ObjC files are not
> currently filtered out by the presubmit, but you did filter them out
> according
> to your CL description.

I think we are not communicating very well. Or you want to do things in a
way and I in another.

Is not clear to me what you want for ObjC. Filter them or not?


-- 
Thiago Farina

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

[tfarina, 2014-12-08 00:59:09]

I have uploaded https://codereview.chromium.org/779033003 for the depot_tools
change.

[tfarina, 2014-12-08 01:09:14]

Trent, I have filtered cocoa (or ObjC code) because when cpplint passes there,
we get many errors (37) like the following:

ui/app_list/cocoa/app_list_pager_view.h:19:  Using C-style cast.  Use
static_cast<int>(...) instead  [readability/casting] [4]
ui/app_list/cocoa/app_list_pager_view.h:22:  Using C-style cast.  Use
reinterpret_cast<AppListPagerView*>(...) instead  [readability/casting] [4]
ui/app_list/cocoa/app_list_pager_view.h:29:  Weird number of spaces at
line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]

I have asked in hendrikw's review to filter them out, but you said to don't.
Hence our disagreement here.

[tapted, 2014-12-08 01:52:20]

I'm saying, if we can mark an area of the code base (e.g. ui/app_list) as
meeting more stringent cpplint checks, then that is good. I agree changing all
of Chrome is not any one individual's responsibility.

What the PRESUBMIT.py change does in this CL is effectively marking
"ui/app_list" as meeting these more stringent checks, but the way of doing that
can be improved.

Copying a tricky function from one PRESUBMIT.py to another PRESUBMIT.py each
time we want to stamp an area as "cpplint-strict" is not maintainable, and it is
error-prone. And this is what we saw the first time CheckChangeLintsClean came
into app_list's presubmit -- it picked up a regex that only worked for code in
src/cc.

Instead of copy-pasting, we should abstract the differences, and change the API
of CheckChangeLintsClean to support them. But that's in depot_tools, so I'm
happy for this CL to land without the presubmit.py changes.

> > A bigger concern is filters like the casting one. The ObjC files are not
> > currently filtered out by the presubmit, but you did filter them out
> > according
> > to your CL description.
> 
> I think we are not communicating very well. Or you want to do things in a
> way and I in another.
> 
> Is not clear to me what you want for ObjC. Filter them or not?

Some of the checks that presubmit_canned_checks.py:CheckChangeLintsClean filters
out behave strangely on Objective C files. When I patch this in and change a
header in ui/app_list/cocoa (e.g. app_list/cocoa/apps_grid_view_item.h) I get
new errors reported when I try to upload:

/Volumes/bdd/tapted/git/chromium/src/ui/app_list/cocoa/apps_grid_view_item.h:23:
 Weird number of spaces at line-start.  Are you using a 2-space indent? 
[whitespace/indent] [3]
/Volumes/bdd/tapted/git/chromium/src/ui/app_list/cocoa/apps_grid_view_item.h:43:
 Using C-style cast.  Use reinterpret_cast<NSButton*>(...) instead 
[readability/casting] [4]
/Volumes/bdd/tapted/git/chromium/src/ui/app_list/cocoa/apps_grid_view_item.h:46:
 Using C-style cast.  Use reinterpret_cast<NSMenu*>(...) instead 
[readability/casting] [4]
/Volumes/bdd/tapted/git/chromium/src/ui/app_list/cocoa/apps_grid_view_item.h:51:
 Using C-style cast.  Use reinterpret_cast<NSBitmapImageRep*>(...) instead 
[readability/casting] [4]


whitespace/indent should be redundant with the clang-format requirement in
app-list.
readability/casting should always be disabled in objective C.

So either these filters need to stay, or the regex needs to change to ensure
headers in ui/app_list/cocoa are not checked.

[tfarina, 2014-12-10 00:46:52]

Trent, I have reverted the changes in PRESUBMIT.py, let's take that into a
separate CL.

Please, take another look.

[tapted, 2014-12-10 04:32:13]

lgtm

[tfarina, 2014-12-10 12:04:35]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-12-10 12:05:03]

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

[commit-bot: I haz the power, 2014-12-10 13:10:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-10 13:10:49]

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