Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1397083003:
Remove never_lint from WebView (Closed)
|
Created:
5 years, 2 months ago by michaelbai Modified:
5 years, 2 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove never_lint from WebView
BUG=532593, 542435
Committed: https://crrev.com/03f9f9f31789469456c615c65d4a02626283bf94
Cr-Commit-Position: refs/heads/master@{#353956}
Patch Set 1 #Patch Set 2 : suppress resouces #
Total comments: 9
Patch Set 3 : address comment #
Total comments: 4
Patch Set 4 : #Patch Set 5 : back to path #
Messages
Total messages: 24 (6 generated)
michaelbai@chromium.org changed reviewers: + newt@chromium.org, sgurun@chromium.org
On 2015/10/13 19:31:07, michaelbai wrote: lgtm
newt@ for build/android/lint
Thanks for enabling this! Some comment inline. https://codereview.chromium.org/1397083003/diff/20001/android_webview/apk/sys... File android_webview/apk/system_webview_apk_common.gypi (left): https://codereview.chromium.org/1397083003/diff/20001/android_webview/apk/sys... android_webview/apk/system_webview_apk_common.gypi:15: 'never_lint': 1, Do you need to make a change to GN too? (or looks like there's no GN rules for android_webview?) https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:81: @SuppressLint("NewApi") What NewApi warnings are you suppressing? NewApi is one of the most helpful lint warnings, so a blanket suppression of all NewApi warnings in a huge class is far from ideal. Here are several better approaches: 1. Use @TargetApi as explained in my comment in WebViewDelegateFactory.java, and/or 2. Apply @TargetApi locally to the methods that call the new APIs instead of to the entire class, and/or 3. Extract the code that uses the new APIs into an inner class and add @TargetApi to that inner class. https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:83: @SuppressLint("NewApi") same deal here https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java (right): https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java:201: @SuppressLint("NewApi") Instead of @SuppressLint("NewApi"), use @TargetApi(Build.VERSION_CODES.LOLLIPOP), which signals to the lint tool that it's OK to use any methods introduced in API 21 or earlier. Lint will still complain if you try to use methods introduced *after* API 21, though, which is helpful. https://codereview.chromium.org/1397083003/diff/20001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1397083003/diff/20001/build/android/lint/supp... build/android/lint/suppressions.xml:46: <ignore path="android_webview/apk/java/res" /> Instead of suppressing density checks for the entire folder, you can just suppress them for the individual file, using something like this <ignore regexp=".*: icon_webview.png$"/> See line 58 for an example.
Thanks, PTAL https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:81: @SuppressLint("NewApi") On 2015/10/13 20:41:38, newt wrote: > What NewApi warnings are you suppressing? NewApi is one of the most helpful lint > warnings, so a blanket suppression of all NewApi warnings in a huge class is far > from ideal. Here are several better approaches: > 1. Use @TargetApi as explained in my comment in WebViewDelegateFactory.java, > and/or > 2. Apply @TargetApi locally to the methods that call the new APIs instead of to > the entire class, and/or > 3. Extract the code that uses the new APIs into an inner class and add > @TargetApi to that inner class. Done. https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:83: @SuppressLint("NewApi") On 2015/10/13 20:41:38, newt wrote: > same deal here Changed to use TargetApi, but it is a little bit over-skill to make it more specific, furthermore, we already check the build version before using M APIs. https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java (right): https://codereview.chromium.org/1397083003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java:201: @SuppressLint("NewApi") On 2015/10/13 20:41:38, newt wrote: > Instead of @SuppressLint("NewApi"), use > @TargetApi(Build.VERSION_CODES.LOLLIPOP), which signals to the lint tool that > it's OK to use any methods introduced in API 21 or earlier. Lint will still > complain if you try to use methods introduced *after* API 21, though, which is > helpful. Done. https://codereview.chromium.org/1397083003/diff/20001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1397083003/diff/20001/build/android/lint/supp... build/android/lint/suppressions.xml:46: <ignore path="android_webview/apk/java/res" /> On 2015/10/13 20:41:38, newt wrote: > Instead of suppressing density checks for the entire folder, you can just > suppress them for the individual file, using something like this > > <ignore regexp=".*: icon_webview.png$"/> > > See line 58 for an example. Done.
https://codereview.chromium.org/1397083003/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/1397083003/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:84: @TargetApi(Build.VERSION_CODES.LOLLIPOP) Why is it overkill to use @TargetApi on individual methods? If this class is only used on Android L+, then this is fine. Otherwise, I don't see any version checks in this class, so how are you ensuring that this doesn't crash on pre-L devices? https://codereview.chromium.org/1397083003/diff/40001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1397083003/diff/40001/build/android/lint/supp... build/android/lint/suppressions.xml:46: <ignore regexp="ic_play_circle_outline_black_48dp.png" /> Make sure this regex won't cause other assets with missing densities to be ignored. In particular you may need to add ": " at the beginning and "$" at the end of the regex.
PTAL https://codereview.chromium.org/1397083003/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/1397083003/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:84: @TargetApi(Build.VERSION_CODES.LOLLIPOP) On 2015/10/13 21:50:57, newt wrote: > Why is it overkill to use @TargetApi on individual methods? If this class is > only used on Android L+, then this is fine. Otherwise, I don't see any version > checks in this class, so how are you ensuring that this doesn't crash on pre-L > devices? Yes, Webview only update down to L. https://codereview.chromium.org/1397083003/diff/40001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1397083003/diff/40001/build/android/lint/supp... build/android/lint/suppressions.xml:46: <ignore regexp="ic_play_circle_outline_black_48dp.png" /> On 2015/10/13 21:50:57, newt wrote: > Make sure this regex won't cause other assets with missing densities to be > ignored. In particular you may need to add ": " at the beginning and "$" at the > end of the regex. Done.
Sounds good. lgtm!
The CQ bit was checked by michaelbai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org Link to the patchset: https://codereview.chromium.org/1397083003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1397083003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1397083003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/10/13 23:09:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) newt@ it seemed specified file name not work, is it because warning "IconMissingDensityFolder" complained about folder? any suggestion?
On 2015/10/14 00:28:18, michaelbai wrote: > On 2015/10/13 23:09:57, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > newt@ it seemed specified file name not work, is it because warning > "IconMissingDensityFolder" complained about folder? any suggestion? Could you copy-paste the output from the lint error?
On 2015/10/14 00:34:47, newt wrote: > On 2015/10/14 00:28:18, michaelbai wrote: > > On 2015/10/13 23:09:57, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > newt@ it seemed specified file name not work, is it because warning > > "IconMissingDensityFolder" complained about folder? any suggestion? > > Could you copy-paste the output from the lint error? This is from bot FAILED: cd ../../android_webview; python ../build/android/gyp/lint.py "--lint-path=/b/build/slave/android_clang_dbg_recipe/build/src/third_party/android_tools/sdk//tools/lint" "--config-path=../build/android/lint/suppressions.xml" "--processed-config-path=../out/Debug/system_webview_apk/lint_config.xml" "--manifest-path=../out/Debug/obj/android_webview/system_webview_apk.gen/AndroidManifest.xml" "--result-path=../out/Debug/system_webview_apk/lint_result.xml" "--resource-dir=apk/java/res" "--product-dir=../out/Debug" "--src-dirs=../build/android/empty/src" "--jar-path=../out/Debug/lib.java/chromium_apk_system_webview_apk.jar" --can-fail-build "--stamp=../out/Debug/system_webview_apk/lint.stamp" --enable android_webview/apk/java/res Missing density variation folders in `android_webview/apk/java/res`: drawable-xxxhdpi: IconMissingDensityFolder [warning] Lint found 1 new issues. - For full explanation refer to out/Debug/system_webview_apk/lint_result.xml - Wanna suppress these issues? 1. Read comment in build/android/lint/suppressions.xml 2. Run "python build/android/lint/suppress.py out/Debug/system_webview_apk/lint_result.xml" ninja: build stopped: subcommand failed.
On 2015/10/14 01:08:19, michaelbai wrote: > On 2015/10/14 00:34:47, newt wrote: > > On 2015/10/14 00:28:18, michaelbai wrote: > > > On 2015/10/13 23:09:57, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > newt@ it seemed specified file name not work, is it because warning > > > "IconMissingDensityFolder" complained about folder? any suggestion? > > > > Could you copy-paste the output from the lint error? > > This is from bot > > > FAILED: cd ../../android_webview; python ../build/android/gyp/lint.py > "--lint-path=/b/build/slave/android_clang_dbg_recipe/build/src/third_party/android_tools/sdk//tools/lint" > "--config-path=../build/android/lint/suppressions.xml" > "--processed-config-path=../out/Debug/system_webview_apk/lint_config.xml" > "--manifest-path=../out/Debug/obj/android_webview/system_webview_apk.gen/AndroidManifest.xml" > "--result-path=../out/Debug/system_webview_apk/lint_result.xml" > "--resource-dir=apk/java/res" "--product-dir=../out/Debug" > "--src-dirs=../build/android/empty/src" > "--jar-path=../out/Debug/lib.java/chromium_apk_system_webview_apk.jar" > --can-fail-build "--stamp=../out/Debug/system_webview_apk/lint.stamp" --enable > > android_webview/apk/java/res Missing density variation folders in > `android_webview/apk/java/res`: drawable-xxxhdpi: IconMissingDensityFolder > [warning] > > Lint found 1 new issues. > - For full explanation refer to out/Debug/system_webview_apk/lint_result.xml > - Wanna suppress these issues? > 1. Read comment in build/android/lint/suppressions.xml > 2. Run "python build/android/lint/suppress.py > out/Debug/system_webview_apk/lint_result.xml" > > ninja: build stopped: subcommand failed. Ah, sorry. I didn't realize the error didn't even list the filename. In that case, your best option is to use <ignore path="..."> like you were doing in patch set 2. The regexp method would be a better option *if* the error message actually listed the offending filenames (since the regex is matched against the error message), but the error doesn't list the filenames, unfortunately.
On 2015/10/14 01:11:41, newt wrote: > On 2015/10/14 01:08:19, michaelbai wrote: > > On 2015/10/14 00:34:47, newt wrote: > > > On 2015/10/14 00:28:18, michaelbai wrote: > > > > On 2015/10/13 23:09:57, commit-bot: I haz the power wrote: > > > > > Try jobs failed on following builders: > > > > > android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) > > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > > > newt@ it seemed specified file name not work, is it because warning > > > > "IconMissingDensityFolder" complained about folder? any suggestion? > > > > > > Could you copy-paste the output from the lint error? > > > > This is from bot > > > > > > FAILED: cd ../../android_webview; python ../build/android/gyp/lint.py > > > "--lint-path=/b/build/slave/android_clang_dbg_recipe/build/src/third_party/android_tools/sdk//tools/lint" > > "--config-path=../build/android/lint/suppressions.xml" > > "--processed-config-path=../out/Debug/system_webview_apk/lint_config.xml" > > > "--manifest-path=../out/Debug/obj/android_webview/system_webview_apk.gen/AndroidManifest.xml" > > "--result-path=../out/Debug/system_webview_apk/lint_result.xml" > > "--resource-dir=apk/java/res" "--product-dir=../out/Debug" > > "--src-dirs=../build/android/empty/src" > > "--jar-path=../out/Debug/lib.java/chromium_apk_system_webview_apk.jar" > > --can-fail-build "--stamp=../out/Debug/system_webview_apk/lint.stamp" --enable > > > > android_webview/apk/java/res Missing density variation folders in > > `android_webview/apk/java/res`: drawable-xxxhdpi: IconMissingDensityFolder > > [warning] > > > > Lint found 1 new issues. > > - For full explanation refer to out/Debug/system_webview_apk/lint_result.xml > > - Wanna suppress these issues? > > 1. Read comment in build/android/lint/suppressions.xml > > 2. Run "python build/android/lint/suppress.py > > out/Debug/system_webview_apk/lint_result.xml" > > > > ninja: build stopped: subcommand failed. > > Ah, sorry. I didn't realize the error didn't even list the filename. In that > case, your best option is to use <ignore path="..."> like you were doing in > patch set 2. The regexp method would be a better option *if* the error message > actually listed the offending filenames (since the regex is matched against the > error message), but the error doesn't list the filenames, unfortunately. So, lgtm after changing the lint suppress to use <ignore path="...">
The CQ bit was checked by michaelbai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1397083003/#ps80001 (title: "back to path")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1397083003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1397083003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/03f9f9f31789469456c615c65d4a02626283bf94 Cr-Commit-Position: refs/heads/master@{#353956} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
