Improve RAII of StyleResolverState.

Improve RAII of StyleResolverState.

This patch ensures that the correct RenderStyle and parent is provided upon
construction of StyleResolverState, and *removes* the possibility of these
pointers changing from the outside at arbitrary times. This makes construction
of member objects easier, removes unnecessary 'setStyle' methods, and reduces
elements of surprise in general.

Also, StyleResolverState::parentStyle is now never nullptr for the lifetime
of the object.

Changes:

 * The parentNode-style-if-null logic in the StyleResolverState constructor
   has been moved to ElementResolveContext. This makes it possible to use the
   result of this logic without constructing a StyleResolverState.
 * StyleBasisContext has been added as a subclass of ElementResolveContext.
   It contains additional context related to animation, inheritance and
   pseudo elements.

R=rune@opera.com

[andersr, 2014-10-28 12:52:40]

Note that this patch currently contains
https://codereview.chromium.org/686723002/ , because I want to use the trybots
right away.

I totally understand if you want to delay the review until 686723002 lands, and
I rebase this.

[andersr, 2014-10-28 13:12:33]

Hmm, maybe it's better to just modify ElementResolveContext.

[andersr, 2014-10-28 14:54:41]

Patch #1 was ugly, sorry. ;)

Patch #3 should be better:

 * Extend ElementResolveContext instead of adding a new class with approximately
the same purpose.
 * Let StyleResolverState.cpp::createStyleFromContext figure out which
RenderStyle to initialize with, that's not the job of a "Context" object.

Problem:

I kind of feel like StyleResolverState::defaultStyleForElement is doing too much
for a "State" object. At the same time, it sounds reasonable and proper that
StyleResolverState should be able to initialize itself given a
ElementResolveContext, in which case defaultStyleForElement must necessarily be
available to StyleResolverState somehow. I doesn't have any side-effects, at
least, so it's probably OK.

[rune, 2014-10-29 12:01:15]

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
File Source/core/css/resolver/FontBuilder.h (right):

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
Source/core/css/resolver/FontBuilder.h:44: FontBuilder(const Document&,
RenderStyle*);
Seems like the FontBuilder construction change is pretty unrelated and is big
enough to have a CL of its own.

[rune, 2014-10-29 12:15:21]

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.h (left):

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.h:60: void setRootStyle(const
RenderStyle* rootStyle) { m_rootStyle = rootStyle; }
These were used unused?

[rune, 2014-10-29 13:01:52]

On 2014/10/28 at 12:52:40, andersr wrote:
> Note that this patch currently contains
https://codereview.chromium.org/686723002/ , because I want to use the trybots
right away.

That's the link to this CL.

[rune, 2014-10-29 13:02:14]

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
File Source/core/css/resolver/ElementResolveContext.h (right):

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
Source/core/css/resolver/ElementResolveContext.h:39:
ElementResolveContext(Document&, Element*, RenderStyle* parentStyle);
I liked it better when the construction for Document style was more explicit
without the Element parameter.

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolverState.cpp:41: }
I'm unsure what to think about this. It feels too magical to have this
RenderStyle creation logic in the StyleResolverState constructor.

[andersr, 2014-10-29 13:23:39]

On 2014/10/29 13:01:52, rune wrote:
> On 2014/10/28 at 12:52:40, andersr wrote:
> > Note that this patch currently contains
> https://codereview.chromium.org/686723002/ , because I want to use the trybots
> right away.
> 
> That's the link to this CL.

I meant: https://codereview.chromium.org/670373002/

[andersr, 2014-10-29 13:29:47]

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.h (left):

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.h:60: void setRootStyle(const
RenderStyle* rootStyle) { m_rootStyle = rootStyle; }
On 2014/10/29 12:15:21, rune wrote:
> These were used unused?

setStyle was used in StyleResolverState::setStyle, but no longer, as of this
patch.

setRootStyle was unused even before this patch.

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
File Source/core/css/resolver/ElementResolveContext.h (right):

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
Source/core/css/resolver/ElementResolveContext.h:39:
ElementResolveContext(Document&, Element*, RenderStyle* parentStyle);
On 2014/10/29 13:02:14, rune wrote:
> I liked it better when the construction for Document style was more explicit
> without the Element parameter.

OK.

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/686723002/diff/70001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolverState.cpp:41: }
On 2014/10/29 13:02:14, rune wrote:
> I'm unsure what to think about this. It feels too magical to have this
> RenderStyle creation logic in the StyleResolverState constructor.

I agree. We can't do this anyway, because of the pseudoElement horribleness.

[andersr, 2014-10-30 13:31:40]

Rebased, dropped [WIP], rewrote description, ... rewrote patch.

PTAL.

[rune, 2014-10-30 15:23:51]

I've done an initial pass over this. Everything outside of StyleResolver.h/cpp
looks good, I think. I need to look through StyleResolver again. My first
impression is that there are too many contexts.

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
File Source/core/css/resolver/FontBuilder.cpp (right):

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
Source/core/css/resolver/FontBuilder.cpp:65: , m_style(style)
It looks like m_style should never be null. ASSERT(style)?

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:959: StyleResolverState
state(document(), nullptr, RenderStyle::create());
I don't think you need a StyleResolverState here. Can't you just use a
FontBuilder instead?

Btw, defaultStyleForElement is a bad name. I think initialStyle() or
initialRenderStyle() is better as default is used as a term when UA style is
included.

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolverState.cpp (left):

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolverState.cpp:42: {
ASSERT(style)
ASSERT(parentStyle)
?

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolverState.cpp:57: {
ASSERT(style)
ASSERT(parentStyle)
?

[andersr, 2014-10-30 16:06:44]

On 2014/10/30 15:23:51, rune wrote:
> I've done an initial pass over this. Everything outside of StyleResolver.h/cpp
> looks good, I think. I need to look through StyleResolver again. My first
> impression is that there are too many contexts.

I removed AnimationResolveContext, and let StyleBasisContext be a subclass of
ElementResolveContext. This way, there is no extra noise in StyleResolver
associated with accessing ElementResolveContext methods.

[andersr, 2014-10-31 13:02:15]

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
File Source/core/css/resolver/FontBuilder.cpp (right):

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
Source/core/css/resolver/FontBuilder.cpp:65: , m_style(style)
On 2014/10/30 15:23:50, rune wrote:
> It looks like m_style should never be null. ASSERT(style)?

Done.

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:959: StyleResolverState
state(document(), nullptr, RenderStyle::create());
On 2014/10/30 15:23:50, rune wrote:
> I don't think you need a StyleResolverState here. Can't you just use a
> FontBuilder instead?
> 
> Btw, defaultStyleForElement is a bad name. I think initialStyle() or
> initialRenderStyle() is better as default is used as a term when UA style is
> included.
> 

Done.

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolverState.cpp (left):

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolverState.cpp:42: {
On 2014/10/30 15:23:50, rune wrote:
> ASSERT(style)
> ASSERT(parentStyle)
> ?

Done.

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/686723002/diff/110001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolverState.cpp:57: {
On 2014/10/30 15:23:50, rune wrote:
> ASSERT(style)
> ASSERT(parentStyle)
> ?

Done.

[rune, 2014-11-13 14:28:43]

https://codereview.chromium.org/686723002/diff/190001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/686723002/diff/190001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:131: class StyleBasisContext : public
ElementResolveContext {
I don't like the name StyleBasisContext. This is still a context for an element.
I think the name should reflect that.

How about calling the base class something like ElementStyleContext and this
class ElementResolveContext?

https://codereview.chromium.org/686723002/diff/190001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:755: return nullptr;
Would it be better to do the clone() here instead of returning a nullptr, and
then skip the cloning in the StyleResolverState constructors?

https://codereview.chromium.org/686723002/diff/190001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/686723002/diff/190001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolverState.h:49: StyleResolverState(Document&,
Element*, PassRefPtr<RenderStyle>, RenderStyle* parentStyle = nullptr);
How about making the parentStyle parameter mandatory non-null? It needs to be
non-null after construction regardlessly, and I think the logic for which parent
style to use in that case should be decided by the caller.

[andersr, 2014-11-13 14:57:20]

https://codereview.chromium.org/686723002/diff/190001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/686723002/diff/190001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:131: class StyleBasisContext : public
ElementResolveContext {
On 2014/11/13 14:28:42, rune wrote:
> I don't like the name StyleBasisContext. This is still a context for an
element.
> I think the name should reflect that.
> 
> How about calling the base class something like ElementStyleContext and this
> class ElementResolveContext?

Yeah, that's pretty good.

https://codereview.chromium.org/686723002/diff/190001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:755: return nullptr;
On 2014/11/13 14:28:42, rune wrote:
> Would it be better to do the clone() here instead of returning a nullptr, and
> then skip the cloning in the StyleResolverState constructors?

I wanted to avoid this verbosity at each call site, which would then be
necessary:

RefPtr<RenderStyle> style = styleForContext(styleContext);
RefPtr<RenderStyle> parentStyle = parentStyleForContext(styleContext,
style.get());
StyleResolverState state(document(), styleContext, style.release(),
parentStyle.release());

But I guess it's more proper.

[andersr, 2014-11-21 15:19:11]

This does not lgtm to me anymore. I'm not sure this actually constitutes a net
improvement, even after fixing the resolved issues.