[Android] Do not fail the build due to lint issues.

[Android] Do not fail the build due to lint issues.

- Treat lint issues as warnings not errors
- Also, disable Recycle rule

BUG=None
NOTRY=True

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

[boliu, 2014-01-09 20:57:46]

Should we do the audit of existing suppressions and decide on a good list of
categories first before turning lint on (even not failing builds?) *ducks*

https://codereview.chromium.org/132473003/diff/1/build/android/lint/suppressi...
File build/android/lint/suppressions.xml (left):

https://codereview.chromium.org/132473003/diff/1/build/android/lint/suppressi...
build/android/lint/suppressions.xml:30: <issue id="DefaultLocale">
I actually think this is a good check. Now we can run webview tests on a device
with locale set to turkey. And I see non-test code in this list...

[frankf, 2014-01-09 21:23:11]

On 2014/01/09 20:57:46, boliu wrote:
> Should we do the audit of existing suppressions and decide on a good list of
> categories first before turning lint on (even not failing builds?) *ducks*
> 
>
https://codereview.chromium.org/132473003/diff/1/build/android/lint/suppressi...
> File build/android/lint/suppressions.xml (left):
> 
>
https://codereview.chromium.org/132473003/diff/1/build/android/lint/suppressi...
> build/android/lint/suppressions.xml:30: <issue id="DefaultLocale">
> I actually think this is a good check. Now we can run webview tests on a
device
> with locale set to turkey. And I see non-test code in this list...

There are 158 rules. If you don't mind, let's turn this on, and add additional
ones afterwards. WDYT?

[Yaron, 2014-01-09 21:37:20]

I actually haven't been keeping up with progress on lint improvements. +some
folks who I think have

[frankf, 2014-01-09 21:42:13]

On 2014/01/09 21:37:20, Yaron wrote:
> I actually haven't been keeping up with progress on lint improvements. +some
> folks who I think have

For context behind this see: https://codereview.chromium.org/127383002/

[newt (away), 2014-01-09 22:04:05]

Several of the existing lint suppressions are real bugs that should be fixed.
Let's not remove those. For example, DefaultLocale, HandlerLeak, and Recycle.

[boliu, 2014-01-09 22:47:49]

On 2014/01/09 22:04:05, newt wrote:
> Several of the existing lint suppressions are real bugs that should be fixed.
> Let's not remove those. For example, DefaultLocale, HandlerLeak, and Recycle.

I hold very different opinion on recycle:
https://codereview.chromium.org/127383002/#msg2

I don't think we should ever call MotionEvent.recycle since it breaks java OO/gc
assumptions which might lead to subtle bugs in the future.

[frankf, 2014-01-09 23:10:59]

On 2014/01/09 22:47:49, boliu wrote:
> On 2014/01/09 22:04:05, newt wrote:
> > Several of the existing lint suppressions are real bugs that should be
fixed.
> > Let's not remove those. For example, DefaultLocale, HandlerLeak, and
Recycle.
> 
> I hold very different opinion on recycle:
> https://codereview.chromium.org/127383002/#msg2
> 
> I don't think we should ever call MotionEvent.recycle since it breaks java
OO/gc
> assumptions which might lead to subtle bugs in the future.

OK, how about a compromise: In patch 2, I've re-enabled rules with 'warning'
severity
and explicitly disabled Recycle (UseSparseArray is useful most of the time?). We
can
disable rules on a case-by-case basis later, but this shouldn't block turning on
lint by default (given we don't fail the build). WDYT?

[newt (away), 2014-01-09 23:38:17]

On 2014/01/09 23:10:59, frankf wrote:
> On 2014/01/09 22:47:49, boliu wrote:
> > On 2014/01/09 22:04:05, newt wrote:
> > > Several of the existing lint suppressions are real bugs that should be
> fixed.
> > > Let's not remove those. For example, DefaultLocale, HandlerLeak, and
> Recycle.
> > 
> > I hold very different opinion on recycle:
> > https://codereview.chromium.org/127383002/#msg2
> > 
> > I don't think we should ever call MotionEvent.recycle since it breaks java
> OO/gc
> > assumptions which might lead to subtle bugs in the future.
> 
> OK, how about a compromise: In patch 2, I've re-enabled rules with 'warning'
> severity
> and explicitly disabled Recycle (UseSparseArray is useful most of the time?).
We
> can
> disable rules on a case-by-case basis later, but this shouldn't block turning
on
> lint by default (given we don't fail the build). WDYT?

Sounds good. I like being able to see all the existing lint warnings and errors
in the suppressions file. So, is lint running on local builds too now or just
the bots?

Re: Recycle. I didn't realize recycling was optional. (Technically, it's not in
SF, but that's another story). So, I'm fine with suppressing those.

[frankf, 2014-01-09 23:44:15]

On 2014/01/09 23:38:17, newt wrote:
> On 2014/01/09 23:10:59, frankf wrote:
> > On 2014/01/09 22:47:49, boliu wrote:
> > > On 2014/01/09 22:04:05, newt wrote:
> > > > Several of the existing lint suppressions are real bugs that should be
> > fixed.
> > > > Let's not remove those. For example, DefaultLocale, HandlerLeak, and
> > Recycle.
> > > 
> > > I hold very different opinion on recycle:
> > > https://codereview.chromium.org/127383002/#msg2
> > > 
> > > I don't think we should ever call MotionEvent.recycle since it breaks java
> > OO/gc
> > > assumptions which might lead to subtle bugs in the future.
> > 
> > OK, how about a compromise: In patch 2, I've re-enabled rules with 'warning'
> > severity
> > and explicitly disabled Recycle (UseSparseArray is useful most of the
time?).
> We
> > can
> > disable rules on a case-by-case basis later, but this shouldn't block
turning
> on
> > lint by default (given we don't fail the build). WDYT?
> 
> Sounds good. I like being able to see all the existing lint warnings and
errors
> in the suppressions file. So, is lint running on local builds too now or just
> the bots?
>
See: https://codereview.chromium.org/127383002/ 

> Re: Recycle. I didn't realize recycling was optional. (Technically, it's not
in
> SF, but that's another story). So, I'm fine with suppressing those.

[newt (away), 2014-01-09 23:49:23]

thanks. lgtm.

[boliu, 2014-01-10 00:11:45]

lgtm

Filed crbug.com/333089 for someone to audit the non-webview suppressions.

https://codereview.chromium.org/132473003/diff/120001/build/android/lint/supp...
File build/android/lint/suppressions.xml (right):

https://codereview.chromium.org/132473003/diff/120001/build/android/lint/supp...
build/android/lint/suppressions.xml:78: <ignore
path="PRODUCT_DIR/chromium_testshell_test_apk/classes/org/chromium/printing/PrintingControllerTest$7.class"/>
I removed a .class file for android_webview. Do you know when this is needed?

https://codereview.chromium.org/132473003/diff/120001/build/android/lint/supp...
build/android/lint/suppressions.xml:119: <issue id="Recycle" severity="ignore"/>
Yay

[frankf, 2014-01-10 01:38:03]

Thanks boliu for filing that bug

https://codereview.chromium.org/132473003/diff/120001/build/android/lint/supp...
File build/android/lint/suppressions.xml (right):

https://codereview.chromium.org/132473003/diff/120001/build/android/lint/supp...
build/android/lint/suppressions.xml:78: <ignore
path="PRODUCT_DIR/chromium_testshell_test_apk/classes/org/chromium/printing/PrintingControllerTest$7.class"/>
On 2014/01/10 00:11:45, boliu wrote:
> I removed a .class file for android_webview. Do you know when this is needed?

Lint rules can use both java and class files. Not sure why NewApi looks at class
files in this instance, perhaps something to do with having inner classes.

[commit-bot: I haz the power, 2014-01-10 01:47:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/frankf@chromium.org/132473003/120001

[commit-bot: I haz the power, 2014-01-10 03:50:35]

Change committed as 244078