Bug 120397 - Replace node() calls with generatingNode() for RenderRegion code
Summary: Replace node() calls with generatingNode() for RenderRegion code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-28 01:42 PDT by Catalin badea
Modified: 2013-09-05 03:32 PDT (History)
9 users (show)

See Also:


Attachments
patch (5.19 KB, patch)
2013-08-28 01:52 PDT, Catalin badea
darin: review+
Details | Formatted Diff | Diff
Patch 2 (7.91 KB, patch)
2013-09-03 23:34 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 3 (7.24 KB, patch)
2013-09-04 09:09 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (7.72 KB, patch)
2013-09-04 23:54 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-01 for mac-mountainlion (683.03 KB, application/zip)
2013-09-05 00:39 PDT, WebKit Commit Bot
no flags Details
Patch for landing (7.74 KB, patch)
2013-09-05 03:01 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.