Bug 83428

Created attachment 136138 [details] Patch

also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node)) return; ASSERT(node);

(In reply to comment #2) > also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this > > if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node)) > return; > > ASSERT(node); 'if (!mainframe' obviously.

(In reply to comment #2) > also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this > > if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node)) > return; > > ASSERT(node); I am not sure if the return makes sense. The ASSERT is better. If nothing else is found the root element should be returned.

(In reply to comment #4) > (In reply to comment #2) > > also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this > > > > if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node)) > > return; > > > > ASSERT(node); > > I am not sure if the return makes sense. The ASSERT is better. If nothing else is found the root element should be returned. ok, sounds good. We lose(ignore) a bool return value on this callstack (findZoomableAreaForPoint()->bestZoomableAreaForTouchPoint()->findBestZoomableArea()->findNodeWithLowestDistanceMetric()) and that is either defective or it suggests that it was originally designed with a different mindset and needs to be changed.

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > > also, wondering if bestZoomableAreaForTouchPoint()'s return value should be checked and do something like this > > > > > > if (mainframe->eventHandler()->bestZoomableAreaForTouchPoint(point, IntSize(area.width() / 2, area.height() / 2), zoomableArea, node)) > > > return; > > > > > > ASSERT(node); > > > > I am not sure if the return makes sense. The ASSERT is better. If nothing else is found the root element should be returned. > > ok, sounds good. We lose(ignore) a bool return value on this callstack (findZoomableAreaForPoint()->bestZoomableAreaForTouchPoint()->findBestZoomableArea()->findNodeWithLowestDistanceMetric()) and that is either defective or it suggests that it was originally designed with a different mindset and needs to be changed. True. More accurately I have no problem with the return if it happens after an ASSERT. It can theoretically happen, but would mean the touch-point given is outside the frame, which is not really wrong, but indicates something has transformed the pointer wrong, or has send the event to the wrong application.

Created attachment 136507 [details] Patch

Comment on attachment 136507 [details] Patch Now looking at the patch, I might need to split it up. I'll do after the review if the direction is okay.

Comment on attachment 136507 [details] Patch Attachment 136507 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12380445

Comment on attachment 136507 [details] Patch Attachment 136507 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12381436

Comment on attachment 136507 [details] Patch Attachment 136507 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12385246

Created attachment 136716 [details] Patch

ixed exporting. (hopefully for gtk too)

(In reply to comment #8) > (From update of attachment 136507 [details]) > Now looking at the patch, I might need to split it up. I'll do after the review if the direction is okay. This patch also modifies the behavior of highlighting a bit. Previously when hittest returned no valid node, the ongoing highlighting was cancelled, even though the UI process sends (0,0) to cancel it. With this patch, only explicit cancelling terminates highlighting.

(In reply to comment #14) > (In reply to comment #8) > > (From update of attachment 136507 [details] [details]) > > Now looking at the patch, I might need to split it up. I'll do after the review if the direction is okay. > > This patch also modifies the behavior of highlighting a bit. Previously when hittest returned no valid node, the ongoing highlighting was cancelled, even though the UI process sends (0,0) to cancel it. With this patch, only explicit cancelling terminates highlighting. That is fine I guess, but it is kinda of theoretical. The highlight is only active for a short time during or shortly after a touch and I guess the gesture recognizer will cancel the old highlight before trying to highlight a new touch. The patch looks great btw.

Comment on attachment 136716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136716&action=review > Source/WebCore/testing/Internals.cpp:650 > +void Internals::setDelegatesScrolling(bool enabled, Document* document, ExceptionCode& ec) Nice! > LayoutTests/touchadjustment/iframe-with-mainframe-scroll-offset.html:1 > +<html> Shouldn\t the name of this file indicate that it is using scroll delegation? or subdir?

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #8) > > > (From update of attachment 136507 [details] [details] [details]) > > > Now looking at the patch, I might need to split it up. I'll do after the review if the direction is okay. > > > > This patch also modifies the behavior of highlighting a bit. Previously when hittest returned no valid node, the ongoing highlighting was cancelled, even though the UI process sends (0,0) to cancel it. With this patch, only explicit cancelling terminates highlighting. > > That is fine I guess, but it is kinda of theoretical. The highlight is only active for a short time during or shortly after a touch and I guess the gesture recognizer will cancel the old highlight before trying to highlight a new touch. Yes, this is merely making the functionality more explicit, but hardly involves change in real life usecases. the code looked more compact after this change which was the only reason to have it included. Thanks for the review.

(In reply to comment #16) > (From update of attachment 136716 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136716&action=review > > > Source/WebCore/testing/Internals.cpp:650 > > +void Internals::setDelegatesScrolling(bool enabled, Document* document, ExceptionCode& ec) > > Nice! > > > LayoutTests/touchadjustment/iframe-with-mainframe-scroll-offset.html:1 > > +<html> > > Shouldn\t the name of this file indicate that it is using scroll delegation? or subdir? Good idea. i'll make a subdir for scroll delegation cases. Thanks.

Created attachment 136859 [details] Patch

Comment on attachment 136859 [details] Patch Clearing flags on attachment: 136859 Committed r113962: <http://trac.webkit.org/changeset/113962>

All reviewed patches have been landed. Closing bug.

Reopen, because a test started to assert from this change. crash log for DumpRenderTree (pid 15712): STDOUT: <empty> STDERR: ASSERTION FAILED: compositor->inCompositingMode() STDERR: ../../../../Source/WebCore/page/FrameView.cpp(657) : void WebCore::FrameView::clearBackingStores() STDERR: 1 0x7f2815382862 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore9FrameView18clearBackingStoresEv+0x6a) [0x7f2815382862] STDERR: 2 0x7f2815386428 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore9FrameView27delegatesScrollingDidChangeEv+0x18) [0x7f2815386428] STDERR: 3 0x7f281548c430 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore10ScrollView21setDelegatesScrollingEb+0x48) [0x7f281548c430] STDERR: 4 0x7f28156ea9e3 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore9Internals21setDelegatesScrollingEbPNS_8DocumentERi+0xa1) [0x7f28156ea9e3] STDERR: 5 0x7f2815dcf76f /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore49jsInternalsPrototypeFunctionSetDelegatesScrollingEPN3JSC9ExecStateE+0x297) [0x7f2815dcf76f] STDERR: 6 0x7f27c8ef7258 [0x7f27c8ef7258] Could you check it, please?

created bug#83860 for this problem.

fixed in http://trac.webkit.org/changeset/114109