[webkit-reviews] review denied: [Bug 93629] Reduce Font.h includes across project -- improves RenderObject.h compile time : [Attachment 158056] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied nbhargava at google.com's request for
review:
Bug 93629: Reduce Font.h includes across project -- improves RenderObject.h
compile time
https://bugs.webkit.org/show_bug.cgi?id=93629

Attachment 158056: Patch
https://bugs.webkit.org/attachment.cgi?id=158056&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=158056&action=review


So what have we learned about the performance impact of removing all this
inlining?

I suggest we make a first patch that just removes the inlining. Then we can
follow up with the patch that makes all the include changes to get the build
time improvements.

> Source/WebCore/rendering/style/RenderStyle.h:1107
> +    bool setFontDescription(const FontDescription& v);

Argument names should be omitted in cases like this.

> Source/WebCore/rendering/style/RenderStyle.h:1112
> +    void setColor(const Color& v);

And this.

> Source/WebCore/rendering/style/RenderStyle.h:1140
> +    void setWordSpacing(int v);
> +    void setLetterSpacing(int v);

And these.

> Source/WebCore/rendering/style/RenderStyle.h:1185
> +    void setListStyleImage(PassRefPtr<StyleImage> v);

Here too.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:68
>> +const FontMetrics& RenderStyle::fontMetrics() const { return
inherited->font.fontMetrics(); }
>> +const Font& RenderStyle::font() const { return inherited->font; }
> 
> Huh?	Does WebProcess not include RenderSTyle.cpp?

Yes, as Eric is implying, this code change is wrong. The correct fix is to add
exports to WebCore.exp.in rather than copying WebCore code into WebKit2.