Bug 131425 – Make page overlay functionality working on coordinated graphics.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Hyowon Kim]

Page overlay functionality is not working on coordinated graphics since r166975.

[Hyowon Kim]

Created attachment 229229 [details]
Patch

[Hyowon Kim]

Created attachment 229235 [details]
Patch

[Darin Adler]

Comment on attachment 229235 [details]
Patch

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.

[Hyowon Kim]

Created attachment 229270 [details]
patch for landing

[Hyowon Kim]

(In reply to comment #3)
> (From update of attachment 229235 [details])
> 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.

Oops. Done.

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.

Done.
 
> > 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?

It's a temporary solution. These functions are not called anymore.
But LayerTreeHostGTK still derives them from LayerTreeHost.
They would be removed if GTK port fixes this issue too.

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

Okay. Undo adding const&.

Thank you for the review!

[Jae Hyun Park]

(In reply to comment #4)
> Created an attachment (id=229270) [details]
> patch for landing

This may not be my place, but Darin's comments are not applied in the patch you uploaded for landing.
Also, in WebKit2/ChangeLog, file/function changes are written 3 times.

I think you may have uploaded the wrong patch.

[Hyowon Kim]

Created attachment 229336 [details]
patch for landing

[Hyowon Kim]

(In reply to comment #6)
> (In reply to comment #4)
> > Created an attachment (id=229270) [details] [details]
> > patch for landing
> 
> This may not be my place, but Darin's comments are not applied in the patch you uploaded for landing.
> Also, in WebKit2/ChangeLog, file/function changes are written 3 times.
> 
> I think you may have uploaded the wrong patch.

Thank you for letting me know.

[WebKit Commit Bot]

Comment on attachment 229336 [details]
patch for landing

Clearing flags on attachment: 229336

Committed r167340: <http://trac.webkit.org/changeset/167340>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.