Bug 120397

Created attachment 209857 [details] patch

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.

Created attachment 210433 [details] Patch 2 For a region, generatingNode() will always return an Element, i uploaded a new patch.

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.

Created attachment 210466 [details] Patch 3

(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 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?

(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.

Created attachment 210582 [details] Patch for landing

(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 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

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

Created attachment 210594 [details] Patch for landing

(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 on attachment 210594 [details] Patch for landing Clearing flags on attachment: 210594 Committed r155109: <http://trac.webkit.org/changeset/155109>

All reviewed patches have been landed. Closing bug.