Don't assume so easily that RenderStyle data can be shared.

Don't assume so easily that RenderStyle data can be shared.

An element can only share RenderStyle data with another element if
BOTH are cacheable. The element that's already in the cache is
obviously cacheable, but we also need to figure out whether the
element that wants to use data from the cache is cachable.

Additionally, get rid of the Element* argument from
applyMatchedProperties(), since it's already part of the state object
passed to that method.

BUG=236329
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154999

[eae, 2013-07-26 15:26:08]

LGTM

[commit-bot: I haz the power, 2013-07-26 15:26:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/20560008/1

[commit-bot: I haz the power, 2013-07-26 17:47:47]

Change committed as 154999

[ojan, 2013-07-28 17:42:31]

https://codereview.chromium.org/20560008/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/20560008/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1202: &&
MatchedPropertiesCache::isCacheable(element, state.style(),
state.parentStyle())) {
Not a big deal, but it'd be nice if we saved the isCacheable bool so we don't
have to call this method twice. It would also simplify the if statements below a
bit.

bool isCacheable = cacheHash && MatchedPropertiesCache::isCacheable(element,
state.style(), state.parentStyle());

Then, down below, the early return just becomes:
if (cachedMatchedProperties || !isCacheable)

Given how much we call applyMatchedProperties, I could imagine doing all the
branching in MatchedPropertiesCache::isCacheable could end up mattering, but
even if that's not the case, I think the code is nicer this way.

[mstensho (USE GERRIT), 2013-07-29 08:29:26]

On 2013/07/28 17:42:31, ojan wrote:
>
https://codereview.chromium.org/20560008/diff/1/Source/core/css/resolver/Styl...
> File Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/20560008/diff/1/Source/core/css/resolver/Styl...
> Source/core/css/resolver/StyleResolver.cpp:1202: &&
> MatchedPropertiesCache::isCacheable(element, state.style(),
> state.parentStyle())) {
> Not a big deal, but it'd be nice if we saved the isCacheable bool so we don't
> have to call this method twice. It would also simplify the if statements below
a
> bit.
> 
> bool isCacheable = cacheHash && MatchedPropertiesCache::isCacheable(element,
> state.style(), state.parentStyle());
> 
> Then, down below, the early return just becomes:
> if (cachedMatchedProperties || !isCacheable)
> 
> Given how much we call applyMatchedProperties, I could imagine doing all the
> branching in MatchedPropertiesCache::isCacheable could end up mattering, but
> even if that's not the case, I think the code is nicer this way.

That sounds good to me as well, but this patch has already landed. Is there
anything I should do about this now?

[ojan, 2013-07-29 17:11:50]

On 2013/07/29 08:29:26, Morten Stenshorne wrote:
> That sounds good to me as well, but this patch has already landed. Is there
> anything I should do about this now?

A followup patch if you're willing. Uploaded to a new issue.

[mstensho (USE GERRIT), 2013-07-29 19:54:56]

On 2013/07/29 17:11:50, ojan wrote:
> On 2013/07/29 08:29:26, Morten Stenshorne wrote:
> > That sounds good to me as well, but this patch has already landed. Is there
> > anything I should do about this now?
> 
> A followup patch if you're willing. Uploaded to a new issue.

Sure - will do. Assuming that I should re-use the bug number, though.

[ojan, 2013-07-29 20:10:51]

On 2013/07/29 19:54:56, Morten Stenshorne wrote:
> On 2013/07/29 17:11:50, ojan wrote:
> > On 2013/07/29 08:29:26, Morten Stenshorne wrote:
> > > That sounds good to me as well, but this patch has already landed. Is
there
> > > anything I should do about this now?
> > 
> > A followup patch if you're willing. Uploaded to a new issue.
> 
> Sure - will do. Assuming that I should re-use the bug number, though.

Yup. Thanks.