Bug 178514 – RenderLayerCompositor: Move implementation of simple methods into the header file.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Frédéric Wang (:fredw)]

RenderLayerCompositor: Move implementation of simple methods into the header file.

[Frédéric Wang (:fredw)]

Created attachment 324226 [details]
Patch

[Frédéric Wang (:fredw)]

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?

[Darin Adler]

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.

[Frédéric Wang (:fredw)]

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.

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 324367 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 324367 [details]
Patch for landing

Clearing flags on attachment: 324367

Committed r223749: <https://trac.webkit.org/changeset/223749>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35568754>