[webkit-reviews] review granted: [Bug 131425] Make page overlay functionality working on coordinated graphics. : [Attachment 229235] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Hyowon Kim <hw1008.kim at samsung.com>'s
request for review:
Bug 131425: Make page overlay functionality working on coordinated graphics.
https://bugs.webkit.org/show_bug.cgi?id=131425

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229235&action=review


>
Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:
75
> +void CompositingCoordinator::setRootCompositingLayer(GraphicsLayer*
CompositingLayer, GraphicsLayer* overlayLayer)

CompositingLayer should not be capitalized, since it’s an argument name.

>
Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:67

> +    void setRootCompositingLayer(GraphicsLayer*, GraphicsLayer*);

Not clear what these two arguments are, so one or both need names here in the
header.

> Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:79
> -    virtual void didInstallPageOverlay(PageOverlay*) = 0;
> -    virtual void didUninstallPageOverlay(PageOverlay*) = 0;
> -    virtual void setPageOverlayNeedsDisplay(PageOverlay*, const
WebCore::IntRect&) = 0;
> +    virtual void didInstallPageOverlay(PageOverlay*) { }
> +    virtual void didUninstallPageOverlay(PageOverlay*) { }
> +    virtual void setPageOverlayNeedsDisplay(PageOverlay*, const
WebCore::IntRect&) { }

Not sure this is a good change. Is this really better than putting the empty
functions in the derived class?

> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:224
> -void PageOverlayController::flushPageOverlayLayers(FloatRect visibleRect)
> +void PageOverlayController::flushPageOverlayLayers(const FloatRect&
visibleRect)

Not sure this is a good change. I know that we save a copy here, but Anders has
been discouraging us from using const& everywhere.