Change which manifest and resources are passed to lint.py.

Change what manifest and resources are passed to lint.py.

BUG=266140
Committed: https://crrev.com/46a78727a03b9aa652201a41ea12ea1e56790e56
Cr-Commit-Position: refs/heads/master@{#318153}

[aurimas (slooooooooow), 2015-02-05 01:04:23]

aurimas@chromium.org changed reviewers:
+ aurimas@chromium.org

[aurimas (slooooooooow), 2015-02-05 01:04:23]

Can we add code to ignore the following lint checks (we disable them in the
internal lint.py file too):
'Assert'
'SetJavaScriptEnabled'
'MissingRegistered'
'SignatureOrSystemPermissions'
'InflateParams'
'RtlSymmetry'
'RtlCompat'
'UnusedAttribute'
'InconsistentLayout'

We found them not useful.

[aurimas (slooooooooow), 2015-02-05 01:26:24]

I applied this patch locally, but it does not seem to catch IconDensities rules.
To test, delete one icon from chrome/android/java/res/drawables-hdpi/ then build
chrome_shell_apk target. It does not seem to complain that now you are missing
an asset in one density.

[mikecase (-- gone --), 2015-02-05 18:50:12]

On 2015/02/05 01:26:24, aurimas wrote:
> I applied this patch locally, but it does not seem to catch IconDensities
rules.
> To test, delete one icon from chrome/android/java/res/drawables-hdpi/ then
build
> chrome_shell_apk target. It does not seem to complain that now you are missing
> an asset in one density.

Well, it looks like the resource_dir for chrome_shell_apk is
chrome/android/shell/res. So if the IconDensity issue was there, I'm pretty sure
it would have been caught.

I ran the lint script on chrome_shell_apk locally where I switch the resouce_dir
to be chrome/android/java/res instead. I got a bunch of lint errors, including
the ones below. I'll look into properly running lint on this directory.

chrome/android/java/res/drawable-hdpi Missing the following drawables in
`drawable-hdpi`: btn_star_empty.png, btn_star_full.png, btn_star_half.png,
card_background_default.9.png, distillation_quality_answer_no_faded.png,
distillation_quality_answer_no_pressed.png,
distillation_quality_answer_yes_faded.png,
distillation_quality_answer_yes_pressed.png, google_play_logo.png,
infobar_close_button.png: IconDensities [warning]

chrome/android/java/res/drawable-mdpi Missing the following drawables in
`drawable-mdpi`: btn_close.png, btn_star_empty.png, btn_star_full.png,
btn_star_half.png, card_background_default.9.png,
distillation_quality_answer_no_faded.png,
distillation_quality_answer_no_pressed.png,
distillation_quality_answer_yes_faded.png,
distillation_quality_answer_yes_pressed.png, google_play_logo.png: IconDensities
[warning]

chrome/android/java/res/drawable-xhdpi Missing the following drawables in
`drawable-xhdpi`: btn_star_empty.png, btn_star_full.png, btn_star_half.png,
card_background_default.9.png, google_play_logo.png, infobar_close_button.png:
IconDensities [warning]

chrome/android/java/res/drawable-xxhdpi Missing the following drawables in
`drawable-xxhdpi`: btn_bg_holo_active_normal.png,
btn_bg_holo_active_pressed.png, btn_bg_holo_pressed.png,
distillation_quality_answer_no_faded.png,
distillation_quality_answer_no_pressed.png,
distillation_quality_answer_yes_faded.png,
distillation_quality_answer_yes_pressed.png, globe_favicon.png,
globe_incognito_favicon.png, ic_stat_notify.png, infobar_close_button.png:
IconDensities [warning]

chrome/android/java/res/drawable-xxxhdpi Missing the following drawables in
`drawable-xxxhdpi`: btn_bg_holo_active_normal.png,
btn_bg_holo_active_pressed.png, btn_bg_holo_pressed.png, btn_star_empty.png,
btn_star_full.png, btn_star_half.png, card_background_default.9.png,
controlled_setting_mandatory.png, distillation_quality_answer_no_faded.png,
distillation_quality_answer_no_pressed.png,
distillation_quality_answer_yes_faded.png,
distillation_quality_answer_yes_pressed.png, globe_favicon.png,
globe_incognito_favicon.png, google_play_logo.png, ic_stat_notify.png,
infobar_close_button.png: IconDensities [warning]

chrome/android/java/res/drawable-hdpi/cvc_icon_amex.png The following unrelated
icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]

chrome/android/java/res/drawable-mdpi/cvc_icon_amex.png The following unrelated
icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]

chrome/android/java/res/drawable-xhdpi/cvc_icon_amex.png The following unrelated
icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]

chrome/android/java/res/drawable-xxhdpi/cvc_icon_amex.png The following
unrelated icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]

chrome/android/java/res/drawable-xxxhdpi/cvc_icon_amex.png The following
unrelated icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]

[aurimas (slooooooooow), 2015-02-05 18:51:44]

On 2015/02/05 at 18:50:12, mikecase wrote:
> On 2015/02/05 01:26:24, aurimas wrote:
> > I applied this patch locally, but it does not seem to catch IconDensities
rules.
> > To test, delete one icon from chrome/android/java/res/drawables-hdpi/ then
build
> > chrome_shell_apk target. It does not seem to complain that now you are
missing
> > an asset in one density.
> 
> Well, it looks like the resource_dir for chrome_shell_apk is
chrome/android/shell/res. So if the IconDensity issue was there, I'm pretty sure
it would have been caught.
> 
> I ran the lint script on chrome_shell_apk locally where I switch the
resouce_dir to be chrome/android/java/res instead. I got a bunch of lint errors,
including the ones below. I'll look into properly running lint on this
directory.
> 
> chrome/android/java/res/drawable-hdpi Missing the following drawables in
`drawable-hdpi`: btn_star_empty.png, btn_star_full.png, btn_star_half.png,
card_background_default.9.png, distillation_quality_answer_no_faded.png,
distillation_quality_answer_no_pressed.png,
distillation_quality_answer_yes_faded.png,
distillation_quality_answer_yes_pressed.png, google_play_logo.png,
infobar_close_button.png: IconDensities [warning]
> 
> chrome/android/java/res/drawable-mdpi Missing the following drawables in
`drawable-mdpi`: btn_close.png, btn_star_empty.png, btn_star_full.png,
btn_star_half.png, card_background_default.9.png,
distillation_quality_answer_no_faded.png,
distillation_quality_answer_no_pressed.png,
distillation_quality_answer_yes_faded.png,
distillation_quality_answer_yes_pressed.png, google_play_logo.png: IconDensities
[warning]
> 
> chrome/android/java/res/drawable-xhdpi Missing the following drawables in
`drawable-xhdpi`: btn_star_empty.png, btn_star_full.png, btn_star_half.png,
card_background_default.9.png, google_play_logo.png, infobar_close_button.png:
IconDensities [warning]
> 
> chrome/android/java/res/drawable-xxhdpi Missing the following drawables in
`drawable-xxhdpi`: btn_bg_holo_active_normal.png,
btn_bg_holo_active_pressed.png, btn_bg_holo_pressed.png,
distillation_quality_answer_no_faded.png,
distillation_quality_answer_no_pressed.png,
distillation_quality_answer_yes_faded.png,
distillation_quality_answer_yes_pressed.png, globe_favicon.png,
globe_incognito_favicon.png, ic_stat_notify.png, infobar_close_button.png:
IconDensities [warning]
> 
> chrome/android/java/res/drawable-xxxhdpi Missing the following drawables in
`drawable-xxxhdpi`: btn_bg_holo_active_normal.png,
btn_bg_holo_active_pressed.png, btn_bg_holo_pressed.png, btn_star_empty.png,
btn_star_full.png, btn_star_half.png, card_background_default.9.png,
controlled_setting_mandatory.png, distillation_quality_answer_no_faded.png,
distillation_quality_answer_no_pressed.png,
distillation_quality_answer_yes_faded.png,
distillation_quality_answer_yes_pressed.png, globe_favicon.png,
globe_incognito_favicon.png, google_play_logo.png, ic_stat_notify.png,
infobar_close_button.png: IconDensities [warning]
> 
> chrome/android/java/res/drawable-hdpi/cvc_icon_amex.png The following
unrelated icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]
> 
> chrome/android/java/res/drawable-mdpi/cvc_icon_amex.png The following
unrelated icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]
> 
> chrome/android/java/res/drawable-xhdpi/cvc_icon_amex.png The following
unrelated icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]
> 
> chrome/android/java/res/drawable-xxhdpi/cvc_icon_amex.png The following
unrelated icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]
> 
> chrome/android/java/res/drawable-xxxhdpi/cvc_icon_amex.png The following
unrelated icon files have identical contents: cvc_icon.png, cvc_icon_amex.png:
IconDuplicates [warning]

I uploaded a fix for a few lint warnings
https://chromiumcodereview.appspot.com/900103002

[mikecase (-- gone --), 2015-02-05 19:18:23]

Ok, I added a resource_dir variable to the chrome_java gyp target. Now lint
warnings for src/chrome/android/java/res will show up when you build.

I also suppressed the lint warnings you mentioned were not useful.

[mikecase (-- gone --), 2015-02-05 19:18:23]

Ok, I added a resource_dir variable to the chrome_java gyp target. Now lint
warnings for src/chrome/android/java/res will show up when you build.

I also suppressed the lint warnings you mentioned were not useful.

[aurimas (slooooooooow), 2015-02-05 19:28:51]

aurimas@chromium.org changed reviewers:
+ cjhopman@chromium.org

[aurimas (slooooooooow), 2015-02-05 19:28:51]

+cjhopman for gyp changes. I hope this would not be breaking anything.

[cjhopman, 2015-02-05 19:48:28]

this is great and is something I've long wanted.

also, will we be able to remove the downstream lint stuff with this change?

https://codereview.chromium.org/880083004/diff/20001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/880083004/diff/20001/chrome/chrome.gyp#newcod...
chrome/chrome.gyp:634: 'resource_dir': 'android/java/res',
This is really confusing. If you specify something other than <(java_in_dir)/res
here, lint will apply to what you specify here, but the library will actually
include the resources in the other directory.

Instead, if you look at build/java.gypi, for targets that have resources we
construct the resource directory as '<(java_in_dir)/res' in the variable
res_dir. We should pass that to lint.

Best would actually be to rename res_dir to resource_dir in java.gypi and
require it to actually be specified by the java target (maybe a first step
towards that would be to do the rename, keep the automatic setting as
<(j_i_d)/res, and allow overriding of it). If you do this, we will then get lint
on some other directories of resources like content and ui.

[mikecase (-- gone --), 2015-02-11 00:06:41]

Well, I renamed res_dir to resource_dir in java.gypi and like you said it made
lint run on resource directories like content and ui and found quite a few
IconDensity and other warnings.

I also tried into making the analogous changes to the BUILD.gn files as I did to
the gyp files. For now, I just made the lint script always use the empty
resource folder when called via GN. I couldn't figure out how to access the
resource_dirs GN variable from where the lint script is called in
internal_rules.gni. cjhopman, do you know what I will need to do? I haven't
worked much with the build files, especially the GN files.

[cjhopman, 2015-02-11 02:05:02]

On 2015/02/11 00:06:41, mikecase wrote:
> Well, I renamed res_dir to resource_dir in java.gypi and like you said it made
> lint run on resource directories like content and ui and found quite a few
> IconDensity and other warnings.
> 
> I also tried into making the analogous changes to the BUILD.gn files as I did
to
> the gyp files. For now, I just made the lint script always use the empty
> resource folder when called via GN. I couldn't figure out how to access the
> resource_dirs GN variable from where the lint script is called in
> internal_rules.gni. cjhopman, do you know what I will need to do? I haven't
> worked much with the build files, especially the GN files.

In gn, resources are built as a separate target (i.e. not bundled together with
a java target). To make this work there, I think we need to add a lint call in
the internal template that handles resources. This would then require that
lint.py work fine on just resources (and no java files). I don't know if that
works currently or not. I'm fine with gn not running lint on resources for now.

[cjhopman, 2015-02-11 02:05:10]

https://codereview.chromium.org/880083004/diff/40001/build/android/gyp/lint.py
File build/android/gyp/lint.py (right):

https://codereview.chromium.org/880083004/diff/40001/build/android/gyp/lint.p...
build/android/gyp/lint.py:168: parser.add_option('--resource-dir', help='Path to
resource dir.')
I think my slight preference would be that --resource-dir is optional
(eventually I'd like to be able to get rid of the build/android/ant/res
directory).

[mikecase (-- gone --), 2015-02-11 19:04:24]

Made resource_dir not required and for now I won't make any changes to the GN
files (they won't lint resources).

https://codereview.chromium.org/880083004/diff/40001/build/android/gyp/lint.py
File build/android/gyp/lint.py (right):

https://codereview.chromium.org/880083004/diff/40001/build/android/gyp/lint.p...
build/android/gyp/lint.py:168: parser.add_option('--resource-dir', help='Path to
resource dir.')
On 2015/02/11 02:05:09, cjhopman wrote:
> I think my slight preference would be that --resource-dir is optional
> (eventually I'd like to be able to get rid of the build/android/ant/res
> directory).

Done.

[aurimas (slooooooooow), 2015-02-12 00:40:54]

On 2015/02/11 at 19:04:24, mikecase wrote:
> Made resource_dir not required and for now I won't make any changes to the GN
files (they won't lint resources).
> 
> https://codereview.chromium.org/880083004/diff/40001/build/android/gyp/lint.py
> File build/android/gyp/lint.py (right):
> 
>
https://codereview.chromium.org/880083004/diff/40001/build/android/gyp/lint.p...
> build/android/gyp/lint.py:168: parser.add_option('--resource-dir', help='Path
to resource dir.')
> On 2015/02/11 02:05:09, cjhopman wrote:
> > I think my slight preference would be that --resource-dir is optional
> > (eventually I'd like to be able to get rid of the build/android/ant/res
> > directory).
> 
> Done.

I am running this locally. I'll try to fix as many of these warnings as I can.

[cjhopman, 2015-02-12 22:27:46]

lgtm

[aurimas (slooooooooow), 2015-02-12 22:28:30]

https://codereview.chromium.org/880083004/diff/60001/build/android/lint/suppr...
File build/android/lint/suppressions.xml (right):

https://codereview.chromium.org/880083004/diff/60001/build/android/lint/suppr...
build/android/lint/suppressions.xml:2: <lint>
We can also suppress Overdraw and RtlEnabled issues.

[aurimas (slooooooooow), 2015-02-12 22:44:00]

I prefer we do not land this before we have suppressed (and file bugs) or fixed
all the existing issues. Otherwise we will just have a ton of noise.

[mikecase (-- gone --), 2015-02-12 22:44:38]

New patchsets have been uploaded after l-g-t-m from cjhopman@chromium.org

[mikecase (-- gone --), 2015-02-12 22:48:40]

Ok, I'll hold off on landing this. Next week I'll look into suppressing all the
remaining errors and filing bugs for them. Thanks for fixing some lint errors
already aurimas  :)

https://codereview.chromium.org/880083004/diff/60001/build/android/lint/suppr...
File build/android/lint/suppressions.xml (right):

https://codereview.chromium.org/880083004/diff/60001/build/android/lint/suppr...
build/android/lint/suppressions.xml:2: <lint>
On 2015/02/12 22:28:30, aurimas wrote:
> We can also suppress Overdraw and RtlEnabled issues.

Done.

[mikecase (wrong mikecase), 2015-02-25 21:37:16]

The CQ bit was checked by mikecase@google.com

[mikecase (wrong mikecase), 2015-02-25 21:37:17]

The CQ bit was unchecked by mikecase@google.com

[mikecase (-- gone --), 2015-02-25 21:38:56]

The CQ bit was checked by mikecase@chromium.org

[aurimas (slooooooooow), 2015-02-25 21:39:41]

You need to rebase this change.

[commit-bot: I haz the power, 2015-02-25 21:39:50]

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

[mikecase (-- gone --), 2015-02-25 21:39:55]

The CQ bit was unchecked by mikecase@chromium.org

[mikecase (-- gone --), 2015-02-25 21:52:55]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2015-02-25 21:52:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjhopman@chromium.org
Link to the patchset: https://codereview.chromium.org/880083004/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-02-25 21:53:52]

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

[commit-bot: I haz the power, 2015-02-26 00:09:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-26 00:10:13]

Patchset 6 (id:??) landed as
https://crrev.com/46a78727a03b9aa652201a41ea12ea1e56790e56
Cr-Commit-Position: refs/heads/master@{#318153}