Bug 120397 – Replace node() calls with generatingNode() for RenderRegion code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Catalin badea]

Change node() calls to generatingNode() calls for RenderRegion. This is need because regions will become anonymous blocks nested inside elements with the flow-from property.

[Catalin badea]

Created attachment 209857 [details]
patch

[Darin Adler]

Comment on attachment 209857 [details]
patch

Does generatingNode have to return a Node*? Can it ever be a non-Element? If not, then it should return an Element* and we should not have all these calls to toElement.

[Mihnea Ovidenie]

Created attachment 210433 [details]
Patch 2

For a region, generatingNode() will always return an Element, i uploaded a new patch.

[Darin Adler]

Comment on attachment 210433 [details]
Patch 2

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

> Source/WebCore/rendering/RenderRegion.h:147
> +    Element* generatingNodeForRegion() const;

This should be inlined; we don’t want function call overhead for what should be a free typecast. This should be called generatingElement, not generatedNodeForRegion.

[Mihnea Ovidenie]

Created attachment 210466 [details]
Patch 3

[Mihnea Ovidenie]

(In reply to comment #4)
> (From update of attachment 210433 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210433&action=review
> 
> > Source/WebCore/rendering/RenderRegion.h:147
> > +    Element* generatingNodeForRegion() const;
> 
> This should be inlined; we don’t want function call overhead for what should be a free typecast. This should be called generatingElement, not generatedNodeForRegion.

Done, thanks for the name, generatingElement is much better than my solution.

[Darin Adler]

Comment on attachment 210466 [details]
Patch 3

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

> Source/WebCore/rendering/RenderRegion.h:40
>  struct LayerFragment;
>  typedef Vector<LayerFragment, 1> LayerFragments;

These should not be in the same paragraph as the straight class definitions. (Can fix that later, just something I noticed).

> Source/WebCore/rendering/RenderRegion.h:156
> +    Element* generatingElement() const { return toElement(RenderObject::generatingNode()); }

Can this be zero? If not, it should return a reference.

> Source/WebCore/rendering/RenderTreeAsText.cpp:673
> +            String tagName = getTagName(renderRegion->node());

Why using node() here instead of generatingElement or generatingNode?

[Mihnea Ovidenie]

(In reply to comment #7)
> (From update of attachment 210466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210466&action=review
> 
> > Source/WebCore/rendering/RenderRegion.h:40
> >  struct LayerFragment;
> >  typedef Vector<LayerFragment, 1> LayerFragments;
> 
> These should not be in the same paragraph as the straight class definitions. (Can fix that later, just something I noticed).

I will that later.

> 
> > Source/WebCore/rendering/RenderRegion.h:156
> > +    Element* generatingElement() const { return toElement(RenderObject::generatingNode()); }
> 
> Can this be zero? If not, it should return a reference.

It should not be 0, i can't think of any situation.

> 
> > Source/WebCore/rendering/RenderTreeAsText.cpp:673
> > +            String tagName = getTagName(renderRegion->node());
> 
> Why using node() here instead of generatingElement or generatingNode?

Because for pseudo-elements as regions i want pseudo to be printed instead of div, to have an indication that this is a region from a pseudo-element. I had a note in the second patch Changelog for that.

[Mihnea Ovidenie]

Created attachment 210582 [details]
Patch for landing

[Mihnea Ovidenie]

(In reply to comment #7)
> (From update of attachment 210466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210466&action=review
> 
> > Source/WebCore/rendering/RenderRegion.h:40
> >  struct LayerFragment;
> >  typedef Vector<LayerFragment, 1> LayerFragments;
> 
> These should not be in the same paragraph as the straight class definitions. (Can fix that later, just something I noticed).
> 
> > Source/WebCore/rendering/RenderRegion.h:156
> > +    Element* generatingElement() const { return toElement(RenderObject::generatingNode()); }
> 
> Can this be zero? If not, it should return a reference.
> 

Filed https://bugs.webkit.org/show_bug.cgi?id=120757 for this

> > Source/WebCore/rendering/RenderTreeAsText.cpp:673
> > +            String tagName = getTagName(renderRegion->node());
> 
> Why using node() here instead of generatingElement or generatingNode?

[WebKit Commit Bot]

Comment on attachment 210582 [details]
Patch for landing

Rejecting attachment 210582 [details] from commit-queue.

New failing tests:
fast/workers/termination-with-port-messages.html
Full output: http://webkit-queues.appspot.com/results/1695714

[WebKit Commit Bot]

Created attachment 210585 [details]
Archive of layout-test-results from webkit-cq-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Mihnea Ovidenie]

Created attachment 210594 [details]
Patch for landing

[Mihnea Ovidenie]

(In reply to comment #11)
> (From update of attachment 210582 [details])
> Rejecting attachment 210582 [details] from commit-queue.
> 
> New failing tests:
> fast/workers/termination-with-port-messages.html
> Full output: http://webkit-queues.appspot.com/results/1695714

Unrelated, trying again.

[WebKit Commit Bot]

Comment on attachment 210594 [details]
Patch for landing

Clearing flags on attachment: 210594

Committed r155109: <http://trac.webkit.org/changeset/155109>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.