Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
552813002:
[android_webview] Use a MotionEvent to trigger ShouldInterceptRequest tests. (Closed)
|
Created:
6 years, 3 months ago by mkosiba (inactive) Modified:
6 years, 3 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org, boliu, Zeeshan Qureshi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[android_webview] Use a MotionEvent to trigger ShouldInterceptRequest tests.
This updates the test case to use a synthesized single tap gesture
to follow the link instead of JavaScript.
BUG=408426
TEST=AndroidWebViewTest
Committed: https://crrev.com/ad25c603b9aa3ac3175468721a924c903754f4e7
Cr-Commit-Position: refs/heads/master@{#293926}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : fix findbugs #Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 26 (9 generated)
mkosiba@chromium.org changed reviewers: + benm@chromium.org
PTAL
https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/util/AwTestTouchUtils.java (right): https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/util/AwTestTouchUtils.java:81: * Performs a single ttouch on the center of the supplied view. nit: touch https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/util/AwTestTouchUtils.java:98: eventTime, eventTime, MotionEvent.ACTION_UP, I guess it's ok that ACTION_UP time == ACTION_DOWN time?
boliu@chromium.org changed reviewers: + boliu@chromium.org
https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java (right): https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java:254: return bitmap.getPixel(0, 0) == Color.BLUE; That looks *super* brittle. What if text rendering changes and you didn't hit a blue pixel? What if the link highlight color changes? How about using a properly sized bitmap, and just check it's not empty?
https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java (right): https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java:254: return bitmap.getPixel(0, 0) == Color.BLUE; On 2014/09/08 16:02:55, boliu wrote: > That looks *super* brittle. What if text rendering changes and you didn't hit a > blue pixel? What if the link highlight color changes? > > How about using a properly sized bitmap, and just check it's not empty? This is not checking the text to be blue. The "link" is a 100%-by-100% clickable blue div (run the test) so it's less brittle than you think. https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/util/AwTestTouchUtils.java (right): https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/util/AwTestTouchUtils.java:81: * Performs a single ttouch on the center of the supplied view. On 2014/09/08 16:00:18, benm wrote: > nit: touch Done. https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/util/AwTestTouchUtils.java:98: eventTime, eventTime, MotionEvent.ACTION_UP, On 2014/09/08 16:00:18, benm wrote: > I guess it's ok that ACTION_UP time == ACTION_DOWN time? seems to work fine, yeath. IIRC we only use time to figure out velocities.
lgtm
lgtm https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java (right): https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java:254: return bitmap.getPixel(0, 0) == Color.BLUE; On 2014/09/08 16:54:36, mkosiba wrote: > On 2014/09/08 16:02:55, boliu wrote: > > That looks *super* brittle. What if text rendering changes and you didn't hit > a > > blue pixel? What if the link highlight color changes? > > > > How about using a properly sized bitmap, and just check it's not empty? > > This is not checking the text to be blue. The "link" is a 100%-by-100% clickable > blue div (run the test) so it's less brittle than you think. Meh, badly named function then. side note, why embed an <img> tag that's blue rather than making the <a> tag itself blue? some test probably depend on it, but just weird...
On 2014/09/08 17:02:11, boliu wrote: > lgtm > > https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... > File > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java > (right): > > https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java:254: > return bitmap.getPixel(0, 0) == Color.BLUE; > On 2014/09/08 16:54:36, mkosiba wrote: > > On 2014/09/08 16:02:55, boliu wrote: > > > That looks *super* brittle. What if text rendering changes and you didn't > hit > > a > > > blue pixel? What if the link highlight color changes? > > > > > > How about using a properly sized bitmap, and just check it's not empty? > > > > This is not checking the text to be blue. The "link" is a 100%-by-100% > clickable > > blue div (run the test) so it's less brittle than you think. > > Meh, badly named function then. > > side note, why embed an <img> tag that's blue rather than making the <a> tag > itself blue? some test probably depend on it, but just weird... It might be because I couldn't get the <a> tag to size itself correctly. I think the problem was that you needed something inside, be it a div or img or whatever.
The CQ bit was checked by mkosiba@chromium.org
On 2014/09/08 17:26:48, mkosiba wrote: > On 2014/09/08 17:02:11, boliu wrote: > > lgtm > > > > > https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... > > File > > > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java > > (right): > > > > > https://codereview.chromium.org/552813002/diff/1/android_webview/javatests/sr... > > > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java:254: > > return bitmap.getPixel(0, 0) == Color.BLUE; > > On 2014/09/08 16:54:36, mkosiba wrote: > > > On 2014/09/08 16:02:55, boliu wrote: > > > > That looks *super* brittle. What if text rendering changes and you didn't > > hit > > > a > > > > blue pixel? What if the link highlight color changes? > > > > > > > > How about using a properly sized bitmap, and just check it's not empty? > > > > > > This is not checking the text to be blue. The "link" is a 100%-by-100% > > clickable > > > blue div (run the test) so it's less brittle than you think. > > > > Meh, badly named function then. > > > > side note, why embed an <img> tag that's blue rather than making the <a> tag > > itself blue? some test probably depend on it, but just weird... > > It might be because I couldn't get the <a> tag to size itself correctly. I think > the problem was that you needed something inside, be it a div or img or > whatever. yup, try: data:text/html,<a href="#" style="background-color: red; width:100%; height:100%">haha!</a>
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkosiba@chromium.org/552813002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by mkosiba@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkosiba@chromium.org/552813002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by mkosiba@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkosiba@chromium.org/552813002/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 (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by mkosiba@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkosiba@chromium.org/552813002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as b511feb1d775e3b1f657dfd1f9ad625b7d2f65b7
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ad25c603b9aa3ac3175468721a924c903754f4e7 Cr-Commit-Position: refs/heads/master@{#293926} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
