Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
460453002:
Fixed Compilation issues related to API Level of Android (Closed)
|
Created:
6 years, 4 months ago by AKVT Modified:
6 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFixed Compilation issues related to API Level of Android
Made changes based on new API level to make Lint happy in all
builds.
BUG=327768
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289190
Patch Set 1 #Patch Set 2 : Removed unwanted performClick calls. #
Total comments: 8
Patch Set 3 : Removed SuppressLint for ClickableViewAccessibility. #
Total comments: 7
Patch Set 4 : Corrected commented TargetApi call. #Patch Set 5 : Removed ClickableViewAccessibility related changes to address as separate change. #
Total comments: 2
Patch Set 6 : Added call to quit() for OS version less than JELLY_BEAN_MR2 #
Messages
Total messages: 22 (0 generated)
PTAL
+Yaron and Ted C
PTAL new patchset.
why did you add so many reviewers? You should pick a minimal set based on OWNERS. I'm starting with Aurimas as he's been doing lots of lint-related work recently. Also, if you're fixing these, presumably there's a whitelist where you can now remove them from?
On 2014/08/11 18:06:58, Yaron wrote: > why did you add so many reviewers? You should pick a minimal set based on > OWNERS. I'm starting with Aurimas as he's been doing lots of lint-related work > recently. > > Also, if you're fixing these, presumably there's a whitelist where you can now > remove them from? @Yaron - Since files are more in this, I have added reviewers from top of OWNERS file and some well known reviewers. I will restrict the reviewers in future. Thanks. @aurimas PTAL
https://codereview.chromium.org/460453002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java (right): https://codereview.chromium.org/460453002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java:79: @SuppressLint("ClickableViewAccessibility") This actually does not fix the bug, just adds a suppression. Please remove. https://codereview.chromium.org/460453002/diff/20001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://codereview.chromium.org/460453002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:203: @TargetApi(Build.VERSION_CODES.JELLY_BEAN) What makes this Jelly Bean only method? https://codereview.chromium.org/460453002/diff/20001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/460453002/diff/20001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:158: @SuppressLint("ClickableViewAccessibility") This actually does not fix the bug, just adds a suppression. Please remove. https://codereview.chromium.org/460453002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java (right): https://codereview.chromium.org/460453002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:142: @SuppressLint("ClickableViewAccessibility") This actually does not fix the bug, just adds a suppression. Please remove. https://codereview.chromium.org/460453002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java (right): https://codereview.chromium.org/460453002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:536: @SuppressLint("ClickableViewAccessibility") This actually does not fix the bug, just adds a suppression. Please remove. https://codereview.chromium.org/460453002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/460453002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:103: @SuppressLint("ClickableViewAccessibility") This actually does not fix the bug, just adds a suppression. Please remove.
@aurima PTAL my comment https://codereview.chromium.org/460453002/diff/20001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://codereview.chromium.org/460453002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:203: @TargetApi(Build.VERSION_CODES.JELLY_BEAN) On 2014/08/12 02:47:55, aurimas wrote: > What makes this Jelly Bean only method? CancellationSignal is only available with JELLY_BEAN or higher versions. https://codereview.chromium.org/460453002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/460453002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:103: @SuppressLint("ClickableViewAccessibility") On 2014/08/12 02:47:56, aurimas wrote: > This actually does not fix the bug, just adds a suppression. Please remove. Actual issue was performClick is not getting called from onTouchEvent(). I was added earlier this code to clear of warnings. if (ACTION_UP == event.getAction()) { performClick(); return true; } Later saw many place we added suppress warnings. So I added suppressLint call. Let me know above change is Ok to proceed ?
@aurimas PTAL. ClickableViewAccessibility issues I will take it separately.
https://chromiumcodereview.appspot.com/460453002/diff/40001/chrome/android/ja... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java (right): https://chromiumcodereview.appspot.com/460453002/diff/40001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java:106: } Undo this change. https://chromiumcodereview.appspot.com/460453002/diff/40001/chrome/android/ja... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://chromiumcodereview.appspot.com/460453002/diff/40001/chrome/android/ja... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:203: //@TargetApi(Build.VERSION_CODES.JELLY_BEAN) Huh? Why is this commented out? https://chromiumcodereview.appspot.com/460453002/diff/40001/content/public/an... File content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java (right): https://chromiumcodereview.appspot.com/460453002/diff/40001/content/public/an... content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:537: return super.performClick(); Why do we need this override? It just calls super. Can we do that directly from onTouchEvent?
@aurimas PTAL https://codereview.chromium.org/460453002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java (right): https://codereview.chromium.org/460453002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java:106: } On 2014/08/12 15:59:44, aurimas wrote: > Undo this change. Done. https://codereview.chromium.org/460453002/diff/40001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://codereview.chromium.org/460453002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:203: //@TargetApi(Build.VERSION_CODES.JELLY_BEAN) On 2014/08/12 15:59:44, aurimas wrote: > Huh? Why is this commented out? That was missed out to uncomment while cross checking compilation issue to address earlier review comment :) https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java (right): https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:537: return super.performClick(); On 2014/08/12 15:59:44, aurimas wrote: > Why do we need this override? It just calls super. Can we do that directly from > onTouchEvent? This is to tackle overrides onTouchEvent but not performClick: ClickableViewAccessibility [warning] issue.
https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java (right): https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:544: performClick(); I guess I did not explain it well. Why don't we call super.performClick() here directly instead of creating an override method with only super call in it?
On 2014/08/12 16:30:21, aurimas wrote: > https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java > (right): > > https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:544: > performClick(); > I guess I did not explain it well. Why don't we call super.performClick() here > directly instead of creating an override method with only super call in it? calling super.performClick() is still not solving the issue. Lint throws messages as follows; Custom view org/chromium/content/browser/PopupZoomer overrides onTouchEvent but not performClick: ClickableViewAccessibility [warning]
On 2014/08/12 16:33:34, AKV wrote: > On 2014/08/12 16:30:21, aurimas wrote: > > > https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... > > File > > content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java > > (right): > > > > > https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... > > > content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:544: > > performClick(); > > I guess I did not explain it well. Why don't we call super.performClick() here > > directly instead of creating an override method with only super call in it? > > calling super.performClick() is still not solving the issue. > Lint throws messages as follows; > > Custom view org/chromium/content/browser/PopupZoomer overrides onTouchEvent but > not performClick: ClickableViewAccessibility [warning] Let's not add this change then in this CL. It seems like this needs more investigation of what the proper fix for ClickableViewAccessibility should be.
On 2014/08/12 16:35:10, aurimas wrote: > On 2014/08/12 16:33:34, AKV wrote: > > On 2014/08/12 16:30:21, aurimas wrote: > > > > > > https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... > > > File > > > > content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java > > > (right): > > > > > > > > > https://codereview.chromium.org/460453002/diff/40001/content/public/android/j... > > > > > > content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:544: > > > performClick(); > > > I guess I did not explain it well. Why don't we call super.performClick() > here > > > directly instead of creating an override method with only super call in it? > > > > calling super.performClick() is still not solving the issue. > > Lint throws messages as follows; > > > > Custom view org/chromium/content/browser/PopupZoomer overrides onTouchEvent > but > > not performClick: ClickableViewAccessibility [warning] > > Let's not add this change then in this CL. It seems like this needs more > investigation of what the proper fix for ClickableViewAccessibility should be. @aurimas PTAL. Removed ClickableViewAccessibility changes to check separately.
lgtm
https://codereview.chromium.org/460453002/diff/80001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/SmartClipProviderTest.java (right): https://codereview.chromium.org/460453002/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/SmartClipProviderTest.java:109: } Should it just call quit on older versions? I don't know and haven't tested. Does the test end up blocking on old versions?
@Yaron PTAL. https://codereview.chromium.org/460453002/diff/80001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/SmartClipProviderTest.java (right): https://codereview.chromium.org/460453002/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/SmartClipProviderTest.java:109: } On 2014/08/12 16:44:09, Yaron wrote: > Should it just call quit on older versions? I don't know and haven't tested. > Does the test end up blocking on old versions? Corrected by using mHandlerThread.quit() for lower versions. Thanks.
lgtm (I assume test a pre-MR2 device, right?)
On 2014/08/12 17:49:50, Yaron wrote: > lgtm > > (I assume test a pre-MR2 device, right?) I had done in KK device only, due to device unavailability here. HandlerThread.quit() is API level 5, so we can trust this API.
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/460453002/100001
Message was sent while issue was closed.
Change committed as 289190 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
