[Android] Enable lint warnings by default.

[Android] Enable lint warnings by default.

Lint has been running on FYI bots for a while,
time to enable them by default on bots and for developers.

BUG=None
R=yfriedman@chromium.org

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

[frankf, 2014-01-08 22:15:18]

Another option here is to have lint issues as warning, but people tend to ignore
these similar to current deprecated warnings.

The slow down due to lint should be negligible (Total serialized time for
clobbber build is around 5 minutes).

[boliu, 2014-01-08 22:28:49]

Lint is also looking for some android specific optimization that have nothing to
do with potential bugs. I found two examples of this while going through webview
errors.

Recycle advices calling MotionEvent.recycle where MotionEvent.obtain is called.
There is definitely no error if we don't call recycle, and blindly calling it is
likely to introduce bugs.

UseSparseArrays suggests use SparseArrays instead of HashMap<Integer, Blah>.
Definitely no error here, just a performance vs memory tradeoff that only makes
sense in some situations.

I vote for ignoring these classes of lint errors all together.

[frankf, 2014-01-09 01:45:11]

On 2014/01/08 22:28:49, boliu wrote:
> Lint is also looking for some android specific optimization that have nothing
to
> do with potential bugs. I found two examples of this while going through
webview
> errors.
> 
> Recycle advices calling MotionEvent.recycle where MotionEvent.obtain is
called.
> There is definitely no error if we don't call recycle, and blindly calling it
is
> likely to introduce bugs.
> 
> UseSparseArrays suggests use SparseArrays instead of HashMap<Integer, Blah>.
> Definitely no error here, just a performance vs memory tradeoff that only
makes
> sense in some situations.
> 
> I vote for ignoring these classes of lint errors all together.

So there are couple of options here. Each lint issue has a category, and 
a severity (error versus warning). The categories are:

    Correctness
    Security
    Performance
    Usability
    Accessibility
    Internationalization
    Bi-directional Text

The ones you're referring to above are Performance and we can disable this
category.
Another option is to ignore all issues with warning severity (or just output
them
but don't fail the build). In our case though, almost all errors are NewApi.

Let me know if there's a strong opinion either way.

[boliu, 2014-01-09 02:09:53]

On 2014/01/09 01:45:11, frankf wrote:
> On 2014/01/08 22:28:49, boliu wrote:
> > Lint is also looking for some android specific optimization that have
nothing
> to
> > do with potential bugs. I found two examples of this while going through
> webview
> > errors.
> > 
> > Recycle advices calling MotionEvent.recycle where MotionEvent.obtain is
> called.
> > There is definitely no error if we don't call recycle, and blindly calling
it
> is
> > likely to introduce bugs.
> > 
> > UseSparseArrays suggests use SparseArrays instead of HashMap<Integer, Blah>.
> > Definitely no error here, just a performance vs memory tradeoff that only
> makes
> > sense in some situations.
> > 
> > I vote for ignoring these classes of lint errors all together.
> 
> So there are couple of options here. Each lint issue has a category, and 
> a severity (error versus warning). The categories are:
> 
>     Correctness
>     Security
>     Performance
>     Usability
>     Accessibility
>     Internationalization
>     Bi-directional Text
> 
> The ones you're referring to above are Performance and we can disable this
> category.
> Another option is to ignore all issues with warning severity (or just output
> them
> but don't fail the build). In our case though, almost all errors are NewApi.
> 
> Let me know if there's a strong opinion either way.

If there is no way to disable individual checks, then I think we should disable
performance category (ie no warnings either). No opinion on other categories.

[Yaron, 2014-01-09 19:25:52]

On 2014/01/09 02:09:53, boliu wrote:
> On 2014/01/09 01:45:11, frankf wrote:
> > On 2014/01/08 22:28:49, boliu wrote:
> > > Lint is also looking for some android specific optimization that have
> nothing
> > to
> > > do with potential bugs. I found two examples of this while going through
> > webview
> > > errors.
> > > 
> > > Recycle advices calling MotionEvent.recycle where MotionEvent.obtain is
> > called.
> > > There is definitely no error if we don't call recycle, and blindly calling
> it
> > is
> > > likely to introduce bugs.
> > > 
> > > UseSparseArrays suggests use SparseArrays instead of HashMap<Integer,
Blah>.
> > > Definitely no error here, just a performance vs memory tradeoff that only
> > makes
> > > sense in some situations.
> > > 
> > > I vote for ignoring these classes of lint errors all together.
> > 
> > So there are couple of options here. Each lint issue has a category, and 
> > a severity (error versus warning). The categories are:
> > 
> >     Correctness
> >     Security
> >     Performance
> >     Usability
> >     Accessibility
> >     Internationalization
> >     Bi-directional Text
> > 
> > The ones you're referring to above are Performance and we can disable this
> > category.
> > Another option is to ignore all issues with warning severity (or just output
> > them
> > but don't fail the build). In our case though, almost all errors are NewApi.
> > 
> > Let me know if there's a strong opinion either way.
> 
> If there is no way to disable individual checks, then I think we should
disable
> performance category (ie no warnings either). No opinion on other categories.

Hmm. This would make it such that developer builds also run lint, even on every
incremental build... That might be a way to get it to be more prominent but this
seems unorthodox, and many times unnecessary. I guess if you treat it like some
clang warnings then you could argue for having it here. If you want it run all
the time, then I agree the errors it lists need to be issues that we always
agree should be fixed/avoided. If there's any grey then it shouldn't fail
compile.

[frankf, 2014-01-09 20:00:55]

On 2014/01/09 19:25:52, Yaron wrote:
> On 2014/01/09 02:09:53, boliu wrote:
> > On 2014/01/09 01:45:11, frankf wrote:
> > > On 2014/01/08 22:28:49, boliu wrote:
> > > > Lint is also looking for some android specific optimization that have
> > nothing
> > > to
> > > > do with potential bugs. I found two examples of this while going through
> > > webview
> > > > errors.
> > > > 
> > > > Recycle advices calling MotionEvent.recycle where MotionEvent.obtain is
> > > called.
> > > > There is definitely no error if we don't call recycle, and blindly
calling
> > it
> > > is
> > > > likely to introduce bugs.
> > > > 
> > > > UseSparseArrays suggests use SparseArrays instead of HashMap<Integer,
> Blah>.
> > > > Definitely no error here, just a performance vs memory tradeoff that
only
> > > makes
> > > > sense in some situations.
> > > > 
> > > > I vote for ignoring these classes of lint errors all together.
> > > 
> > > So there are couple of options here. Each lint issue has a category, and 
> > > a severity (error versus warning). The categories are:
> > > 
> > >     Correctness
> > >     Security
> > >     Performance
> > >     Usability
> > >     Accessibility
> > >     Internationalization
> > >     Bi-directional Text
> > > 
> > > The ones you're referring to above are Performance and we can disable this
> > > category.
> > > Another option is to ignore all issues with warning severity (or just
output
> > > them
> > > but don't fail the build). In our case though, almost all errors are
NewApi.
> > > 
> > > Let me know if there's a strong opinion either way.
> > 
> > If there is no way to disable individual checks, then I think we should
> disable
> > performance category (ie no warnings either). No opinion on other
categories.
> 
> Hmm. This would make it such that developer builds also run lint, even on
every
> incremental build... That might be a way to get it to be more prominent but
this
> seems unorthodox, and many times unnecessary. I guess if you treat it like
some
> clang warnings then you could argue for having it here. If you want it run all
> the time, then I agree the errors it lists need to be issues that we always
> agree should be fixed/avoided. If there's any grey then it shouldn't fail
> compile.

OK, to avoid being chased by a mob with pitchforks, let's have lint not
fail the build and disable less severe rules:
https://codereview.chromium.org/132473003/

But, these warnings are only useful if seen by developers during incremental
build, otherwise
they get ignored on the bots.

[frankf, 2014-01-10 19:19:50]

We now treat all lint issues as warnings. Any objections with landing this?

[Yaron, 2014-01-10 19:23:00]

On 2014/01/10 19:19:50, frankf wrote:
> We now treat all lint issues as warnings. Any objections with landing this?

lgtm

[frankf, 2014-01-10 22:02:19]

Committed patchset #2 manually as r244256 (presubmit successful).

[DaleCurtis, 2014-01-10 22:28:23]

A revert of this CL has been created in
https://codereview.chromium.org/134673003/ by dalecurtis@chromium.org.

The reason for reverting is: Broke android build..

[frankf, 2014-01-11 00:35:44]

On 2014/01/10 22:28:23, DaleCurtis wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/134673003/ by mailto:dalecurtis@chromium.org.
> 
> The reason for reverting is: Broke android build..

Interesting, it seems in that instance lint binary failed with a non-zero
rc and created a blank or invalid xml result file. I haven't seen
this happen locally or on other bots.