114146 2013-04-08 00:14:59 -0700 [BlackBerry] Replace getRect() with boundingBox() 2014-01-09 23:46:17 -0800 1 1 1 Unclassified WebKit WebKit BlackBerry 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 111729 0 berto webkit-unassigned cgarcia mifenton rwlbuis tonikitoo webkit.review.bot oldest_to_newest 870642 0 berto 2013-04-08 00:14:59 -0700 This was renamed in r128006: http://trac.webkit.org/changeset/128006/trunk/Source/WebCore/dom/Node.h 870643 1 196833 berto 2013-04-08 00:16:16 -0700 Created attachment 196833 Patch 870650 2 196833 cgarcia 2013-04-08 00:44:09 -0700 Comment on attachment 196833 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196833&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:2601 > - return view->contentsToWindow(node->getRect()); > + return view->contentsToWindow(IntRect(node->boundingBox())); Why do you need the IntRect()? 870653 3 berto 2013-04-08 00:49:50 -0700 (In reply to comment #2) > > Source/WebKit/blackberry/Api/WebPage.cpp:2601 > > - return view->contentsToWindow(node->getRect()); > > + return view->contentsToWindow(IntRect(node->boundingBox())); > > Why do you need the IntRect()? Node::boundingBox() returns LayoutRect. 870654 4 cgarcia 2013-04-08 00:54:06 -0700 (In reply to comment #3) > (In reply to comment #2) > > > Source/WebKit/blackberry/Api/WebPage.cpp:2601 > > > - return view->contentsToWindow(node->getRect()); > > > + return view->contentsToWindow(IntRect(node->boundingBox())); > > > > Why do you need the IntRect()? > > Node::boundingBox() returns LayoutRect. I think there's an explicit constructor for that, note that getRect() also returned a LayoutRect. 870657 5 berto 2013-04-08 01:01:11 -0700 (In reply to comment #4) > > > Why do you need the IntRect()? > > > > Node::boundingBox() returns LayoutRect. > > I think there's an explicit constructor for that Exactly, _explicit_, that's why there's no implicit conversion here: Source/WebKit/blackberry/Api/WebPage.cpp:2611:62: error: no matching function for call to 'WebCore::FrameView::contentsToWindow(WebCore::LayoutRect)' Source/WebKit/blackberry/Api/WebPage.cpp:2611:62: note: candidates are: Source/WebCore/platform/ScrollView.h:231:14: note: WebCore::IntPoint WebCore::ScrollView::contentsToWindow(const WebCore::IntPoint&) const Source/WebCore/platform/ScrollView.h:231:14: note: no known conversion for argument 1 from 'WebCore::LayoutRect' to 'const WebCore::IntPoint&' Source/WebCore/platform/ScrollView.h:233:13: note: WebCore::IntRect WebCore::ScrollView::contentsToWindow(const WebCore::IntRect&) const Source/WebCore/platform/ScrollView.h:233:13: note: no known conversion for argument 1 from 'WebCore::LayoutRect' to 'const WebCore::IntRect&' > note that getRect() also returned a LayoutRect. Well, the code was not compiling before either, but I guess it doesn't make sense to split this change in two :) 870661 6 cgarcia 2013-04-08 01:08:14 -0700 (In reply to comment #5) > (In reply to comment #4) > > > > Why do you need the IntRect()? > > > > > > Node::boundingBox() returns LayoutRect. > > > > I think there's an explicit constructor for that > > Exactly, _explicit_, that's why there's no implicit conversion here: > > Source/WebKit/blackberry/Api/WebPage.cpp:2611:62: error: no matching function for call to 'WebCore::FrameView::contentsToWindow(WebCore::LayoutRect)' > Source/WebKit/blackberry/Api/WebPage.cpp:2611:62: note: candidates are: > Source/WebCore/platform/ScrollView.h:231:14: note: WebCore::IntPoint WebCore::ScrollView::contentsToWindow(const WebCore::IntPoint&) const > Source/WebCore/platform/ScrollView.h:231:14: note: no known conversion for argument 1 from 'WebCore::LayoutRect' to 'const WebCore::IntPoint&' > Source/WebCore/platform/ScrollView.h:233:13: note: WebCore::IntRect WebCore::ScrollView::contentsToWindow(const WebCore::IntRect&) const > Source/WebCore/platform/ScrollView.h:233:13: note: no known conversion for argument 1 from 'WebCore::LayoutRect' to 'const WebCore::IntRect&' > > > note that getRect() also returned a LayoutRect. > > Well, the code was not compiling before either, but I guess it doesn't > make sense to split this change in two :) No, it's just that I was assuming the code compiled before :-) 870665 7 196833 cgarcia 2013-04-08 01:25:31 -0700 Comment on attachment 196833 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196833&action=review >> Source/WebKit/blackberry/Api/WebPage.cpp:2601 >> + return view->contentsToWindow(IntRect(node->boundingBox())); > > Why do you need the IntRect()? I wonder if what we really want here is pixelSnappedBoundingBox() here. > Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:1957 > - WebCore::IntRect elementRectInRootView = select->document()->view()->contentsToRootView(enclosingIntRect(select->getRect())); > + WebCore::IntRect elementRectInRootView = select->document()->view()->contentsToRootView(enclosingIntRect(select->boundingBox())); And same here. 873627 8 196833 xan.lopez 2013-04-11 02:08:53 -0700 Comment on attachment 196833 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196833&action=review r- because we should use different APIs and I think some of the code we are changing is dead. >>> Source/WebKit/blackberry/Api/WebPage.cpp:2601 >>> + return view->contentsToWindow(IntRect(node->boundingBox())); >> >> Why do you need the IntRect()? > > I wonder if what we really want here is pixelSnappedBoundingBox() here. If you read the code this seems to be used to check whether the bounding box intersects with the visible rectangle of another node in ::contextNode. Since the second rectangle is not using a snapped version, I think we should not use it here. > Source/WebKit/blackberry/Api/WebPage.cpp:5013 > } I cannot even find where this code is used. Seems to be dead? If that's so let's just remove it. >> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:1957 >> + WebCore::IntRect elementRectInRootView = select->document()->view()->contentsToRootView(enclosingIntRect(select->boundingBox())); > > And same here. Here we should use a pixel snapped version, since it is what Chromium does when calling this API. 896926 9 berto 2013-06-04 09:37:16 -0700 (In reply to comment #8) > > Source/WebKit/blackberry/Api/WebPage.cpp:5013 > > } > > I cannot even find where this code is used. Seems to be dead? If > that's so let's just remove it. You're right about this, I'll double check and file a separate bug. 966257 10 berto 2014-01-09 23:46:17 -0800 This bug is obsolete, closing. 196833 2013-04-08 00:16:16 -0700 2013-04-11 02:08:53 -0700 Patch 114146.diff text/plain 2750 berto ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9BcGkvV2ViUGFnZS5jcHAgYi9T b3VyY2UvV2ViS2l0L2JsYWNrYmVycnkvQXBpL1dlYlBhZ2UuY3BwCmluZGV4IGVhMzU1MDUuLjU1 YThhN2YgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9BcGkvV2ViUGFnZS5j cHAKKysrIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0FwaS9XZWJQYWdlLmNwcApAQCAtMjU5 OCw3ICsyNTk4LDcgQEAgc3RhdGljIEludFJlY3QgZ2V0Tm9kZVdpbmRvd1JlY3QoTm9kZSogbm9k ZSkKIHsKICAgICBpZiAoRnJhbWUqIGZyYW1lID0gZnJhbWVGb3JOb2RlKG5vZGUpKSB7CiAgICAg ICAgIGlmIChGcmFtZVZpZXcqIHZpZXcgPSBmcmFtZS0+dmlldygpKQotICAgICAgICAgICAgcmV0 dXJuIHZpZXctPmNvbnRlbnRzVG9XaW5kb3cobm9kZS0+Z2V0UmVjdCgpKTsKKyAgICAgICAgICAg IHJldHVybiB2aWV3LT5jb250ZW50c1RvV2luZG93KEludFJlY3Qobm9kZS0+Ym91bmRpbmdCb3go KSkpOwogICAgIH0KICAgICBBU1NFUlRfTk9UX1JFQUNIRUQoKTsKICAgICByZXR1cm4gSW50UmVj dCgpOwpAQCAtNTAwOCw3ICs1MDA4LDcgQEAgYm9vbCBXZWJQYWdlOjpnZXROb2RlUmVjdChjb25z dCBXZWJET01Ob2RlJiBub2RlLCBQbGF0Zm9ybTo6SW50UmVjdCYgcmVzdWx0KQogewogICAgIE5v ZGUqIG5vZGVJbXBsID0gbm9kZS5pbXBsKCk7CiAgICAgaWYgKG5vZGVJbXBsICYmIG5vZGVJbXBs LT5yZW5kZXJlcigpKSB7Ci0gICAgICAgIHJlc3VsdCA9IG5vZGVJbXBsLT5nZXRSZWN0KCk7Cisg ICAgICAgIHJlc3VsdCA9IG5vZGVJbXBsLT5ib3VuZGluZ0JveCgpOwogICAgICAgICByZXR1cm4g dHJ1ZTsKICAgICB9CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9DaGFu Z2VMb2cgYi9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVycnkvQ2hhbmdlTG9nCmluZGV4IGZmYTZhNjYu LjNjNTEyMjAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9DaGFuZ2VMb2cK KysrIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBA CisyMDEzLTA0LTA4ICBBbGJlcnRvIEdhcmNpYSAgPGFnYXJjaWFAaWdhbGlhLmNvbT4KKworICAg ICAgICBbQmxhY2tCZXJyeV0gUmVwbGFjZSBnZXRSZWN0KCkgd2l0aCBib3VuZGluZ0JveCgpCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTQxNDYKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIHdhcyBy ZW5hbWVkIGluIHIxMjgwMDYuCisKKyAgICAgICAgKiBBcGkvV2ViUGFnZS5jcHA6CisgICAgICAg IChCbGFja0JlcnJ5OjpXZWJLaXQ6OmdldE5vZGVXaW5kb3dSZWN0KToKKyAgICAgICAgKEJsYWNr QmVycnk6OldlYktpdDo6V2ViUGFnZTo6Z2V0Tm9kZVJlY3QpOgorICAgICAgICAqIFdlYktpdFN1 cHBvcnQvSW5wdXRIYW5kbGVyLmNwcDoKKyAgICAgICAgKEJsYWNrQmVycnk6OldlYktpdDo6SW5w dXRIYW5kbGVyOjpvcGVuU2VsZWN0UG9wdXApOgorCiAyMDEzLTA0LTA1ICBBbGJlcnRvIEdhcmNp YSAgPGFnYXJjaWFAaWdhbGlhLmNvbT4KIAogICAgICAgICBbQmxhY2tCZXJyeV0gUmVtb3ZlIHVu dXNlZCBwYXJhbWV0ZXJzIGZyb20gbm90aWZ5QW5pbWF0aW9uU3RhcnRlZCgpIGFuZCBwYWludENv bnRlbnRzKCkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9XZWJLaXRTdXBw b3J0L0lucHV0SGFuZGxlci5jcHAgYi9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVycnkvV2ViS2l0U3Vw cG9ydC9JbnB1dEhhbmRsZXIuY3BwCmluZGV4IDdkZGUyYjkuLjJkYjZjN2MgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9XZWJLaXRTdXBwb3J0L0lucHV0SGFuZGxlci5jcHAK KysrIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L1dlYktpdFN1cHBvcnQvSW5wdXRIYW5kbGVy LmNwcApAQCAtMTk1NCw3ICsxOTU0LDcgQEAgYm9vbCBJbnB1dEhhbmRsZXI6Om9wZW5TZWxlY3RQ b3B1cChIVE1MU2VsZWN0RWxlbWVudCogc2VsZWN0KQogICAgIH0KIAogICAgIFNlbGVjdFBvcHVw Q2xpZW50KiBzZWxlY3RDbGllbnQgPSBuZXcgU2VsZWN0UG9wdXBDbGllbnQobXVsdGlwbGUsIHNp emUsIGxhYmVscywgZW5hYmxlZHMsIGl0ZW1UeXBlcywgc2VsZWN0ZWRzLCBtX3dlYlBhZ2UsIHNl bGVjdCk7Ci0gICAgV2ViQ29yZTo6SW50UmVjdCBlbGVtZW50UmVjdEluUm9vdFZpZXcgPSBzZWxl Y3QtPmRvY3VtZW50KCktPnZpZXcoKS0+Y29udGVudHNUb1Jvb3RWaWV3KGVuY2xvc2luZ0ludFJl Y3Qoc2VsZWN0LT5nZXRSZWN0KCkpKTsKKyAgICBXZWJDb3JlOjpJbnRSZWN0IGVsZW1lbnRSZWN0 SW5Sb290VmlldyA9IHNlbGVjdC0+ZG9jdW1lbnQoKS0+dmlldygpLT5jb250ZW50c1RvUm9vdFZp ZXcoZW5jbG9zaW5nSW50UmVjdChzZWxlY3QtPmJvdW5kaW5nQm94KCkpKTsKICAgICAvLyBGYWls IHRvIGNyZWF0ZSBIVE1MIHBvcHVwLCB1c2UgdGhlIG9sZCBwYXRoCiAgICAgaWYgKCFtX3dlYlBh Z2UtPm1fcGFnZS0+Y2hyb21lKCktPmNsaWVudCgpLT5vcGVuUGFnZVBvcHVwKHNlbGVjdENsaWVu dCwgZWxlbWVudFJlY3RJblJvb3RWaWV3KSkKICAgICAgICAgbV93ZWJQYWdlLT5tX2NsaWVudC0+ b3BlblBvcHVwTGlzdChtdWx0aXBsZSwgc2l6ZSwgbGFiZWxzLCBlbmFibGVkcywgaXRlbVR5cGVz LCBzZWxlY3RlZHMpOwo=