Bug 120397

Summary: Replace node() calls with generatingNode() for RenderRegion code
Product: WebKit Reporter: Catalin badea <badea>
Component: WebCore Misc.Assignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kangil.han, kondapallykalyan, macpherson, menard, mihnea, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
darin: review+
Patch 2
none
Patch 3
none
Patch for landing
none
Archive of layout-test-results from webkit-cq-01 for mac-mountainlion
none
Patch for landing none

Description Catalin badea 2013-08-28 01:42:31 PDT
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.
Comment 1 Catalin badea 2013-08-28 01:52:34 PDT
Created attachment 209857 [details]
patch
Comment 2 Darin Adler 2013-08-28 09:37:44 PDT
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.
Comment 3 Mihnea Ovidenie 2013-09-03 23:34:32 PDT
Created attachment 210433 [details]
Patch 2

For a region, generatingNode() will always return an Element, i uploaded a new patch.
Comment 4 Darin Adler 2013-09-04 08:00:36 PDT
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.
Comment 5 Mihnea Ovidenie 2013-09-04 09:09:20 PDT
Created attachment 210466 [details]
Patch 3
Comment 6 Mihnea Ovidenie 2013-09-04 09:10:05 PDT
(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.
Comment 7 Darin Adler 2013-09-04 09:24:10 PDT
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?
Comment 8 Mihnea Ovidenie 2013-09-04 11:20:33 PDT
(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.
Comment 9 Mihnea Ovidenie 2013-09-04 23:54:45 PDT
Created attachment 210582 [details]
Patch for landing
Comment 10 Mihnea Ovidenie 2013-09-04 23:59:27 PDT
(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?
Comment 11 WebKit Commit Bot 2013-09-05 00:39:22 PDT
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
Comment 12 WebKit Commit Bot 2013-09-05 00:39:24 PDT
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
Comment 13 Mihnea Ovidenie 2013-09-05 03:01:44 PDT
Created attachment 210594 [details]
Patch for landing
Comment 14 Mihnea Ovidenie 2013-09-05 03:02:25 PDT
(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.
Comment 15 WebKit Commit Bot 2013-09-05 03:32:02 PDT
Comment on attachment 210594 [details]
Patch for landing

Clearing flags on attachment: 210594

Committed r155109: <http://trac.webkit.org/changeset/155109>
Comment 16 WebKit Commit Bot 2013-09-05 03:32:05 PDT
All reviewed patches have been landed.  Closing bug.