Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
475633002:
Pass TouchMajor to HitTestResult (Closed)
|
Created:
6 years, 4 months ago by hush (inactive) Modified:
6 years, 1 month ago CC:
android-webview-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, jdduke+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPass TouchMajor to HitTestResult
Blink side change to do hit-test that mimics GestureTap:
https://codereview.chromium.org/470833002
Without the touch major information, HitTestResult may not
match how touch events are handled in content view core. As
a result, it is possible that the user touches a link while
the hitTestResult returns unknown type.
BUG=403865
Committed: https://crrev.com/1d93adc3addd7b336d541f77eb272c2aa43a7053
Cr-Commit-Position: refs/heads/master@{#304067}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : comments #Patch Set 3 : #Patch Set 4 : Use blink::WebView::hitTestResultForTap #
Total comments: 6
Patch Set 5 : review #
Total comments: 7
Patch Set 6 : reviews #
Total comments: 4
Patch Set 7 : review #Patch Set 8 : rename variables #
Total comments: 3
Patch Set 9 : use floats #
Total comments: 2
Patch Set 10 : use hitTestResultAt in RenderViewImpl::NodeContainsPoint #
Total comments: 4
Patch Set 11 : Use const ref #Messages
Total messages: 52 (10 generated)
https://codereview.chromium.org/475633002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2298: (pointerCount > 1 ? event.getToolMajor(1) : 0f) / (float) mDIPScale); Shouldn't this be getToolMinor rather than getting it for the second touch point? https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... android_webview/renderer/aw_render_view_ext.cc:281: int radius = 0.5f * std::max(touch_major_0, touch_major_1); hmm, should we do max or min here...?
https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... android_webview/renderer/aw_render_view_ext.cc:281: int radius = 0.5f * std::max(touch_major_0, touch_major_1); Sorry for the drive-by, but why put this max logic in the renderer? Also, this doesn't follow what WebInputEventUtil::CreateWebTouchPoint is doing, as that method only uses the touch_major value for a single point. Why not pass in a rect, or the (x,y) and (width,height)? It also seems a bit odd to pass in multiple touch major values; shouldn't we pass the value corresponding to the pointer at (|view_x|, |view_y|)?
https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... android_webview/renderer/aw_render_view_ext.cc:281: int radius = 0.5f * std::max(touch_major_0, touch_major_1); Sorry. I'm probably just doing a random thing here. If there are 2 fingers touching the screen, HitTestResult behaviour will be pretty much undefined right? Like what if one finger is touching a link and the other is touching an image, what is hitTestResult supposed to return? On 2014/08/14 16:36:53, boliu wrote: > hmm, should we do max or min here...?
https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... android_webview/renderer/aw_render_view_ext.cc:281: int radius = 0.5f * std::max(touch_major_0, touch_major_1); On 2014/08/14 17:03:32, hush wrote: > Sorry. I'm probably just doing a random thing here. If there are 2 fingers > touching the screen, HitTestResult behaviour will be pretty much undefined > right? Use the first point here. > Like what if one finger is touching a link and the other is touching an > image, what is hitTestResult supposed to return? > > On 2014/08/14 16:36:53, boliu wrote: > > hmm, should we do max or min here...? >
https://codereview.chromium.org/475633002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2295: (int) Math.round(event.getX(actionIndex) / mDIPScale), Bo, I think this "actionIndex" is wrong here. Should be just 0, or just call event.getX(), because event.getX() is expecting a pointerIndex here, which is not actionIndex. https://codereview.chromium.org/475633002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2298: (pointerCount > 1 ? event.getToolMajor(1) : 0f) / (float) mDIPScale); I think I should just remove this parameter and just use "getToolMajor(0)". I took a look at what ContentViewCore is doing here: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and... ContentViewCore is using ToolMajor() for each touch point, and not using ToolMinor at all. For webview, we just need to use the first touch point. Jared, what do you think? On 2014/08/14 16:36:53, boliu wrote: > Shouldn't this be getToolMinor rather than getting it for the second touch > point? https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/40001/android_webview/renderer... android_webview/renderer/aw_render_view_ext.cc:281: int radius = 0.5f * std::max(touch_major_0, touch_major_1); Thanks Jared for the comments! By "following what WebInputEventUtil::CreateWebTouchPoint" is doing, I just meant taking half of radius and creating a blink touch event. Right, it is odd to pass in multiple touch major values. I will just use the first touch major value as Bo suggested. Android WebView's getHitTestResult API probably assumes there is only one touch major anyway. On 2014/08/14 17:01:57, jdduke wrote: > Sorry for the drive-by, but why put this max logic in the renderer? Also, this > doesn't follow what WebInputEventUtil::CreateWebTouchPoint is doing, as that > method only uses the touch_major value for a single point. > > Why not pass in a rect, or the (x,y) and (width,height)? It also seems a bit > odd to pass in multiple touch major values; shouldn't we pass the value > corresponding to the pointer at (|view_x|, |view_y|)?
https://codereview.chromium.org/475633002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2297: event.getToolMajor(0) / (float) mDIPScale, Just talked offline with Bo, didn't realize this was only called after ACTION_DOWN. ACTION_DOWN will only ever have 1 pointer, that's guaranteed. so you can just call |getTouchMajor()|.
https://codereview.chromium.org/475633002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2295: (int) Math.round(event.getX(actionIndex) / mDIPScale), Sorry... this is correct. Nevermind On 2014/08/14 17:25:21, hush wrote: > Bo, I think this "actionIndex" is wrong here. Should be just 0, or just call > event.getX(), because event.getX() is expecting a pointerIndex here, which is > not actionIndex.
https://codereview.chromium.org/475633002/diff/100001/android_webview/rendere... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/100001/android_webview/rendere... android_webview/renderer/aw_render_view_ext.cc:277: render_view()->GetWebView()->hitTestResultForTap( I only changed here in PS4. Others are just rebase. The blink CL will land first, then this one, then another CL to remove hitTestResultAt from blink::WebView API.
https://codereview.chromium.org/475633002/diff/100001/android_webview/rendere... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/100001/android_webview/rendere... android_webview/renderer/aw_render_view_ext.cc:275: const blink::WebSize touch_area(radius, radius); We shouldn't be using a radius here. Radii are only used with touch events because that's how their members are exposed to the web; tap/press events use a regular bounding box (width/height) for hit testing.
https://codereview.chromium.org/475633002/diff/100001/android_webview/browser... File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right): https://codereview.chromium.org/475633002/diff/100001/android_webview/browser... android_webview/browser/renderer_host/aw_render_view_host_ext.h:58: void RequestNewHitTestDataAt(int view_x, int view_y, float touch_major); Could we make this more generic and pass a RectF?
I'll stamp when Jared is happy.
https://codereview.chromium.org/475633002/diff/100001/android_webview/browser... File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right): https://codereview.chromium.org/475633002/diff/100001/android_webview/browser... android_webview/browser/renderer_host/aw_render_view_host_ext.h:58: void RequestNewHitTestDataAt(int view_x, int view_y, float touch_major); On 2014/10/28 23:09:14, jdduke wrote: > Could we make this more generic and pass a RectF? Actually, if this is WebView only I don't really care about the API as long as it's clear and consistent. Either a rect, or a coordinate and width/height should be fine.
https://codereview.chromium.org/475633002/diff/100001/android_webview/browser... File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right): https://codereview.chromium.org/475633002/diff/100001/android_webview/browser... android_webview/browser/renderer_host/aw_render_view_host_ext.h:58: void RequestNewHitTestDataAt(int view_x, int view_y, float touch_major); On 2014/10/29 02:15:17, jdduke wrote: > On 2014/10/28 23:09:14, jdduke wrote: > > Could we make this more generic and pass a RectF? > > Actually, if this is WebView only I don't really care about the API as long as > it's clear and consistent. Either a rect, or a coordinate and width/height > should be fine. I will pass gfx::RectF like you suggested. https://codereview.chromium.org/475633002/diff/100001/android_webview/rendere... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/100001/android_webview/rendere... android_webview/renderer/aw_render_view_ext.cc:275: const blink::WebSize touch_area(radius, radius); On 2014/10/28 23:08:04, jdduke wrote: > We shouldn't be using a radius here. Radii are only used with touch events > because that's how their members are exposed to the web; tap/press events use a > regular bounding box (width/height) for hit testing. Instead of passing touch_major, I will pass both the touch major and touch minor. (both included in a RectF) And use half of touch major as the width and half of touch minor as the height.
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/475633002/diff/160001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/160001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2352: float touchMinor = event.getToolMinor(actionIndex) / (float) mDIPScale; I wouldn't use touchMinor here, as then you'd also need to use the touch orientation. Major/minor don't necessarily correspond to X/Y axes, so it should be sufficient to just use touchMajor. https://codereview.chromium.org/475633002/diff/160001/android_webview/native/... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/475633002/diff/160001/android_webview/native/... android_webview/native/aw_contents.cc:819: gfx::RectF touch_area(x, y, touch_major / 2, touch_minor / 2); x/y here correspond to the *center* of the rect, but you supply them as the rect origin. Also, touch_major *is* the size of the area, so I'm still unsure why you're dividing it in half... https://codereview.chromium.org/475633002/diff/160001/android_webview/rendere... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/160001/android_webview/rendere... android_webview/renderer/aw_render_view_ext.cc:274: blink::WebPoint(touch_area.x(), touch_area.y()), Ugg, this is misleading. The x/y coordinate of your box are at the top left of the box, but Blink expects the center coordinate. Please just mirror the Blink API to prevent any confusion (i.e., use a center point gfx::PointF, and a size, gfx::SizeF).
hush@chromium.org changed reviewers: + rbyers@chromium.org
+Rick for confirmation of EventHandler::hitTestResultAtPoint "padding" param's meaning. https://codereview.chromium.org/475633002/diff/160001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/160001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2352: float touchMinor = event.getToolMinor(actionIndex) / (float) mDIPScale; On 2014/10/30 19:54:58, jdduke wrote: > I wouldn't use touchMinor here, as then you'd also need to use the touch > orientation. Major/minor don't necessarily correspond to X/Y axes, so it should > be sufficient to just use touchMajor. Okay. In this case, I am basically treating the touch area as a square, whose width/height is touchMajor. https://codereview.chromium.org/475633002/diff/160001/android_webview/native/... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/475633002/diff/160001/android_webview/native/... android_webview/native/aw_contents.cc:819: gfx::RectF touch_area(x, y, touch_major / 2, touch_minor / 2); On 2014/10/30 19:54:58, jdduke wrote: > x/y here correspond to the *center* of the rect, but you supply them as the rect > origin. Also, touch_major *is* the size of the area, so I'm still unsure why > you're dividing it in half... I agree this is weird. (you mentioned this in a comment in AwRenderViewExt as well). I will pass the center point and the padding separately in the next patch. > I'm still unsure why you're dividing it in half The "padding" parameter of the function EventHandler::hitTestResultAtPoint is not the width and height of the touch area, it is half of that. I sort of guessed it by looking at these 2 places: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... and: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... So padding.width() and padding.height() is how far the center point is from the left/right and top/bottom. Rick, please correct me if I said it wrong. https://codereview.chromium.org/475633002/diff/160001/android_webview/rendere... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/160001/android_webview/rendere... android_webview/renderer/aw_render_view_ext.cc:274: blink::WebPoint(touch_area.x(), touch_area.y()), On 2014/10/30 19:54:58, jdduke wrote: > Ugg, this is misleading. The x/y coordinate of your box are at the top left of > the box, but Blink expects the center coordinate. Please just mirror the Blink > API to prevent any confusion (i.e., use a center point gfx::PointF, and a size, > gfx::SizeF). Yes, misleading. I will pass the point and padding separately.
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/475633002/diff/160001/android_webview/native/... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/475633002/diff/160001/android_webview/native/... android_webview/native/aw_contents.cc:819: gfx::RectF touch_area(x, y, touch_major / 2, touch_minor / 2); On 2014/11/04 01:55:57, hush wrote: > On 2014/10/30 19:54:58, jdduke wrote: > > x/y here correspond to the *center* of the rect, but you supply them as the > rect > > origin. Also, touch_major *is* the size of the area, so I'm still unsure why > > you're dividing it in half... > > I agree this is weird. (you mentioned this in a comment in AwRenderViewExt as > well). I will pass the center point and the padding separately in the next > patch. > > > I'm still unsure why you're dividing it in half > The "padding" parameter of the function EventHandler::hitTestResultAtPoint is > not the width and height of the touch area, it is half of that. > I sort of guessed it by looking at these 2 places: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > and: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > So padding.width() and padding.height() is how far the center point is from the > left/right and top/bottom. > > Rick, please correct me if I said it wrong. Sorry. Scratch that. Blink does the half resize for us in this case. https://codereview.chromium.org/470833002/diff/40001/Source/core/page/EventHa...
Patchset #6 (id:200001) has been deleted
Thanks, sorry for the earlier confusion. https://codereview.chromium.org/475633002/diff/220001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/220001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2348: int actionIndex = event.getActionIndex(); Nit: |getActionIndex()| is really only intended for use with ACTION_POINTER_{UP,DOWN}. I would remove it, and simply use: event.getX(), event.getY() and event.getToolMajor(). I'm also not sure what the effective difference is between getToolMajor and getTouchMajor, but I guess as long as this works for all of the relevant inputs. https://codereview.chromium.org/475633002/diff/220001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2353: (int) Math.round(event.getX(actionIndex) / mDIPScale), Nit: Let's be consistent with fractional coordinate usage on the host side. I would send a PointF/SizeF, and do the truncation as late as possible (in this case I guess that's at the Blink API boundary?).
https://codereview.chromium.org/475633002/diff/220001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/220001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2348: int actionIndex = event.getActionIndex(); On 2014/11/04 16:37:29, jdduke wrote: > Nit: > |getActionIndex()| is really only intended for use with > ACTION_POINTER_{UP,DOWN}. I would remove it, and simply use: > > event.getX(), event.getY() and event.getToolMajor(). > > I'm also not sure what the effective difference is between getToolMajor and > getTouchMajor, but I guess as long as this works for all of the relevant inputs. I will use getTouchMajor then. (ContentViewCore is using that as well.) And I don't find anywhere else interesting that uses getToolMajor. https://codereview.chromium.org/475633002/diff/220001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2353: (int) Math.round(event.getX(actionIndex) / mDIPScale), On 2014/11/04 16:37:30, jdduke wrote: > Nit: Let's be consistent with fractional coordinate usage on the host side. I > would send a PointF/SizeF, and do the truncation as late as possible (in this > case I guess that's at the Blink API boundary?). Done.
Also removed content/renderer/render_view_impl.cc usage of hitTestResultAt, and switched it to the new blink api.
Thanks. I'm not an owner here, but the general changes lgtm with one suggestion. https://codereview.chromium.org/475633002/diff/260001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2351: (int) Math.round(event.getX() / mDIPScale), Can we push the rounding/truncation as far down the pipeline as possible (just before handing to Blink?). That should make it simpler if/when we allow fractional hit testing coordinates at the Blink boundary.
Looks like blink side has not landed yet.. https://codereview.chromium.org/475633002/diff/260001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2351: (int) Math.round(event.getX() / mDIPScale), On 2014/11/07 16:09:06, jdduke wrote: > Can we push the rounding/truncation as far down the pipeline as possible (just > before handing to Blink?). That should make it simpler if/when we allow > fractional hit testing coordinates at the Blink boundary. Agreed. You've already updated nativeRequestNewHitTestDataAt to take floats, so why pass it ints?
https://codereview.chromium.org/475633002/diff/260001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/475633002/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2351: (int) Math.round(event.getX() / mDIPScale), On 2014/11/07 16:30:19, boliu wrote: > On 2014/11/07 16:09:06, jdduke wrote: > > Can we push the rounding/truncation as far down the pipeline as possible (just > > before handing to Blink?). That should make it simpler if/when we allow > > fractional hit testing coordinates at the Blink boundary. > > Agreed. You've already updated nativeRequestNewHitTestDataAt to take floats, so > why pass it ints? my bad, I missed this bit. Yes. I should've removed the int conversions.
hush@chromium.org changed reviewers: + jamesr@chromium.org
+jamesr for content/renderer/ The change in content/renderer is about using a new API in blink defined here: https://codereview.chromium.org/470833002/
android_webview lgtm
https://codereview.chromium.org/475633002/diff/280001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/475633002/diff/280001/content/renderer/render... content/renderer/render_view_impl.cc:2553: webview()->hitTestResultForTap(WebPoint(point.x(), point.y()), WebSize()); this is used for autofill and exposed in the content public API so may be used for other things. is this a behavior change?
Conceptually the RenderView check doesn't have anything to do with tapping, so it's not clear why you would want to call a tap function for it.
On 2014/11/08 00:21:37, jamesr wrote: > Conceptually the RenderView check doesn't have anything to do with tapping, so > it's not clear why you would want to call a tap function for it. I want to remove hitTestResultAt from the blink webview API and only use hitTestResultForTap instead. (If Rick agrees to do so) So hitTestResultForTap is just blink's only API for doing hit tests. So it is not really a tap function. It is instead a function that does hit tests around an area. The "ForTap" part is only suggesting that the API will take an area around the center point.
https://codereview.chromium.org/475633002/diff/280001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/475633002/diff/280001/content/renderer/render... content/renderer/render_view_impl.cc:2553: webview()->hitTestResultForTap(WebPoint(point.x(), point.y()), WebSize()); On 2014/11/08 00:21:08, jamesr wrote: > this is used for autofill and exposed in the content public API so may be used > for other things. is this a behavior change? This is not a behavior change. hitTestResultAt does single point hit testing. hitTestResultForTap does rect-based hit testing. But I'm passing a 0x0 tap area here. So it is the same as single point hit testing.
Seems like a bad API name if it's not intended to be for tap.
On 2014/11/08 01:06:36, jamesr wrote: > Seems like a bad API name if it's not intended to be for tap. Agreed - this is why I think we should leave the existing API in place, and only call this new API when we really want to mimic the hit testing behavior of tap: https://codereview.chromium.org/470833002/#msg36
On 2014/11/10 18:44:10, Rick Byers wrote: > On 2014/11/08 01:06:36, jamesr wrote: > > Seems like a bad API name if it's not intended to be for tap. > > Agreed - this is why I think we should leave the existing API in place, and only > call this new API when we really want to mimic the hit testing behavior of tap: > https://codereview.chromium.org/470833002/#msg36 I agree. Please ping me to review again once this is resolved in the blink API. Don't land before then.
On 2014/11/10 18:44:10, Rick Byers wrote: > On 2014/11/08 01:06:36, jamesr wrote: > > Seems like a bad API name if it's not intended to be for tap. > > Agreed - this is why I think we should leave the existing API in place, and only > call this new API when we really want to mimic the hit testing behavior of tap: > https://codereview.chromium.org/470833002/#msg36 Alright. I reverted the part in content/renderer in PS10.
OK - it's up to the android_webview OWNERS then.
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/475633002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hush@chromium.org changed reviewers: + dcheng@chromium.org
Hi Daniel, Please take a look at android_webview/common/render_view_messages.h Thanks!
https://codereview.chromium.org/475633002/diff/300001/android_webview/browser... File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right): https://codereview.chromium.org/475633002/diff/300001/android_webview/browser... android_webview/browser/renderer_host/aw_render_view_host_ext.h:60: void RequestNewHitTestDataAt(gfx::PointF touch_center, gfx::SizeF touch_area); Pass by const ref, not by value. https://codereview.chromium.org/475633002/diff/300001/android_webview/rendere... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/300001/android_webview/rendere... android_webview/renderer/aw_render_view_ext.cc:268: void AwRenderViewExt::OnDoHitTest(gfx::PointF touch_center, Pass by const ref, not by value.
https://codereview.chromium.org/475633002/diff/300001/android_webview/browser... File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right): https://codereview.chromium.org/475633002/diff/300001/android_webview/browser... android_webview/browser/renderer_host/aw_render_view_host_ext.h:60: void RequestNewHitTestDataAt(gfx::PointF touch_center, gfx::SizeF touch_area); On 2014/11/13 02:37:58, dcheng wrote: > Pass by const ref, not by value. Done. https://codereview.chromium.org/475633002/diff/300001/android_webview/rendere... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/475633002/diff/300001/android_webview/rendere... android_webview/renderer/aw_render_view_ext.cc:268: void AwRenderViewExt::OnDoHitTest(gfx::PointF touch_center, On 2014/11/13 02:37:58, dcheng wrote: > Pass by const ref, not by value. Done.
lgtm
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/475633002/320001
Message was sent while issue was closed.
Committed patchset #11 (id:320001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/1d93adc3addd7b336d541f77eb272c2aa43a7053 Cr-Commit-Position: refs/heads/master@{#304067} |
