Clear StyleResolverState after each resolve.

Clear StyleResolverState after each resolve.

This makes sure RenderStyles aren't kept alive unnecessarily by the
two RefPtr<RenderStyles> inside there.  However, it's not enough to
make sure that *all* RenderStyles are destroyed eagerly.

BUG=172011
R=eseidel@chromium.org, esprehn@chromium.org

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

[Jeffrey Yasskin, 2013-07-11 21:24:05]

At
https://codereview.chromium.org/18371008/diff/2001/Source/core/rendering/styl...,
I'm using the destruction of a RenderStyle object to notify the Blink embedder
when no more elements match a given selector. My test
(https://codereview.chromium.org/19045002/diff/1/chrome/browser/extensions/api...)
initially failed because the StyleResolverState was keeping a reference to the
last RenderStyle it created and its parent style. This change lets the test pass
reliably, but probably doesn't make RenderStyle destruction fully deterministic.

This change is probably beneficial even without my larger change
("declarativeContent") since it'll slightly reduce memory use.

Bugs in declarativeContent from this are likely to be subtle but also
low-severity: this particular CL would make a page action go away 60s sooner in
the unlikely case that an extension is watching the last element on a page.

If you search http://grokweb-chromium/ for
"cpp:WTF::class-RefPtr<cpp:WebCore::class-RenderStyle@TYPE>", you can see the
list of uses I'm planning to go through to check if they can delay RenderStyle
destruction, but I'll chase those after you have a chance to consider whether
this goal is good at all.

https://codereview.chromium.org/5216392399814656/diff/5629499534213120/Source...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/5629499534213120/Source...
Source/core/css/resolver/StyleResolver.cpp:1155:
m_state.initForStyleResolve(document(), e);
This initForStyleResolve in a loop looks weird to me, since the later
styleForKeyframe() calls appear to use the state that gets set up here, without
re-initializing it.

https://codereview.chromium.org/5216392399814656/diff/5629499534213120/Source...
Source/core/css/resolver/StyleResolver.cpp:1230: state.clear();
This .clear() on an early return is a good sign that we want an RAII/scoped
object instead of explicit clear()s, but some of the other uses make that
tricky. The scope would likely start at an initForStyleResolve() call and would
auto-clear the state.

[esprehn, 2013-07-11 21:30:35]

Can we just add a new method like finishStyleResolve() that does this? I don't
particularly like takeAndClear(). I'd rather you just added a clear() call or
RAII thinger to each method.

[Jeffrey Yasskin, 2013-07-11 21:52:29]

On 2013/07/11 21:30:35, esprehn wrote:
> Can we just add a new method like finishStyleResolve() that does this? I don't
> particularly like takeAndClear(). I'd rather you just added a clear() call or
> RAII thinger to each method.

Sounds fine. I think the only thing preventing me from doing an RAII thinger
easily is the initForStyleResolve() in a loop at
https://codereview.chromium.org/5216392399814656/diff/5629499534213120/Source...,
and the lack of initForStyleResolve in the two ifs later in that function. Would
it make sense to move that initForStyleResolve out of the loop or replicate it
in the two ifs?

finishStyleResolve() sounds like a fine name if I can't get an RAII thinger
working. I assume it'd do the same thing as my current takeAndClear() method? Or
would you want it to just .clear() and have people take() manually?

[esprehn, 2013-07-11 22:12:42]

On 2013/07/11 21:52:29, Jeffrey Yasskin wrote:
> On 2013/07/11 21:30:35, esprehn wrote:
> > Can we just add a new method like finishStyleResolve() that does this? I
don't
> > particularly like takeAndClear(). I'd rather you just added a clear() call
or
> > RAII thinger to each method.
> 
> Sounds fine. I think the only thing preventing me from doing an RAII thinger
> easily is the initForStyleResolve() in a loop at
>
https://codereview.chromium.org/5216392399814656/diff/5629499534213120/Source...,
> and the lack of initForStyleResolve in the two ifs later in that function.
Would
> it make sense to move that initForStyleResolve out of the loop or replicate it
> in the two ifs?

You should put that initForStyleResolve inside styleForKeyframe and put the RAII
thinger in there I think? I think it does need to be in the loop so the flags
get reset between style recalcs for each keyframe, maybe we don't do that
properly right now.

> 
> finishStyleResolve() sounds like a fine name if I can't get an RAII thinger
> working. I assume it'd do the same thing as my current takeAndClear() method?
Or
> would you want it to just .clear() and have people take() manually?

It could do what yours does right now. I just wanted it more clear that this is
the last thing you should do.

[Jeffrey Yasskin, 2013-07-12 00:59:40]

To fix the failing tests, I've refactored applyPropertyToStyle and its two
callers in FontLoader and CanvasRenderingContext2D.

On 2013/07/11 22:12:42, esprehn wrote:
> On 2013/07/11 21:52:29, Jeffrey Yasskin wrote:
> > On 2013/07/11 21:30:35, esprehn wrote:
> > > Can we just add a new method like finishStyleResolve() that does this? I
> don't
> > > particularly like takeAndClear(). I'd rather you just added a clear() call
> or
> > > RAII thinger to each method.
> > 
> > Sounds fine. I think the only thing preventing me from doing an RAII thinger
> > easily is the initForStyleResolve() in a loop at
> >
>
https://codereview.chromium.org/5216392399814656/diff/5629499534213120/Source...,
> > and the lack of initForStyleResolve in the two ifs later in that function.
> Would
> > it make sense to move that initForStyleResolve out of the loop or replicate
it
> > in the two ifs?
> 
> You should put that initForStyleResolve inside styleForKeyframe and put the
RAII
> thinger in there I think? I think it does need to be in the loop so the flags
> get reset between style recalcs for each keyframe, maybe we don't do that
> properly right now.

Ok, all done. The net effect is to add two initForStyleResolve()s for the 0% and
100% keyframes, which were missing them before.

> > finishStyleResolve() sounds like a fine name if I can't get an RAII thinger
> > working. I assume it'd do the same thing as my current takeAndClear()
method?
> Or
> > would you want it to just .clear() and have people take() manually?
> 
> It could do what yours does right now. I just wanted it more clear that this
is
> the last thing you should do.

With the RAII class, I can just remove takeAndClear(), so I haven't done
anything to add finishStyleResolve().

[dglazkov, 2013-07-12 16:45:47]

This change is definitely net-positive! It's starting to get largish, but it
incrementally helps in isolating effects.

I am okay with this if esprehn is.

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
Source/core/css/resolver/StyleResolver.cpp:1079: PassRefPtr<RenderStyle>
StyleResolver::styleForKeyframe(Element* e, const RenderStyle* elementStyle,
const StyleKeyframe* keyframe, KeyframeValue& keyframeValue)
we can get elementStyle from element, right? Why pass both of them?

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
Source/core/css/resolver/StyleResolver.h:249: struct PropertyValue {
Does this have to be nested? I just tried to un-nest all of the other ones.

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
Source/core/css/resolver/StyleResolverState.h:64: class ScopedStyleResolution {
Can we un-nest this?

[Jeffrey Yasskin, 2013-07-12 19:41:27]

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
Source/core/css/resolver/StyleResolver.cpp:1079: PassRefPtr<RenderStyle>
StyleResolver::styleForKeyframe(Element* e, const RenderStyle* elementStyle,
const StyleKeyframe* keyframe, KeyframeValue& keyframeValue)
On 2013/07/12 16:45:47, Dimitri Glazkov wrote:
> we can get elementStyle from element, right? Why pass both of them?

keyframeStylesForAnimation takes both rather than deriving elementStyle from
element, and that separation goes up the call chain at least as far as 
CompositeAnimation::updateKeyframeAnimations.

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
Source/core/css/resolver/StyleResolver.h:249: struct PropertyValue {
On 2013/07/12 16:45:47, Dimitri Glazkov wrote:
> Does this have to be nested? I just tried to un-nest all of the other ones.

Doesn't need to be, although the name should be more descriptive when un-nested.
CSSPropertyValue?

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/5216392399814656/diff/14001/Source/core/css/r...
Source/core/css/resolver/StyleResolverState.h:64: class ScopedStyleResolution {
On 2013/07/12 16:45:47, Dimitri Glazkov wrote:
> Can we un-nest this?

Yep, done.

[Jeffrey Yasskin, 2013-07-12 19:58:27]

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/r...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/r...
Source/core/css/resolver/StyleResolverState.h:86: class ScopedStyleResolution {
In a subsequent change, we could remove this entirely, in favor of constructing
a new StyleResolverState on the stack for each resolution and storing a pointer
to it in m_state. I don't want to do that in this change because it could have
performance implications (https://bugs.webkit.org/show_bug.cgi?id=109909).

[esprehn, 2013-07-12 20:15:27]

LGTM. This is really pretty. Couple random comments

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/F...
File Source/core/css/FontLoader.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/F...
Source/core/css/FontLoader.cpp:351:
styleResolver->applyPropertiesToStyle(properties, arraysize(properties),
style.get());
Can we do this now? We usually use WTF_ARRAY_SIZE. What is arraysize?

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/r...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/r...
Source/core/css/resolver/StyleResolverState.cpp:50:
ScopedStyleResolution::ScopedStyleResolution(StyleResolverState* state,
Document* document, Element* e, RenderStyle* parentStyle, RenderRegion*
regionForStyling)
It's funny, I would have preferred this as a nested class like 

StyleResolverState::Scope scope;

This is fine though

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/html/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/html/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1994:
CSSPropertyValue(CSSPropertyFontFamily, *parsedStyle),
Do you need to use the CSSPropertyValue syntax, can't you do initialization with
the array syntax?

properties[] = {
  { CSSPropertyFontFamily, *parsedStyle },
  { ... }
};

[Jeffrey Yasskin, 2013-07-12 20:49:08]

Thanks for the review!

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/F...
File Source/core/css/FontLoader.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/F...
Source/core/css/FontLoader.cpp:351:
styleResolver->applyPropertiesToStyle(properties, arraysize(properties),
style.get());
On 2013/07/12 20:15:27, esprehn wrote:
> Can we do this now? We usually use WTF_ARRAY_SIZE. What is arraysize?

I see ~7 uses in WebKit, although codesearch seems to think they come from
chrome's base/.
(https://code.google.com/p/chromium/codesearch/#search/&sq=package:chromium&q=...

I don't see WTF_ARRAY_SIZE at all. There's ARRAY_SIZE
(https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...),
but it's only used once and isn't as safe.

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/r...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/r...
Source/core/css/resolver/StyleResolverState.cpp:50:
ScopedStyleResolution::ScopedStyleResolution(StyleResolverState* state,
Document* document, Element* e, RenderStyle* parentStyle, RenderRegion*
regionForStyling)
On 2013/07/12 20:15:27, esprehn wrote:
> It's funny, I would have preferred this as a nested class like 
> 
> StyleResolverState::Scope scope;
> 
> This is fine though

'k; leaving it this way then. :)

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/html/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/html/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1994:
CSSPropertyValue(CSSPropertyFontFamily, *parsedStyle),
On 2013/07/12 20:15:27, esprehn wrote:
> Do you need to use the CSSPropertyValue syntax, can't you do initialization
with
> the array syntax?
> 
> properties[] = {
>   { CSSPropertyFontFamily, *parsedStyle },
>   { ... }
> };

I do need the the CSSPropertyValue(...) syntax because this constructor actually
does something and we're not using C++11 yet. I could remove both constructors
(to make the class an "aggregate") and use { CSSPropertyFontFamily,
parsedStyle->getPropertyCSSValue(CSSPropertyFontFamily).get()}, but I don't
think that's better.

[commit-bot: I haz the power, 2013-07-12 21:10:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/5216392399814656...

[Jeffrey Yasskin, 2013-07-12 21:10:12]

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/F...
File Source/core/css/FontLoader.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/25001/Source/core/css/F...
Source/core/css/FontLoader.cpp:351:
styleResolver->applyPropertiesToStyle(properties, arraysize(properties),
style.get());
On 2013/07/12 20:49:08, Jeffrey Yasskin wrote:
> On 2013/07/12 20:15:27, esprehn wrote:
> > Can we do this now? We usually use WTF_ARRAY_SIZE. What is arraysize?
> 
> I see ~7 uses in WebKit, although codesearch seems to think they come from
> chrome's base/.
>
(https://code.google.com/p/chromium/codesearch/#search/&sq=package:chromium&q=...)
> 
> I don't see WTF_ARRAY_SIZE at all. There's ARRAY_SIZE
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...),
> but it's only used once and isn't as safe.

Switched to WTF_ARRAY_LENGTH.

[eseidel, 2013-07-12 22:04:18]

lgtm

I really appreciate you making the extra effort to join the StyleResolver
sanification process with this patch.

Thanks again Jeffrey.

[eseidel, 2013-07-12 22:05:42]

https://codereview.chromium.org/5216392399814656/diff/36001/Source/core/css/r...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/36001/Source/core/css/r...
Source/core/css/resolver/StyleResolver.cpp:1017: ScopedStyleResolution
resolution(&state, document(), element, defaultParent, regionForStyling);
This seems somewhat confusingly named.  ScopedStyles are a separate concept from
what you're doing here, or?

[commit-bot: I haz the power, 2013-07-12 22:23:13]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[Jeffrey Yasskin, 2013-07-12 22:59:05]

https://codereview.chromium.org/5216392399814656/diff/36001/Source/core/css/r...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/5216392399814656/diff/36001/Source/core/css/r...
Source/core/css/resolver/StyleResolver.cpp:1017: ScopedStyleResolution
resolution(&state, document(), element, defaultParent, regionForStyling);
On 2013/07/12 22:05:42, eseidel wrote:
> This seems somewhat confusingly named.  ScopedStyles are a separate concept
from
> what you're doing here, or?

Yeah; switched to StyleResolveScope. I'm hopeful that I can make
StyleResolverState a local object in the next change, so the exact name for this
type may not matter that much.

[commit-bot: I haz the power, 2013-07-12 22:59:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/5216392399814656...

[commit-bot: I haz the power, 2013-07-12 23:18:42]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-07-12 23:46:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/5216392399814656...

[commit-bot: I haz the power, 2013-07-13 00:58:48]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-07-13 06:23:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/5216392399814656...

[commit-bot: I haz the power, 2013-07-13 07:30:27]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[eseidel, 2013-07-13 07:57:56]

Going to update this after levi's change and see if I can land it to unblock
additional work on this file.

[eseidel, 2013-07-13 08:02:51]

Committed patchset #8 manually as r154162 (presubmit successful).

[eseidel, 2013-07-13 08:03:15]

I updated the patch, built and ran the layout tests locally.  Thanks again for
the patch!

[Jeffrey Yasskin, 2013-07-13 23:06:41]

On 2013/07/13 07:57:56, eseidel wrote:
> Going to update this after levi's change and see if I can land it to unblock
> additional work on this file.

Sounds good; thanks for the commit!