Bug 101070 – Move m_element checks out of canShareStyle into locateSharedStyle

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Move m_element checks out of canShareStyle into locateSharedStyle

[Ojan Vafai]

Created attachment 172082 [details]
Patch

[Ojan Vafai]

Just stumbled across this as I was looking at canShareStyleWithElement earlier today.

[Darin Adler]

Comment on attachment 172082 [details]
Patch

Looks logical. Ideally we would measure the performance improvement to also make sure there is no performance loss. It’s conceivable that this could make things slower in some cases even though it should make most things faster.

[Eric Seidel (no email)]

canShareStyle* is quite hot (on the bechnmarks where we've gotten rid of more egregiously slow code.  So anything we can do to make it faster is good. :)

I trust our perf bots (http://webkit-perf.appspot.com/ and http://build.chromium.org/f/chromium/perf/dashboard/overview.html) will tell us if we did anything wrong. :)

[Ojan Vafai]

Yeah. I don't actually expect this to have a meaningful performance impact in practice. I'll monitor the chromium perf dashboards though to be sure.

[Ojan Vafai]

Committed r133315: <http://trac.webkit.org/changeset/133315>

[Darin Adler]

Comment on attachment 172082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172082&action=review

> Source/WebCore/css/StyleResolver.cpp:1217
> -        if (!m_element->hasTagName(progressTag))
> -            return false;
> -
> +        ASSERT(!m_element->hasTagName(progressTag));

Wait, what? Is this assertion backwards?

[Darin Adler]

Comment on attachment 172082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172082&action=review

>> Source/WebCore/css/StyleResolver.cpp:1217
>> +        ASSERT(!m_element->hasTagName(progressTag));
> 
> Wait, what? Is this assertion backwards?

Oh, I see you noticed and fixed this later.