Bug 122963

Created attachment 214951 [details] Patch

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.

Created attachment 214953 [details] Patch 2 Fix TestExpectations.

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.

Created attachment 215161 [details] Patch 3

(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 on attachment 215161 [details] Patch 3 Attachment 215161 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/12308029

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 on attachment 215161 [details] Patch 3 Attachment 215161 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/12258025

Created attachment 215164 [details] Patch 4

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

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

Created attachment 215291 [details] Patch 5

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.

Created attachment 215370 [details] Patch for landing

Comment on attachment 215370 [details] Patch for landing Clearing flags on attachment: 215370 Committed r158184: <http://trac.webkit.org/changeset/158184>

All reviewed patches have been landed. Closing bug.