70152 2011-10-14 15:15:49 -0700 Widget window coordinate functions should use root view coordinate functions 2011-10-19 14:38:36 -0700 1 1 1 Unclassified WebKit Frames 528+ (Nightly build) All All RESOLVED WONTFIX P2 Normal --- 70288 1 jeffm jeffm rniwa simon.fraser webkit.review.bot oldest_to_newest 484347 0 jeffm 2011-10-14 15:15:49 -0700 Follow-up to bug 69963. Simon Fraser suggests: > Source/WebCore/platform/Widget.cpp:106 > +IntRect Widget::convertFromRootView(const IntRect& rootRect) const > +{ > + if (const ScrollView* parentScrollView = parent()) { > + IntRect parentRect = parentScrollView->convertFromRootView(rootRect); > + return convertFromContainingView(parentRect); > + } > + return rootRect; > +} > + > +IntRect Widget::convertToRootView(const IntRect& localRect) const > +{ > + if (const ScrollView* parentScrollView = parent()) { > + IntRect parentRect = convertToContainingView(localRect); > + return parentScrollView->convertToRootView(parentRect); > + } > + return localRect; > +} > + > +IntPoint Widget::convertFromRootView(const IntPoint& rootPoint) const > +{ > + if (const ScrollView* parentScrollView = parent()) { > + IntPoint parentPoint = parentScrollView->convertFromRootView(rootPoint); > + return convertFromContainingView(parentPoint); > + } > + return rootPoint; > +} > + > +IntPoint Widget::convertToRootView(const IntPoint& localPoint) const > +{ > + if (const ScrollView* parentScrollView = parent()) { > + IntPoint parentPoint = convertToContainingView(localPoint); > + return parentScrollView->convertToRootView(parentPoint); > + } > + return localPoint; > +} How about calling these from the existing toWindow/fromWindow calls? 485434 1 111307 jeffm 2011-10-17 13:15:27 -0700 Created attachment 111307 Patch 485439 2 webkit.review.bot 2011-10-17 13:18:39 -0700 Attachment 111307 did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource From git://git.webkit.org/WebKit abfc01f..284a880 master -> origin/master M Source/WebKit/mac/ChangeLog M Source/WebKit/mac/WebView/WebView.mm M Source/WebCore/WebCore.exp.in M Source/WebCore/ChangeLog M Source/WebCore/page/Page.h M Source/WebCore/page/Page.cpp M Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp M Source/WebKit2/ChangeLog r97640 = 284a880fb02564497ff4d4c705cfc8d3ca46285e (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style. 485454 3 jeffm 2011-10-17 13:27:56 -0700 (In reply to comment #2) > Attachment 111307 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 > > Updating OpenSource > From git://git.webkit.org/WebKit > abfc01f..284a880 master -> origin/master > M Source/WebKit/mac/ChangeLog > M Source/WebKit/mac/WebView/WebView.mm > M Source/WebCore/WebCore.exp.in > M Source/WebCore/ChangeLog > M Source/WebCore/page/Page.h > M Source/WebCore/page/Page.cpp > M Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp > M Source/WebKit2/ChangeLog > r97640 = 284a880fb02564497ff4d4c705cfc8d3ca46285e (refs/remotes/trunk) > First, rewinding head to replay your work on top of it... > Fast-forwarded master to refs/remotes/trunk. > Updating chromium port dependencies using gclient... > Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. > Re-trying 'depot_tools/gclient sync' > Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. > Re-trying 'depot_tools/gclient sync' > Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. > Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. > Re-trying 'depot_tools/gclient sync' > No such file or directory at Tools/Scripts/update-webkit line 104. > > > If any of these errors are false positives, please file a bug against check-webkit-style. This patch passed check-webkit-style locally when I used "webkit-patch upload". This looks like a temporary failure and not an actual bug in check-webkit-style, so I'm not going to file a bug. 485456 4 jeffm 2011-10-17 13:29:09 -0700 Committed r97641: <http://trac.webkit.org/changeset/97641> 485702 5 rniwa 2011-10-17 18:13:18 -0700 Reopen the bug because it was rolled out in http://trac.webkit.org/changeset/97688. 486145 6 jeffm 2011-10-18 10:29:02 -0700 Ryosuke Niwa notes: "This patch appears to have caused 15+ tests to fail on Snow Leopard: http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r97655%20(2609)/results.html Tests passed on r97640 but fail on r97646." I will ensure these tests pass before landing a new patch. 486163 7 jeffm 2011-10-18 10:51:26 -0700 I verified that my original patch does cause these test failures. 487017 8 jeffm 2011-10-19 14:38:36 -0700 After further investigation, it would be sub-optimal to try to leverage the new root view coordinate functions when dealing with window coordinates. Since convertFromRootToContainingWindow() and convertFromContainingWindowToRoot() require the root widget, so we would have to walk up the view hierarchy twice. The test failures were being caused by the fact that I was passing the non-root widget to these two member functions, which led to incorrect results on the Mac. Closing as WONTFIX. 111307 2011-10-17 13:15:27 -0700 2011-10-18 10:29:52 -0700 Patch bug-70152-20111017131525.patch text/plain 2874 jeffm SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDk3NjM5KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMTAtMTcgIEplZmYgTWls bGVyICA8amVmZm1AYXBwbGUuY29tPgorCisgICAgICAgIFdpZGdldCB3aW5kb3cgY29vcmRpbmF0 ZSBmdW5jdGlvbnMgc2hvdWxkIHVzZSByb290IHZpZXcgY29vcmRpbmF0ZSBmdW5jdGlvbnMKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTcwMTUyCisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3Rz LCBubyBjaGFuZ2VzIHRvIGZ1bmN0aW9uYWxpdHkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9XaWRn ZXQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6V2lkZ2V0Ojpjb252ZXJ0RnJvbUNvbnRhaW5pbmdX aW5kb3cpOiBVc2UgY29udmVydEZyb21Sb290VmlldygpLgorICAgICAgICAoV2ViQ29yZTo6V2lk Z2V0Ojpjb252ZXJ0VG9Db250YWluaW5nV2luZG93KTogVXNlIGNvbnZlcnRUb1Jvb3RWaWV3KCku CisKIDIwMTEtMTAtMTcgIEFudHRpIEtvaXZpc3RvICA8YW50dGlAYXBwbGUuY29tPgogCiAgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02OTk2NgpJbmRleDog U291cmNlL1dlYkNvcmUvcGxhdGZvcm0vV2lkZ2V0LmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9XaWRnZXQuY3BwCShyZXZpc2lvbiA5NzYzMykKKysrIFNvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL1dpZGdldC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTEwNywzOCArMTA3 LDIyIEBAIEludFBvaW50IFdpZGdldDo6Y29udmVydFRvUm9vdFZpZXcoY29uc3QKIAogSW50UmVj dCBXaWRnZXQ6OmNvbnZlcnRGcm9tQ29udGFpbmluZ1dpbmRvdyhjb25zdCBJbnRSZWN0JiB3aW5k b3dSZWN0KSBjb25zdAogewotICAgIGlmIChjb25zdCBTY3JvbGxWaWV3KiBwYXJlbnRTY3JvbGxW aWV3ID0gcGFyZW50KCkpIHsKLSAgICAgICAgSW50UmVjdCBwYXJlbnRSZWN0ID0gcGFyZW50U2Ny b2xsVmlldy0+Y29udmVydEZyb21Db250YWluaW5nV2luZG93KHdpbmRvd1JlY3QpOwotICAgICAg ICByZXR1cm4gY29udmVydEZyb21Db250YWluaW5nVmlldyhwYXJlbnRSZWN0KTsKLSAgICB9Ci0g ICAgcmV0dXJuIGNvbnZlcnRGcm9tQ29udGFpbmluZ1dpbmRvd1RvUm9vdCh0aGlzLCB3aW5kb3dS ZWN0KTsKKyAgICByZXR1cm4gY29udmVydEZyb21Sb290Vmlldyhjb252ZXJ0RnJvbUNvbnRhaW5p bmdXaW5kb3dUb1Jvb3QodGhpcywgd2luZG93UmVjdCkpOwogfQogCiBJbnRSZWN0IFdpZGdldDo6 Y29udmVydFRvQ29udGFpbmluZ1dpbmRvdyhjb25zdCBJbnRSZWN0JiBsb2NhbFJlY3QpIGNvbnN0 CiB7Ci0gICAgaWYgKGNvbnN0IFNjcm9sbFZpZXcqIHBhcmVudFNjcm9sbFZpZXcgPSBwYXJlbnQo KSkgewotICAgICAgICBJbnRSZWN0IHBhcmVudFJlY3QgPSBjb252ZXJ0VG9Db250YWluaW5nVmll dyhsb2NhbFJlY3QpOwotICAgICAgICByZXR1cm4gcGFyZW50U2Nyb2xsVmlldy0+Y29udmVydFRv Q29udGFpbmluZ1dpbmRvdyhwYXJlbnRSZWN0KTsKLSAgICB9Ci0gICAgcmV0dXJuIGNvbnZlcnRG cm9tUm9vdFRvQ29udGFpbmluZ1dpbmRvdyh0aGlzLCBsb2NhbFJlY3QpOworICAgIHJldHVybiBj b252ZXJ0RnJvbVJvb3RUb0NvbnRhaW5pbmdXaW5kb3codGhpcywgY29udmVydFRvUm9vdFZpZXco bG9jYWxSZWN0KSk7CiB9CiAKIEludFBvaW50IFdpZGdldDo6Y29udmVydEZyb21Db250YWluaW5n V2luZG93KGNvbnN0IEludFBvaW50JiB3aW5kb3dQb2ludCkgY29uc3QKIHsKLSAgICBpZiAoY29u c3QgU2Nyb2xsVmlldyogcGFyZW50U2Nyb2xsVmlldyA9IHBhcmVudCgpKSB7Ci0gICAgICAgIElu dFBvaW50IHBhcmVudFBvaW50ID0gcGFyZW50U2Nyb2xsVmlldy0+Y29udmVydEZyb21Db250YWlu aW5nV2luZG93KHdpbmRvd1BvaW50KTsKLSAgICAgICAgcmV0dXJuIGNvbnZlcnRGcm9tQ29udGFp bmluZ1ZpZXcocGFyZW50UG9pbnQpOwotICAgIH0KLSAgICByZXR1cm4gY29udmVydEZyb21Db250 YWluaW5nV2luZG93VG9Sb290KHRoaXMsIHdpbmRvd1BvaW50KTsKKyAgICByZXR1cm4gY29udmVy dEZyb21Sb290Vmlldyhjb252ZXJ0RnJvbUNvbnRhaW5pbmdXaW5kb3dUb1Jvb3QodGhpcywgd2lu ZG93UG9pbnQpKTsKIH0KIAogSW50UG9pbnQgV2lkZ2V0Ojpjb252ZXJ0VG9Db250YWluaW5nV2lu ZG93KGNvbnN0IEludFBvaW50JiBsb2NhbFBvaW50KSBjb25zdAogewotICAgIGlmIChjb25zdCBT Y3JvbGxWaWV3KiBwYXJlbnRTY3JvbGxWaWV3ID0gcGFyZW50KCkpIHsKLSAgICAgICAgSW50UG9p bnQgcGFyZW50UG9pbnQgPSBjb252ZXJ0VG9Db250YWluaW5nVmlldyhsb2NhbFBvaW50KTsKLSAg ICAgICAgcmV0dXJuIHBhcmVudFNjcm9sbFZpZXctPmNvbnZlcnRUb0NvbnRhaW5pbmdXaW5kb3co cGFyZW50UG9pbnQpOwotICAgIH0KLSAgICByZXR1cm4gY29udmVydEZyb21Sb290VG9Db250YWlu aW5nV2luZG93KHRoaXMsIGxvY2FsUG9pbnQpOworICAgIHJldHVybiBjb252ZXJ0RnJvbVJvb3RU b0NvbnRhaW5pbmdXaW5kb3codGhpcywgY29udmVydFRvUm9vdFZpZXcobG9jYWxQb2ludCkpOwog fQogCiAjaWYgIVBMQVRGT1JNKE1BQykK