Bug 114146 – [BlackBerry] Replace getRect() with boundingBox()

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit BlackBerry

[Alberto Garcia]

This was renamed in r128006:

http://trac.webkit.org/changeset/128006/trunk/Source/WebCore/dom/Node.h

[Alberto Garcia]

Created attachment 196833 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 196833 [details]
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()?

[Alberto Garcia]

(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.

[Carlos Garcia Campos]

(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.

[Alberto Garcia]

(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 :)

[Carlos Garcia Campos]

(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 :-)

[Carlos Garcia Campos]

Comment on attachment 196833 [details]
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.

[Xan Lopez]

Comment on attachment 196833 [details]
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.

[Alberto Garcia]

(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.

[Alberto Garcia]

This bug is obsolete, closing.