[webkit-reviews] review denied: [Bug 114146] [BlackBerry] Replace getRect() with boundingBox() : [Attachment 196833] Patch

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Alberto Garcia
<agarcia at igalia.com>'s request for review:
Bug 114146: [BlackBerry] Replace getRect() with boundingBox()
https://bugs.webkit.org/show_bug.cgi?id=114146

Attachment 196833: Patch
https://bugs.webkit.org/attachment.cgi?id=196833&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
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->boundin
gBox()));
> 
> And same here.

Here we should use a pixel snapped version, since it is what Chromium does when
calling this API.