Summary: | [chromium] Compute the best target node on a GestureTap event | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Terry Anderson <tdanderson> | ||||||||||||||||||||||||||||||||
Component: | Platform | Assignee: | Terry Anderson <tdanderson> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | allan.jensen, dglazkov, rjkroege, trchen, webkit.review.bot, wjmaclean | ||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
Bug Depends on: | 85289, 85296, 86083, 86218, 86260, 86397 | ||||||||||||||||||||||||||||||||||
Bug Blocks: | 83947, 84487 | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Terry Anderson
2012-04-27 14:47:34 PDT
Created attachment 139282 [details]
Patch
Attachment 139282 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 139535 [details]
Patch
Attachment 139535 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 139535 [details] Patch Attachment 139535 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12585441 New failing tests: fast/events/touch/gesture/gesture-click.html fast/events/touch/page-scaled-touch-gesture-click.html Created attachment 139550 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 139653 [details]
Patch
(In reply to comment #7) > Created an attachment (id=139653) [details] > Patch Depends on https://bugs.webkit.org/show_bug.cgi?id=85296 landing first, since this patch calls WebCore::EventHandler::handleGestureTap with an extra parameter. Comment on attachment 139653 [details] Patch Attachment 139653 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12588623 Comment on attachment 139653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139653&action=review > Source/WebKit/chromium/ChangeLog:3 > + [chromium] Compute the best target node on a GestureTap event My limited intuition of the way-of-WebKit is that this needs to be a bit more detailed. I want more detail and I participated in the design discussions so a reviewer probably needs more context. > Source/WebKit/chromium/src/WebViewImpl.cpp:647 > + // rectangle-based hit detection to find the best touch target afaik, these kinds of comments are usually not found in WebKit. > Source/WebKit/chromium/src/WebViewImpl.cpp:665 > + m_bestTouchTarget = 0; I think m_bestTouchTarget.clear() would be better? > Source/WebKit/chromium/src/WebViewImpl.cpp:958 > + return node; is this right usage? Created attachment 139661 [details]
Patch
Comment on attachment 139653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139653&action=review >> Source/WebKit/chromium/ChangeLog:3 >> + [chromium] Compute the best target node on a GestureTap event > > My limited intuition of the way-of-WebKit is that this needs to be a bit more detailed. I want more detail and I participated in the design discussions so a reviewer probably needs more context. I added a more thorough description in the ChangeLog. >> Source/WebKit/chromium/src/WebViewImpl.cpp:647 >> + // rectangle-based hit detection to find the best touch target > > afaik, these kinds of comments are usually not found in WebKit. Removed. >> Source/WebKit/chromium/src/WebViewImpl.cpp:665 >> + m_bestTouchTarget = 0; > > I think m_bestTouchTarget.clear() would be better? WebCore::Node does not have a clear() routine. >> Source/WebKit/chromium/src/WebViewImpl.cpp:958 >> + return node; > > is this right usage? I believe so, but I'm not 100% sure... (In reply to comment #11) > Created an attachment (id=139661) [details] > Patch Layout tests are in progress. Comment on attachment 139661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139661&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:647 > + WebCore::IntPoint inputPoint(event.x, event.y); nit: no need for WebCore:: prefix > Source/WebKit/chromium/src/WebViewImpl.cpp:953 > + || node->hasEventListeners(eventNames().clickEvent) nit: please indent by four spaces instead of only one > Source/WebKit/chromium/src/WebViewImpl.h:792 > + RefPtr<WebCore::Node> m_bestTouchTarget; Why does this need to be a member variable? Created attachment 140131 [details]
Patch
Comment on attachment 140131 [details]
Patch
Looks reasonable (to me) in general, but I wonder if we should consider moving some or all of this out of WebViewImpl and into a utility class. This might allow it to be useful here and elsewhere, and I'm worried that WVI is becoming a bit of a catch-all class.
(In reply to comment #16) > (From update of attachment 140131 [details]) > Looks reasonable (to me) in general, but I wonder if we should consider moving some or all of this out of WebViewImpl and into a utility class. This might allow it to be useful here and elsewhere, and I'm worried that WVI is becoming a bit of a catch-all class. I've been worried about WVI bloat myself. This seems like an excellent suggestion. Comment on attachment 140131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140131&action=review > Source/WebKit/chromium/ChangeLog:9 > + (WebKit::WebViewImpl::handleGestureEvent): Your ChangeLog doesn't seem formatted quite like the others. > Source/WebKit/chromium/ChangeLog:28 > + Note: I expect that this patch will cause progressions in two existing layout You mean regressions or failures? > Source/WebKit/chromium/src/WebViewImpl.h:599 > + // Returns the best touch target given a rectangle defined by a center point these can be inside enable(GESTURE_EVENTS) right? And I agree with wjmaclean@: a separate external utility class would probably be a good idea. Created attachment 140617 [details]
Patch
Attachment 140617 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #19) > Created an attachment (id=140617) [details] > Patch Change of plan: I would now like to use the TOUCH_ADJUSTMENT code in WebCore instead of re-writing this in WebViewImpl. After speaking with Allan today (carewolf on #webkit) I discovered that this code will work for chromium if the scroll offsets are taken into account. Allan: can you please comment on how my proposed changes will impact the qt platform? If needed I can always put some parts of this code behind a setting so that it doesn't get in your platform's way. I tried this out on a variety of pages and it seems to work fine, but I do get an "Aw, Snap" (crash) when dispatching a GestureTap event on the checkboxes here: http://www.w3schools.com/html/tryit.asp?filename=tryhtml_checkbox (In reply to comment #21) > (In reply to comment #19) > > Created an attachment (id=140617) [details] [details] > > Patch > > Change of plan: I would now like to use the TOUCH_ADJUSTMENT code in WebCore instead of re-writing this in WebViewImpl. After speaking with Allan today (carewolf on #webkit) I discovered that this code will work for chromium if the scroll offsets are taken into account. > > Allan: can you please comment on how my proposed changes will impact the qt platform? If needed I can always put some parts of this code behind a setting so that it doesn't get in your platform's way. > It shouldn't affect the Qt platform, as far as I can tell. The problem is we use a slightly different scrolling model. Please add regression-tests though. I have some touch-adjustment tests in LayoutTests/touchadjustment, but you should add tests with overflow and scroll-offset. Comment on attachment 140617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140617&action=review > Source/WebCore/page/EventHandler.cpp:2440 > + bestClickableNodeForTouchPoint(gestureEvent.position(), IntSize(gestureEvent.area().width() / 2, gestureEvent.area().height() / 2), adjustedPoint, targetNode, containingFrame); containingFrame can just as well be calculated in bestClickableNodeForTouchPoint as here, so I don't think there is any reason to give it as an argument. > Source/WebCore/page/EventHandler.cpp:2446 > + // convert back to window coordinates > + adjustedPoint.setX(adjustedPoint.x() - containingFrame->document()->view()->scrollX()); > + adjustedPoint.setY(adjustedPoint.y() - containingFrame->document()->view()->scrollY()); I think this conversion should be done in findBestClickableCandidate, preferable using contentsToWindow. Comment on attachment 140617 [details]
Patch
Perhaps you could add some kind of window.internals based test for bestClickableNodeForTouchPoint? And/or the layout tests you were preparing?
(In reply to comment #24) > (From update of attachment 140617 [details]) > Perhaps you could add some kind of window.internals based test for bestClickableNodeForTouchPoint? And/or the layout tests you were preparing? There is an internals API for it, just follow the examples in LayoutTests/touchadjustment. This depends on fixing the bug in touch-adjustment that scroll-offset was not properly handled. I am tracking that separately in bug #86083 now. Created attachment 141172 [details]
Patch
(In reply to comment #27) > Created an attachment (id=141172) [details] > Patch This patch is an improvement over my last one, as it appears to work for cases involving frames and scroll offsets. But in light of https://bugs.webkit.org/show_bug.cgi?id=86083 being filed (and about to land), I will have to re-revise. (In reply to comment #28) > (In reply to comment #27) > > Created an attachment (id=141172) [details] [details] > > Patch > > This patch is an improvement over my last one, as it appears to work for cases involving frames and scroll offsets. But in light of https://bugs.webkit.org/show_bug.cgi?id=86083 being filed (and about to land), I will have to re-revise. I apologize, but your arguments made me realize it was a general problem for both touch-adjustment functions, so I wanted a general solution. Hopefully this should work for you as well, otherwise feel free to improve it :) Created attachment 141251 [details]
Patch
Created attachment 141253 [details]
Diffs for two failing touch adjustment layout tests in chromium
In the chromium port, all of the layout tests in the touchadjustment/ directory pass except for two:
touchadjustment/event-triggered-widgets.html
touchadjustment/touch-inlines.html
Attachment 141251 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #29) > (In reply to comment #28) > > (In reply to comment #27) > > > Created an attachment (id=141172) [details] [details] [details] > > > Patch > > > > This patch is an improvement over my last one, as it appears to work for cases involving frames and scroll offsets. But in light of https://bugs.webkit.org/show_bug.cgi?id=86083 being filed (and about to land), I will have to re-revise. > > I apologize, but your arguments made me realize it was a general problem for both touch-adjustment functions, so I wanted a general solution. Hopefully this should work for you as well, otherwise feel free to improve it :) I tried out the touch adjustment code in chromium after your patch (https://bugs.webkit.org/show_bug.cgi?id=86083) landed, and everything seemed to work great without making any other modifications to the code. However, two of the layout tests in touchadjustment/ fail (I have attached the diffs to this bug) and it is not immediately obvious to me why this is. Do you have any ideas about what might be causing this difference? Comment on attachment 141251 [details] Patch Attachment 141251 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12671230 New failing tests: touchadjustment/touch-inlines.html touchadjustment/event-triggered-widgets.html plugins/embed-attributes-style.html Created attachment 141325 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #33) > (In reply to comment #29) > > (In reply to comment #28) > > > (In reply to comment #27) > > > > Created an attachment (id=141172) [details] [details] [details] [details] > > > > Patch > > > > > > This patch is an improvement over my last one, as it appears to work for cases involving frames and scroll offsets. But in light of https://bugs.webkit.org/show_bug.cgi?id=86083 being filed (and about to land), I will have to re-revise. > > > > I apologize, but your arguments made me realize it was a general problem for both touch-adjustment functions, so I wanted a general solution. Hopefully this should work for you as well, otherwise feel free to improve it :) > > I tried out the touch adjustment code in chromium after your patch (https://bugs.webkit.org/show_bug.cgi?id=86083) landed, and everything seemed to work great without making any other modifications to the code. > > However, two of the layout tests in touchadjustment/ fail (I have attached the diffs to this bug) and it is not immediately obvious to me why this is. Do you have any ideas about what might be causing this difference? These two tests fail for me as well when using DumpRenderTree, but works fine when I use WebKitTestRunner. They also pass using DRT on our automatic testing bots, so I assume they are flaky somehow. I am going to investigate it, so feel free to skip them until I find out why they change depending on platform. Created attachment 141438 [details]
Patch
Comment on attachment 141438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141438&action=review > Source/WebCore/WebCore.gypi:3058 > + 'page/TouchAdjustment.cpp', > + 'page/TouchAdjustment.h', Did you mean to svn add these files? (In reply to comment #36) > (In reply to comment #33) > > (In reply to comment #29) > > > (In reply to comment #28) > > > > (In reply to comment #27) > > > > > Created an attachment (id=141172) [details] [details] [details] [details] [details] > > > > > Patch > > > > > > > > This patch is an improvement over my last one, as it appears to work for cases involving frames and scroll offsets. But in light of https://bugs.webkit.org/show_bug.cgi?id=86083 being filed (and about to land), I will have to re-revise. > > > > > > I apologize, but your arguments made me realize it was a general problem for both touch-adjustment functions, so I wanted a general solution. Hopefully this should work for you as well, otherwise feel free to improve it :) > > > > I tried out the touch adjustment code in chromium after your patch (https://bugs.webkit.org/show_bug.cgi?id=86083) landed, and everything seemed to work great without making any other modifications to the code. > > > > However, two of the layout tests in touchadjustment/ fail (I have attached the diffs to this bug) and it is not immediately obvious to me why this is. Do you have any ideas about what might be causing this difference? > > These two tests fail for me as well when using DumpRenderTree, but works fine when I use WebKitTestRunner. They also pass using DRT on our automatic testing bots, so I assume they are flaky somehow. I am going to investigate it, so feel free to skip them until I find out why they change depending on platform. I have skipped them for now. I just went through and performed all of these tests manually one by one and can confirm that they actually work on the chromium port. Should we address the strange behavior of DRT with these tests in a separate bug? Comment on attachment 141438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141438&action=review Generally, this patch looks fine. One small question below. >> Source/WebCore/WebCore.gypi:3058 >> + 'page/TouchAdjustment.h', > > Did you mean to svn add these files? /me discovers that they're already in WebKit. > Source/WebKit/chromium/src/WebInputEventConversion.cpp:151 > + m_area = IntSize(e.deltaX * 2, e.deltaY * 2); Where does the constant 2 come from? Why is this WebKit's job rather than the embedder's job to do this multiplication. (I don't know much about the division of responsibility here.) Comment on attachment 141438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141438&action=review >> Source/WebKit/chromium/src/WebInputEventConversion.cpp:151 >> + m_area = IntSize(e.deltaX * 2, e.deltaY * 2); > > Where does the constant 2 come from? Why is this WebKit's job rather than the embedder's job to do this multiplication. (I don't know much about the division of responsibility here.) In chromium, I am using deltaX and deltaY to store the touch radii (i.e., the horizontal and vertical distances from the center of the touch region to the edge of the touch region) which in general can be given by the hardware as the radii of an elliptical touch region. What I'm doing here is using these two radii to define a rectangular touch region (of width deltaX*2 and height deltaY*2) to be used in the TouchAdjustment code. Comment on attachment 141438 [details]
Patch
Thanks for the explanation. I still don't fully understand why we don't set m_area for each type of event, but we can always change that in the future if necessary.
Comment on attachment 141438 [details] Patch Clearing flags on attachment: 141438 Committed r116802: <http://trac.webkit.org/changeset/116802> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 86260 (In reply to comment #39) > I have skipped them for now. I just went through and performed all of these tests manually one by one and can confirm that they actually work on the chromium port. Should we address the strange behavior of DRT with these tests in a separate bug? Bug #86218, but it should be solved by now. Turned out to be pixel/metrics differences in font-rendering, which shifted the text enough to give different hit-test results. Created attachment 141730 [details] Diff for failing test touchadjustment/touch-inlines.html The test touchadjustment/touch-inlines.html still fails on chromium after patch https://bugs.webkit.org/show_bug.cgi?id=86218 was landed. Created attachment 141748 [details]
Patch
Comment on attachment 141748 [details]
Patch
Ok.
Comment on attachment 141748 [details] Patch Clearing flags on attachment: 141748 Committed r116983: <http://trac.webkit.org/changeset/116983> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 86397 Created attachment 141992 [details]
Patch
(In reply to comment #53) > Created an attachment (id=141992) [details] > Patch This now compiles on chromium win: http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/29198 Comment on attachment 141992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141992&action=review > Source/WebCore/page/TouchAdjustment.cpp:234 > - return INFINITY; > + return std::numeric_limits<float>::infinity(); Ah, that makes sense. Comment on attachment 141992 [details] Patch Clearing flags on attachment: 141992 Committed r117128: <http://trac.webkit.org/changeset/117128> All reviewed patches have been landed. Closing bug. |