RenderLayerCompositor: Move implementation of simple methods into the header file.
Created attachment 324226 [details] Patch
Comment on attachment 324226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324226&action=review > Source/WebCore/ChangeLog:10 > + use RenderLayer.h and ChromeClient.h which are already required by RenderLayerCompositor.h Note to reviewers: I mean, I'm not sure whether it's a good idea to move the bodies of methods that actually require RenderLayer.h and ChromeClient.h. WDYT?
Comment on attachment 324226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324226&action=review > Source/WebCore/ChangeLog:3 > + RenderLayerCompositor: Move implementation of simple methods into the header file. I am OK moving these function bodies into the header file. But I personally would slightly prefer that these be kept as separate function bodies with the "inline" keyword rather than moving them into the class definition for four reasons: 1) Separate function bodies makes it easier to later refactor if we are reducing includes, moving them back into the implementation file removing "inline" or moving them into an inlines file. 2) Separate function bodies can cut down on distractions in the class definition and make it easier to focus on what the interface of the class is. It’s especially hard to read through a class if there are multi-line function implementations interspersed in the interface. 3) Separate function bodies avoid the need to comment out argument names for unused arguments. 4) Longer function bodies like useCoordinatedScrollingForLayer can turn into quite long lines that aren’t so easy to read. Of course, there are arguments in favor of putting the function definitions into the class definition: a) For short functions this is much more terse and keeps the overall source code smaller. b) For short functions having the definition right there can help make it clear exactly what a function does. But I personally think (1)-(4) outweigh (a)-(b). >> Source/WebCore/ChangeLog:10 >> + use RenderLayer.h and ChromeClient.h which are already required by RenderLayerCompositor.h > > Note to reviewers: I mean, I'm not sure whether it's a good idea to move the bodies of methods that actually require RenderLayer.h and ChromeClient.h. WDYT? I think it’s fine to either move these function bodies into the header, or to do the work to remove the need to include RenderLayer.h and ChromeClient.h. I don’t think we need to leave them out of the header just because of the possibility that some day we want to remove the includes.
Comment on attachment 324226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324226&action=review >> Source/WebCore/ChangeLog:3 >> + RenderLayerCompositor: Move implementation of simple methods into the header file. > > I am OK moving these function bodies into the header file. > > But I personally would slightly prefer that these be kept as separate function bodies with the "inline" keyword rather than moving them into the class definition for four reasons: > > 1) Separate function bodies makes it easier to later refactor if we are reducing includes, moving them back into the implementation file removing "inline" or moving them into an inlines file. > > 2) Separate function bodies can cut down on distractions in the class definition and make it easier to focus on what the interface of the class is. It’s especially hard to read through a class if there are multi-line function implementations interspersed in the interface. > > 3) Separate function bodies avoid the need to comment out argument names for unused arguments. > > 4) Longer function bodies like useCoordinatedScrollingForLayer can turn into quite long lines that aren’t so easy to read. > > Of course, there are arguments in favor of putting the function definitions into the class definition: > > a) For short functions this is much more terse and keeps the overall source code smaller. > > b) For short functions having the definition right there can help make it clear exactly what a function does. > > But I personally think (1)-(4) outweigh (a)-(b). Thanks. I was also not quite sure about this. However, I believe that for the straightforward setter/getter of class members like scrollLayer() or setTracksRepaints(), (1)-(4) do not really apply. So I guess I'll upload a simplified patch to handle only these cases and use "inline" for other functions.
Comment on attachment 324226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324226&action=review I don't really see the point of this kind of change unless you've identified that functions need to inline for performance reasons. > Source/WebCore/rendering/RenderLayerCompositor.h:339 > + WEBCORE_EXPORT void startTrackingLayerFlushes() { m_layerFlushCount = 0; } > + WEBCORE_EXPORT unsigned layerFlushCount() const { return m_layerFlushCount; } You can't export an inline function. > Source/WebCore/rendering/RenderLayerCompositor.h:342 > + WEBCORE_EXPORT void startTrackingCompositingUpdates() { m_compositingUpdateCount = 0; } > + WEBCORE_EXPORT unsigned compositingUpdateCount() const { return m_compositingUpdateCount; } Ditto.
(In reply to Simon Fraser (smfr) from comment #5) > Comment on attachment 324226 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324226&action=review > > I don't really see the point of this kind of change unless you've identified > that functions need to inline for performance reasons. What Darin said in (a)-(b). As I said, I'll probably restrict the change to straigthforward setters/getters. Or otherwise WONTFIX this...
Created attachment 324367 [details] Patch for landing
Comment on attachment 324367 [details] Patch for landing Clearing flags on attachment: 324367 Committed r223749: <https://trac.webkit.org/changeset/223749>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35568754>