Bug 122963 – [CSSRegions] Display anonymous regions in DRT

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihnea Ovidenie]

As a follow up after https://bugs.webkit.org/show_bug.cgi?id=119135, DRT should also display the anonymous region (RenderNamedFlowFragment) inside a block that has flow-flow style.

[Mihnea Ovidenie]

Created attachment 214951 [details]
Patch

[WebKit Commit Bot]

Attachment 214951 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/regions/auto-size/autoheight-regions-mark-expected.txt', u'LayoutTests/fast/regions/flows-dependency-dynamic-remove-expected.txt', u'LayoutTests/fast/regions/flows-dependency-same-flow-expected.png', u'LayoutTests/fast/regions/flows-dependency-same-flow-expected.txt', u'LayoutTests/fast/regions/iframe-html-collected-expected.txt', u'LayoutTests/fast/repaint/overflow-flipped-writing-mode-block-in-regions-expected.txt', u'LayoutTests/fast/repaint/region-painting-via-layout-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/efl/fast/regions/multiple-directionality-changes-in-variable-width-regions-expected.txt', u'LayoutTests/platform/efl/fast/regions/region-dynamic-after-before-expected.txt', u'LayoutTests/platform/efl/fast/regions/region-generated-content-before-after-expected.txt', u'LayoutTests/platform/efl/fast/regions/text-region-split-small-pagination-expected.txt', u'LayoutTests/platform/efl/fast/repaint/japanese-rl-selection-repaint-in-regions-expected.txt', u'LayoutTests/platform/efl/fast/repaint/line-flow-with-floats-in-regions-expected.png', u'LayoutTests/platform/efl/fast/repaint/line-flow-with-floats-in-regions-expected.txt', u'LayoutTests/platform/efl/fast/repaint/overflow-flipped-writing-mode-block-in-regions-expected.png', u'LayoutTests/platform/efl/fast/repaint/region-painting-invalidation-expected.txt', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/gtk/fast/regions/region-dynamic-after-before-expected.txt', u'LayoutTests/platform/gtk/fast/regions/region-generated-content-before-after-expected.txt', u'LayoutTests/platform/gtk/fast/repaint/line-flow-with-floats-in-regions-expected.png', u'LayoutTests/platform/gtk/fast/repaint/line-flow-with-floats-in-regions-expected.txt', u'LayoutTests/platform/gtk/fast/repaint/overflow-flipped-writing-mode-block-in-regions-expected.png', u'LayoutTests/platform/mac/fast/regions/auto-size/autoheight-regions-mark-expected.png', u'LayoutTests/platform/mac/fast/regions/auto-size/autoheight-regions-mark-expected.txt', u'LayoutTests/platform/mac/fast/regions/autoheight-regions-mark-expected.png', u'LayoutTests/platform/mac/fast/regions/flows-dependency-dynamic-remove-expected.txt', u'LayoutTests/platform/mac/fast/regions/flows-dependency-same-flow-expected.png', u'LayoutTests/platform/mac/fast/regions/flows-dependency-same-flow-expected.txt', u'LayoutTests/platform/mac/fast/regions/iframe-html-collected-expected.png', u'LayoutTests/platform/mac/fast/regions/iframe-html-collected-expected.txt', u'LayoutTests/platform/mac/fast/regions/multiple-directionality-changes-in-variable-width-regions-expected.txt', u'LayoutTests/platform/mac/fast/regions/region-dynamic-after-before-expected.txt', u'LayoutTests/platform/mac/fast/regions/region-generated-content-before-after-expected.txt', u'LayoutTests/platform/mac/fast/regions/text-region-split-small-pagination-expected.txt', u'LayoutTests/platform/mac/fast/regions/top-overflow-out-of-second-region-expected.txt', u'LayoutTests/platform/mac/fast/repaint/japanese-rl-selection-repaint-in-regions-expected.png', u'LayoutTests/platform/mac/fast/repaint/japanese-rl-selection-repaint-in-regions-expected.txt', u'LayoutTests/platform/mac/fast/repaint/line-flow-with-floats-in-regions-expected.txt', u'LayoutTests/platform/mac/fast/repaint/overflow-flipped-writing-mode-block-in-regions-expected.txt', u'LayoutTests/platform/mac/fast/repaint/region-painting-invalidation-expected.txt', u'LayoutTests/platform/mac/fast/repaint/region-painting-via-layout-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderNamedFlowFragment.h', u'Source/WebCore/rendering/RenderTreeAsText.cpp']" exit_code: 1
LayoutTests/platform/efl/TestExpectations:1583:  expecting "[", "#", or end of line instead of "[Missing]"  [test/expectations] [5]
LayoutTests/platform/efl/TestExpectations:1584:  expecting "[", "#", or end of line instead of "[Missing]"  [test/expectations] [5]
LayoutTests/platform/efl/TestExpectations:1585:  expecting "[", "#", or end of line instead of "[Missing]"  [test/expectations] [5]
LayoutTests/platform/efl/TestExpectations:1586:  expecting "[", "#", or end of line instead of "[Missing]"  [test/expectations] [5]
LayoutTests/platform/efl/TestExpectations:1587:  expecting "[", "#", or end of line instead of "[Missing]"  [test/expectations] [5]
LayoutTests/platform/efl/TestExpectations:1588:  expecting "[", "#", or end of line instead of "[Missing]"  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:55:  expecting "[", "#", or end of line instead of "[Missing]"  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:56:  expecting "[", "#", or end of line instead of "[Missing]"  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:57:  expecting "[", "#", or end of line instead of "[Missing]"  [test/expectations] [5]
Total errors found: 9 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mihnea Ovidenie]

Created attachment 214953 [details]
Patch 2

Fix TestExpectations.

[Alexandru Chiculita]

Comment on attachment 214953 [details]
Patch 2

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

Thanks Mihnea. I have a couple comments below.

> Source/WebCore/rendering/RenderTreeAsText.cpp:664
> +static void writeRenderNamedFlowRegionList(const RenderRegionList& flowThreadRegionList, TextStream& ts, int indent)

I guess that the previous name was better in case we need to reuse the function for other types of RenderFlowThreads.

> Source/WebCore/rendering/RenderTreeAsText.cpp:667
> +        RenderNamedFlowFragment* region = toRenderNamedFlowFragment(*itRR);

There's no need to cast to RenderNamedFlowFragment.

> Source/WebCore/rendering/RenderTreeAsText.cpp:669
> +        ts << "RenderNamedFlowFragment";

We can use the renderer name function instead.

> Source/WebCore/rendering/RenderTreeAsText.cpp:670
> +        if (region->generatingElement()) {

region->generatingElement() is used a lot of times. Might be better to put it on the stack.

> Source/WebCore/rendering/RenderTreeAsText.cpp:717
>          if (!validRegionsList.isEmpty() || !invalidRegionsList.isEmpty()) {

You can remove this "if" statement. The condition is duplicated in the nested ifs.

[Mihnea Ovidenie]

Created attachment 215161 [details]
Patch 3

[Mihnea Ovidenie]

(In reply to comment #4)
> (From update of attachment 214953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214953&action=review
> 
> Thanks Mihnea. I have a couple comments below.
> 
> > Source/WebCore/rendering/RenderTreeAsText.cpp:664
> > +static void writeRenderNamedFlowRegionList(const RenderRegionList& flowThreadRegionList, TextStream& ts, int indent)
> 
> I guess that the previous name was better in case we need to reuse the function for other types of RenderFlowThreads.
> 

I kept the original name and i added a virtual method - dumpRegion - in RenderRegion and in RenderNamedFlowFragment. The method dumps the region properties in a TextStream.

> > Source/WebCore/rendering/RenderTreeAsText.cpp:667
> > +        RenderNamedFlowFragment* region = toRenderNamedFlowFragment(*itRR);
> 
> There's no need to cast to RenderNamedFlowFragment.
> 

I wanted this cast in order to make sure that these regions are in fact RenderNamedFlowFragments. With the new approach, there is no need for that.

> > Source/WebCore/rendering/RenderTreeAsText.cpp:669
> > +        ts << "RenderNamedFlowFragment";
> 
> We can use the renderer name function instead.
> 

Done.

> > Source/WebCore/rendering/RenderTreeAsText.cpp:670
> > +        if (region->generatingElement()) {
> 
> region->generatingElement() is used a lot of times. Might be better to put it on the stack.
> 

Done, both in RenderRegion::dumpRegion and in RenderNamedFlowFragment::dumpRegion.

> > Source/WebCore/rendering/RenderTreeAsText.cpp:717
> >          if (!validRegionsList.isEmpty() || !invalidRegionsList.isEmpty()) {
> 
> You can remove this "if" statement. The condition is duplicated in the nested ifs.

Right :)

[EFL EWS Bot]

Comment on attachment 215161 [details]
Patch 3

Attachment 215161 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/12308029

[EFL EWS Bot]

Comment on attachment 215161 [details]
Patch 3

Attachment 215161 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/12268034

[kov's GTK+ EWS bot]

Comment on attachment 215161 [details]
Patch 3

Attachment 215161 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/12258025

[Mihnea Ovidenie]

Created attachment 215164 [details]
Patch 4

[Alexandru Chiculita]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 214953 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=214953&action=review
> > 
> > Thanks Mihnea. I have a couple comments below.
> > 
> > > Source/WebCore/rendering/RenderTreeAsText.cpp:664
> > > +static void writeRenderNamedFlowRegionList(const RenderRegionList& flowThreadRegionList, TextStream& ts, int indent)
> > 
> > I guess that the previous name was better in case we need to reuse the function for other types of RenderFlowThreads.
> > 
> 
> I kept the original name and i added a virtual method - dumpRegion - in RenderRegion and in RenderNamedFlowFragment. The method dumps the region properties in a TextStream.
> 
> > > Source/WebCore/rendering/RenderTreeAsText.cpp:667
> > > +        RenderNamedFlowFragment* region = toRenderNamedFlowFragment(*itRR);
> > 
> > There's no need to cast to RenderNamedFlowFragment.
> > 
> 
> I wanted this cast in order to make sure that these regions are in fact RenderNamedFlowFragments. With the new approach, there is no need for that.
I guess an assert would be better to check for things, but I would not put that in dump render tree. I guess the check must live somewhere in RenderNamedFlowThread if it is not there already.

I still don't get why you need to duplicate the code. I think that the more complicated function can handle both normal RenderRegions and the new one without casting it. You don't access any of the new functions in the new class.

> 
> > > Source/WebCore/rendering/RenderTreeAsText.cpp:669
> > > +        ts << "RenderNamedFlowFragment";
> > 
> > We can use the renderer name function instead.
> > 
> 
> Done.
> 
> > > Source/WebCore/rendering/RenderTreeAsText.cpp:670
> > > +        if (region->generatingElement()) {
> > 
> > region->generatingElement() is used a lot of times. Might be better to put it on the stack.
> > 
> 
> Done, both in RenderRegion::dumpRegion and in RenderNamedFlowFragment::dumpRegion.
> 
> > > Source/WebCore/rendering/RenderTreeAsText.cpp:717
> > >          if (!validRegionsList.isEmpty() || !invalidRegionsList.isEmpty()) {
> > 
> > You can remove this "if" statement. The condition is duplicated in the nested ifs.
> 
> Right :)

[Mihnea Ovidenie]

(In reply to comment #11)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (From update of attachment 214953 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=214953&action=review
> > > 
> > > Thanks Mihnea. I have a couple comments below.
> > > 
> > > > Source/WebCore/rendering/RenderTreeAsText.cpp:664
> > > > +static void writeRenderNamedFlowRegionList(const RenderRegionList& flowThreadRegionList, TextStream& ts, int indent)
> > > 
> > > I guess that the previous name was better in case we need to reuse the function for other types of RenderFlowThreads.
> > > 
> > 
> > I kept the original name and i added a virtual method - dumpRegion - in RenderRegion and in RenderNamedFlowFragment. The method dumps the region properties in a TextStream.
> > 
> > > > Source/WebCore/rendering/RenderTreeAsText.cpp:667
> > > > +        RenderNamedFlowFragment* region = toRenderNamedFlowFragment(*itRR);
> > > 
> > > There's no need to cast to RenderNamedFlowFragment.
> > > 
> > 
> > I wanted this cast in order to make sure that these regions are in fact RenderNamedFlowFragments. With the new approach, there is no need for that.
> I guess an assert would be better to check for things, but I would not put that in dump render tree. I guess the check must live somewhere in RenderNamedFlowThread if it is not there already.
> 
> I still don't get why you need to duplicate the code. I think that the more complicated function can handle both normal RenderRegions and the new one without casting it. You don't access any of the new functions in the new class.
> 

Ok :) i will post a new patch.

> > 
> > > > Source/WebCore/rendering/RenderTreeAsText.cpp:669
> > > > +        ts << "RenderNamedFlowFragment";
> > > 
> > > We can use the renderer name function instead.
> > > 
> > 
> > Done.
> > 
> > > > Source/WebCore/rendering/RenderTreeAsText.cpp:670
> > > > +        if (region->generatingElement()) {
> > > 
> > > region->generatingElement() is used a lot of times. Might be better to put it on the stack.
> > > 
> > 
> > Done, both in RenderRegion::dumpRegion and in RenderNamedFlowFragment::dumpRegion.
> > 
> > > > Source/WebCore/rendering/RenderTreeAsText.cpp:717
> > > >          if (!validRegionsList.isEmpty() || !invalidRegionsList.isEmpty()) {
> > > 
> > > You can remove this "if" statement. The condition is duplicated in the nested ifs.
> > 
> > Right :)

[Mihnea Ovidenie]

Created attachment 215291 [details]
Patch 5

[Alexandru Chiculita]

Comment on attachment 215291 [details]
Patch 5

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

Thanks Mihnea!

> Source/WebCore/rendering/RenderTreeAsText.cpp:703
> +        writeIndent(ts, indent + 3);

nit: I think the +3 should happen in the caller method.

> Source/WebCore/rendering/RenderTreeAsText.cpp:715
> +                ts << " (anonymous child of)";

nit: I guess ")" should come at the end after the generated element is printed out.

[Mihnea Ovidenie]

Created attachment 215370 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 215370 [details]
Patch for landing

Clearing flags on attachment: 215370

Committed r158184: <http://trac.webkit.org/changeset/158184>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.