[webkit-reviews] review granted: [Bug 178514] RenderLayerCompositor: Move implementation of simple methods into the header file. : [Attachment 324226] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 178514: RenderLayerCompositor: Move implementation of simple methods into
the header file.
https://bugs.webkit.org/show_bug.cgi?id=178514

Attachment 324226: Patch

https://bugs.webkit.org/attachment.cgi?id=324226&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 324226
  --> https://bugs.webkit.org/attachment.cgi?id=324226
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.