Summary: | Make page overlay functionality working on coordinated graphics. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hyowon Kim <hw1008.kim> | ||||||||||
Component: | Layout and Rendering | Assignee: | Hyowon Kim <hw1008.kim> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bunhere, cmarcelo, commit-queue, jaepark, kondapallykalyan, luiz, noam, ossy, sergio, thorton, zeno | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 131353 | ||||||||||||
Attachments: |
|
Description
Hyowon Kim
2014-04-08 20:56:00 PDT
Created attachment 229229 [details]
Patch
Created attachment 229235 [details]
Patch
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. Created attachment 229270 [details]
patch for landing
(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! (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. Created attachment 229336 [details]
patch for landing
(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. Comment on attachment 229336 [details] patch for landing Clearing flags on attachment: 229336 Committed r167340: <http://trac.webkit.org/changeset/167340> All reviewed patches have been landed. Closing bug. |