Bug 85101 - [chromium] Compute the best target node on a GestureTap event
Summary: [chromium] Compute the best target node on a GestureTap event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Terry Anderson
URL:
Keywords:
Depends on: 85289 85296 86083 86218 86260 86397
Blocks: 83947 84487
  Show dependency treegraph
 
Reported: 2012-04-27 14:47 PDT by Terry Anderson
Modified: 2012-05-15 12:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.32 KB, patch)
2012-04-27 14:53 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (8.97 KB, patch)
2012-04-30 15:54 PDT, Terry Anderson
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.26 MB, application/zip)
2012-04-30 17:20 PDT, WebKit Review Bot
no flags Details
Patch (5.01 KB, patch)
2012-05-01 11:08 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (5.59 KB, patch)
2012-05-01 13:03 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (15.24 KB, patch)
2012-05-03 17:21 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (8.13 KB, patch)
2012-05-07 16:26 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (6.50 KB, patch)
2012-05-10 08:00 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (4.93 KB, patch)
2012-05-10 14:15 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Diffs for two failing touch adjustment layout tests in chromium (1.73 KB, text/plain)
2012-05-10 14:19 PDT, Terry Anderson
no flags Details
Archive of layout-test-results from ec2-cr-linux-02 (860.96 KB, application/zip)
2012-05-10 21:56 PDT, WebKit Review Bot
no flags Details
Patch (5.42 KB, patch)
2012-05-11 10:07 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Diff for failing test touchadjustment/touch-inlines.html (864 bytes, text/plain)
2012-05-14 08:00 PDT, Terry Anderson
no flags Details
Patch (5.58 KB, patch)
2012-05-14 10:25 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2012-05-15 10:16 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Terry Anderson 2012-04-27 14:47:34 PDT
Use the new function WebViewImpl::computeBestTouchTarget determine the most likely target Node given a rectangular area as input.  Still to do: Refactor to avoid multiple hit tests.
Comment 1 Terry Anderson 2012-04-27 14:53:20 PDT
Created attachment 139282 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-27 14:58:58 PDT
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.
Comment 3 Terry Anderson 2012-04-30 15:54:00 PDT
Created attachment 139535 [details]
Patch
Comment 4 WebKit Review Bot 2012-04-30 15:57:31 PDT
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 5 WebKit Review Bot 2012-04-30 17:20:51 PDT
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
Comment 6 WebKit Review Bot 2012-04-30 17:20:56 PDT
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
Comment 7 Terry Anderson 2012-05-01 11:08:36 PDT
Created attachment 139653 [details]
Patch
Comment 8 Terry Anderson 2012-05-01 11:10:15 PDT
(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 9 WebKit Review Bot 2012-05-01 11:23:39 PDT
Comment on attachment 139653 [details]
Patch

Attachment 139653 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12588623
Comment 10 Robert Kroeger 2012-05-01 11:34:15 PDT
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?
Comment 11 Terry Anderson 2012-05-01 13:03:56 PDT
Created attachment 139661 [details]
Patch
Comment 12 Terry Anderson 2012-05-01 13:06:53 PDT
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...
Comment 13 Terry Anderson 2012-05-01 13:09:00 PDT
(In reply to comment #11)
> Created an attachment (id=139661) [details]
> Patch

Layout tests are in progress.
Comment 14 Darin Fisher (:fishd, Google) 2012-05-01 13:22:25 PDT
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?
Comment 15 Terry Anderson 2012-05-03 17:21:27 PDT
Created attachment 140131 [details]
Patch
Comment 16 W. James MacLean 2012-05-04 06:45:07 PDT
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.
Comment 17 Robert Kroeger 2012-05-04 06:55:57 PDT
(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 18 Robert Kroeger 2012-05-04 07:00:26 PDT
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.
Comment 19 Terry Anderson 2012-05-07 16:26:46 PDT
Created attachment 140617 [details]
Patch
Comment 20 WebKit Review Bot 2012-05-07 16:32:03 PDT
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.
Comment 21 Terry Anderson 2012-05-07 16:37:20 PDT
(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
Comment 22 Allan Sandfeld Jensen 2012-05-08 00:36:44 PDT
(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 23 Allan Sandfeld Jensen 2012-05-08 00:43:27 PDT
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 24 Robert Kroeger 2012-05-08 06:37:48 PDT
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?
Comment 25 Allan Sandfeld Jensen 2012-05-08 06:59:59 PDT
(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.
Comment 26 Allan Sandfeld Jensen 2012-05-10 04:10:27 PDT
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.
Comment 27 Terry Anderson 2012-05-10 08:00:05 PDT
Created attachment 141172 [details]
Patch
Comment 28 Terry Anderson 2012-05-10 08:04:51 PDT
(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.
Comment 29 Allan Sandfeld Jensen 2012-05-10 12:58:34 PDT
(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 :)
Comment 30 Terry Anderson 2012-05-10 14:15:55 PDT
Created attachment 141251 [details]
Patch
Comment 31 Terry Anderson 2012-05-10 14:19:27 PDT
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
Comment 32 WebKit Review Bot 2012-05-10 14:23:34 PDT
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.
Comment 33 Terry Anderson 2012-05-10 14:30:31 PDT
(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 34 WebKit Review Bot 2012-05-10 21:56:18 PDT
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
Comment 35 WebKit Review Bot 2012-05-10 21:56:23 PDT
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
Comment 36 Allan Sandfeld Jensen 2012-05-11 07:18:31 PDT
(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.
Comment 37 Terry Anderson 2012-05-11 10:07:31 PDT
Created attachment 141438 [details]
Patch
Comment 38 Adam Barth 2012-05-11 10:10:39 PDT
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?
Comment 39 Terry Anderson 2012-05-11 10:14:12 PDT
(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 40 Adam Barth 2012-05-11 10:24:48 PDT
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 41 Terry Anderson 2012-05-11 10:38:37 PDT
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 42 Adam Barth 2012-05-11 10:48:12 PDT
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 43 WebKit Review Bot 2012-05-11 14:15:35 PDT
Comment on attachment 141438 [details]
Patch

Clearing flags on attachment: 141438

Committed r116802: <http://trac.webkit.org/changeset/116802>
Comment 44 WebKit Review Bot 2012-05-11 14:15:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 45 WebKit Review Bot 2012-05-11 15:32:14 PDT
Re-opened since this is blocked by 86260
Comment 46 Allan Sandfeld Jensen 2012-05-12 09:45:31 PDT
(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.
Comment 47 Terry Anderson 2012-05-14 08:00:13 PDT
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.
Comment 48 Terry Anderson 2012-05-14 10:25:05 PDT
Created attachment 141748 [details]
Patch
Comment 49 Adam Barth 2012-05-14 11:22:29 PDT
Comment on attachment 141748 [details]
Patch

Ok.
Comment 50 WebKit Review Bot 2012-05-14 12:23:31 PDT
Comment on attachment 141748 [details]
Patch

Clearing flags on attachment: 141748

Committed r116983: <http://trac.webkit.org/changeset/116983>
Comment 51 WebKit Review Bot 2012-05-14 12:23:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 52 WebKit Review Bot 2012-05-14 13:42:55 PDT
Re-opened since this is blocked by 86397
Comment 53 Terry Anderson 2012-05-15 10:16:51 PDT
Created attachment 141992 [details]
Patch
Comment 54 Terry Anderson 2012-05-15 10:17:32 PDT
(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 55 Adam Barth 2012-05-15 10:22:22 PDT
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 56 WebKit Review Bot 2012-05-15 12:58:12 PDT
Comment on attachment 141992 [details]
Patch

Clearing flags on attachment: 141992

Committed r117128: <http://trac.webkit.org/changeset/117128>
Comment 57 WebKit Review Bot 2012-05-15 12:58:20 PDT
All reviewed patches have been landed.  Closing bug.