[webkit-reviews] review denied: [Bug 78517] [BlackBerry] Upstream backing store related classes : [Attachment 126972] Patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 78517: [BlackBerry] Upstream backing store related classes
https://bugs.webkit.org/show_bug.cgi?id=78517

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

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126972&action=review


one more round needed.

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.cpp:43
> +static inline WebCore::IntSize pointToSize(const WebCore::IntPoint& point)

unneeded webcore:: here and elsewhere in the patch.

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.cpp:144
> +    if (!isMainFrame()) {
> +	   // It is possible that the owner HTML element has been removed at
this point,
> +	   // especially when the frame is loading a JavaScript URL.
> +	   if (Element* elt = m_frame->ownerElement()) {
> +	       if (RenderBox* obj = elt->renderBox())
> +		   rect.move(obj->borderLeft() + obj->paddingLeft(),
obj->borderTop() + obj->paddingTop());
> +	   }
> +    }
> +
> +    WebCore::Frame* frame = m_frame;
> +    while (frame) {
> +	   if (Element* element = static_cast<Element*>(frame->ownerElement()))
{
> +	       do {
> +		   rect.move(element->offsetLeft(), element->offsetTop());
> +	       } while ((element = element->offsetParent()));
> +	   }
> +
> +	   if ((frame = frame->tree()->parent()))
> +	       rect.move((-frame->view()->scrollOffset()));
> +    }

we can certainly improve this/speed it up, but in a follow up. I think I had
done that already.

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.h:93
> +    bool isLoading() const;
> +    WebPagePrivate::LoadState loadState() const;

these methods should not belong here, but that is another story...

> Source/WebKit/blackberry/WebKitSupport/BackingStoreClient.h:115
> +    // FIXME: Leaving the below lines commented out as a reference for us to
soon be sure if we need these
> +    // methods and class variables be moved from WebPage to
BackingStoreClient.
> +    // Notification methods that deliver changes to the real geometry of the
device as specified above.
> +    // void notifyTransformChanged();
> +    // void notifyTransformedContentsSizeChanged();
> +    // void notifyTransformedScrollChanged();
> +    // m_overflowExceedsContentsSize = true;

hummmmm....

> Source/WebKit/blackberry/WebKitSupport/BackingStoreCompositingSurface.h:21
> +#ifndef BackingStoreCompositingSurface_h
> +#define BackingStoreCompositingSurface_h
> +

is this the same as leo's patch? with the internal changes/clean ups he did
yesterday?