[webkit-reviews] review denied: [Bug 129471] [iOS][WK2] Pages with Graphics Layers cause giant tiling surfaces on zoom : [Attachment 225439] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 129471: [iOS][WK2] Pages with Graphics Layers cause giant tiling surfaces
on zoom
https://bugs.webkit.org/show_bug.cgi?id=129471

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225439&action=review


Code is OK but I really want to see some comments to differentiate exposed rect
from visibleContentRect. You're adding more rects; we need to document them.

> Source/WebCore/ChangeLog:18
> +	   To solve this, this patch switch from the exposedRect API the
generic concept

to the generic

> Source/WebCore/ChangeLog:25
> +	   The case with inside frame is untested due to stability issues :(.

File a bug?

> Source/WebCore/platform/ScrollView.h:174
> +    void setVisibleExtentContentRect(const IntRect&);

This should have a comment saying that it only applies to platforms without a
platform widget. Maybe it should even ASSERT that.

> Source/WebCore/platform/ios/ScrollViewIOS.mm:134
> +    IntRect rootViewExtentContentRect =
rootView->visibleExtentContentRect();
> +    IntRect selfExtentContentRect =
rootViewToContents(rootViewExtentContentRect);
> +    selfExtentContentRect.intersect(boundsRect());

This isn't applying the clipping from all enclosing frames, and it should I
think, eventually.

> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:97
> +    virtual void setVisibleExtentContentRect(const WebCore::FloatRect&) = 0;


Where is the comment saying how this differs from exposedRect()?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1649
> +    float floatBoundedScale = static_cast<float>(boundedScale);

The explicit cast doesn't seem worth it.