Bug 122963

Summary: [CSSRegions] Display anonymous regions in DRT
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: achicu, cdumez, commit-queue, eflews.bot, esprehn+autocc, glenn, gtk-ews, gyuyoung.kim, kondapallykalyan, mibalan, rakuco, WebkitBugTracker, xan.lopez
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
none
Patch 2
achicu: review-
Patch 3
eflews.bot: commit-queue-
Patch 4
none
Patch 5
none
Patch for landing none

Description Mihnea Ovidenie 2013-10-17 07:11:15 PDT
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.
Comment 1 Mihnea Ovidenie 2013-10-23 06:17:56 PDT
Created attachment 214951 [details]
Patch
Comment 2 WebKit Commit Bot 2013-10-23 06:20:51 PDT
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.
Comment 3 Mihnea Ovidenie 2013-10-23 06:31:31 PDT
Created attachment 214953 [details]
Patch 2

Fix TestExpectations.
Comment 4 Alexandru Chiculita 2013-10-24 11:22:03 PDT
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.
Comment 5 Mihnea Ovidenie 2013-10-25 04:31:06 PDT
Created attachment 215161 [details]
Patch 3
Comment 6 Mihnea Ovidenie 2013-10-25 04:34:29 PDT
(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 :)
Comment 7 EFL EWS Bot 2013-10-25 04:35:38 PDT
Comment on attachment 215161 [details]
Patch 3

Attachment 215161 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/12308029
Comment 8 EFL EWS Bot 2013-10-25 04:52:12 PDT
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
Comment 9 kov's GTK+ EWS bot 2013-10-25 04:53:18 PDT
Comment on attachment 215161 [details]
Patch 3

Attachment 215161 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/12258025
Comment 10 Mihnea Ovidenie 2013-10-25 05:03:31 PDT
Created attachment 215164 [details]
Patch 4
Comment 11 Alexandru Chiculita 2013-10-25 09:38:33 PDT
(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 :)
Comment 12 Mihnea Ovidenie 2013-10-25 10:14:14 PDT
(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 :)
Comment 13 Mihnea Ovidenie 2013-10-28 02:30:45 PDT
Created attachment 215291 [details]
Patch 5
Comment 14 Alexandru Chiculita 2013-10-28 11:07:12 PDT
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.
Comment 15 Mihnea Ovidenie 2013-10-29 00:51:00 PDT
Created attachment 215370 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2013-10-29 01:22:14 PDT
Comment on attachment 215370 [details]
Patch for landing

Clearing flags on attachment: 215370

Committed r158184: <http://trac.webkit.org/changeset/158184>
Comment 17 WebKit Commit Bot 2013-10-29 01:22:17 PDT
All reviewed patches have been landed.  Closing bug.