[webkit-reviews] review granted: [Bug 20180] Firefox is faster than webkit on :nth-child tests on SlickSpeed : [Attachment 23337] Addresses a few pseudoreview comments

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Smith <catfish.man at gmail.com>'s
request for review:
Bug 20180: Firefox is faster than webkit on :nth-child tests on SlickSpeed
https://bugs.webkit.org/show_bug.cgi?id=20180

Attachment 23337: Addresses a few pseudoreview comments
https://bugs.webkit.org/attachment.cgi?id=23337&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This is a great fix!

+    virtual RenderStyle* privateRenderStyle() const;

In other places I've called this sort of function virtualXXX, such as
virtualFirstChild. But also, this function isn't really just a private
renderStyle() function. It's specifically renderStyle() when m_renderer is 0.
I'd suggest a name more like nonRendererRenderStyle().

I'm a little sad that we have to add a whole header just for a single function.
Is there a better name for that header that could allow us to use it for more
things in the future? It's not scalable to have a separate header for each
function.

+    virtual RenderStyle* privateRenderStyle() const { return m_style; }
     String m_value;

I'd like to see a blank line here between the functions and data members.

It's not so great to have these virtual functions defined in the class
definition. There's no reason to inline them.

It's a little strange to have this 10% speedup be a patch on a bug saying
"Firefox is faster". I'd prefer to not have bugs where the titles are are
literally about competitive comparisons, although I think it's great to compare
us with other engines to figure out what we should work on. Does this actually
make WebKit faster than some version of Gecko? Could we retitle the bug or
something.

r=me, but please consider my suggestions