Bug 183187 – Remove RenderElement::s_affectsParentBlock

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

This is a hack to track state change between styleWillChange and styleDidChange.

[Radar WebKit Bug Importer]

<rdar://problem/37961079>

[alan]

Created attachment 334712 [details]
Patch

[alan]

Created attachment 334724 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 334724 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:-97
> -bool RenderElement::s_affectsParentBlock = false;

Any reason why you do the removal of s_noLongerAffectsParentBlock in the separate bug 183196? Or it's just to be careful and do smaller changes?

> Source/WebCore/rendering/RenderElement.cpp:-773
> -        s_affectsParentBlock = false;

Normally you should remove braces per https://webkit.org/code-style-guidelines/#braces-one-line although I understand this else will eventually go away.

> Source/WebCore/rendering/RenderElement.cpp:812
> +        && parent() && (parent()->isRenderBlockFlow() || parent()->isRenderInline())) {

I wonder whether keeping a local affectsParentBlock variable would help making the meaning of this big conditional more explicit or whether the comment is enough.

[Antti Koivisto]

r=me

[alan]

(In reply to Frédéric Wang (:fredw) from comment #4)
> Comment on attachment 334724 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334724&action=review
> 
> > Source/WebCore/rendering/RenderElement.cpp:-97
> > -bool RenderElement::s_affectsParentBlock = false;
> 
> Any reason why you do the removal of s_noLongerAffectsParentBlock in the
> separate bug 183196? Or it's just to be careful and do smaller changes?
> 
just being careful (moving the s_noLongerAffectsParentBlock needs a bit more work since it has a bit of a dependency)

> > Source/WebCore/rendering/RenderElement.cpp:-773
> > -        s_affectsParentBlock = false;
> 
> Normally you should remove braces per
> https://webkit.org/code-style-guidelines/#braces-one-line although I
> understand this else will eventually go away.
Yeah, I'll remove the braces anyway.

> 
> > Source/WebCore/rendering/RenderElement.cpp:812
> > +        && parent() && (parent()->isRenderBlockFlow() || parent()->isRenderInline())) {
> 
> I wonder whether keeping a local affectsParentBlock variable would help
> making the meaning of this big conditional more explicit or whether the
> comment is enough.
I was thinking about that too but for some reason I decided not to. but now I am leaning towards having the variable there.

[alan]

(In reply to zalan from comment #6)
> (In reply to Frédéric Wang (:fredw) from comment #4)
> > Comment on attachment 334724 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=334724&action=review
> > 
> > > Source/WebCore/rendering/RenderElement.cpp:-97
> > > -bool RenderElement::s_affectsParentBlock = false;
> > 
> > Any reason why you do the removal of s_noLongerAffectsParentBlock in the
> > separate bug 183196? Or it's just to be careful and do smaller changes?
> > 
> just being careful (moving the s_noLongerAffectsParentBlock needs a bit more
> work since it has a bit of a dependency)
> 
> > > Source/WebCore/rendering/RenderElement.cpp:-773
> > > -        s_affectsParentBlock = false;
> > 
> > Normally you should remove braces per
> > https://webkit.org/code-style-guidelines/#braces-one-line although I
> > understand this else will eventually go away.
> Yeah, I'll remove the braces anyway.
> 
> > 
> > > Source/WebCore/rendering/RenderElement.cpp:812
> > > +        && parent() && (parent()->isRenderBlockFlow() || parent()->isRenderInline())) {
> > 
> > I wonder whether keeping a local affectsParentBlock variable would help
> > making the meaning of this big conditional more explicit or whether the
> > comment is enough.
> I was thinking about that too but for some reason I decided not to. but now
> I am leaning towards having the variable there.
Though this is all temporary anyway since the point of this exercise is to move the mutation logic to the tree updater.

[alan]

Created attachment 334744 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 334744 [details]
Patch

Clearing flags on attachment: 334744

Committed r229091: <https://trac.webkit.org/changeset/229091>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.