Bug 142266

Created attachment 248725 [details] Patch

Adding reviewers. Please, could you take a look? Thanks!

Comment on attachment 248725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248725&action=review > Source/WebCore/rendering/InlineElementBox.cpp:73 > LayoutPoint childPoint = paintOffset; > if (is<RenderBox>(renderer()) && parent()->renderer().style().isFlippedBlocksWritingMode()) // Faster than calling containingBlock(). > childPoint = renderer().containingBlock()->flipForWritingModeForChild(&downcast<RenderBox>(renderer()), childPoint); Looks like we will do this computation even in cases where we won’t be painting. Seems unfortunate. > Source/WebCore/rendering/RenderElement.cpp:1174 > + bool preservePhase = paintInfo.phase == PaintPhaseSelection || paintInfo.phase == PaintPhaseTextClip; The check for PaintPhaseTextClip here seems to be dead code. We return above unless the phase is foreground or selection. > Source/WebCore/rendering/RenderElement.cpp:1187 > + PaintInfo info(paintInfo); > + info.phase = preservePhase ? paintInfo.phase : PaintPhaseBlockBackground; > + paint(info, childPoint); > + if (!preservePhase) { > + info.phase = PaintPhaseChildBlockBackgrounds; > + paint(info, childPoint); > + info.phase = PaintPhaseFloat; > + paint(info, childPoint); > + info.phase = PaintPhaseForeground; > + paint(info, childPoint); > + info.phase = PaintPhaseOutline; > + paint(info, childPoint); > + } The code here seems needlessly complicated. I would write: if (paintInfo.phase == PaintPhaseForeground) { paint(paintInfo, childPoint); return; } Then the code to paint all the different phases. Also seems we could reuse the original PaintInfo and just set the phase back to PaintPhaseSelection when done rather than copying the PaintInfo. Also seems that a helper function that sets the phase would make the code more readable rather than ll the assignment statements followed by calls to pain. > Source/WebCore/rendering/RenderFlexibleBox.cpp:336 > + if (!paintChild(*child, paintInfo, paintOffset, paintInfoForChild, usePrintRect, true)) We use enums rather than booleans in cases like this to avoid mysterious “true” like this one. Please do that here even though an enum might take a few more characters. > Source/WebCore/rendering/RenderGrid.cpp:1069 > + paintChild(*child, paintInfo, paintOffset, forChild, usePrintRect, true); Ditto.

Created attachment 248923 [details] Patch for landing

Thanks for the review! (In reply to comment #3) > > Source/WebCore/rendering/InlineElementBox.cpp:73 > > LayoutPoint childPoint = paintOffset; > > if (is<RenderBox>(renderer()) && parent()->renderer().style().isFlippedBlocksWritingMode()) // Faster than calling containingBlock(). > > childPoint = renderer().containingBlock()->flipForWritingModeForChild(&downcast<RenderBox>(renderer()), childPoint); > > Looks like we will do this computation even in cases where we won’t be > painting. Seems unfortunate. Yeah, I've kept the early return to avid this. > > Source/WebCore/rendering/RenderElement.cpp:1174 > > + bool preservePhase = paintInfo.phase == PaintPhaseSelection || paintInfo.phase == PaintPhaseTextClip; > > The check for PaintPhaseTextClip here seems to be dead code. We return above > unless the phase is foreground or selection. True, this is not needed anymore. > > Source/WebCore/rendering/RenderElement.cpp:1187 > > + PaintInfo info(paintInfo); > > + info.phase = preservePhase ? paintInfo.phase : PaintPhaseBlockBackground; > > + paint(info, childPoint); > > + if (!preservePhase) { > > + info.phase = PaintPhaseChildBlockBackgrounds; > > + paint(info, childPoint); > > + info.phase = PaintPhaseFloat; > > + paint(info, childPoint); > > + info.phase = PaintPhaseForeground; > > + paint(info, childPoint); > > + info.phase = PaintPhaseOutline; > > + paint(info, childPoint); > > + } > > The code here seems needlessly complicated. I would write: > > if (paintInfo.phase == PaintPhaseForeground) { > paint(paintInfo, childPoint); > return; > } > > Then the code to paint all the different phases. Also seems we could reuse > the original PaintInfo and just set the phase back to PaintPhaseSelection > when done rather than copying the PaintInfo. Also seems that a helper > function that sets the phase would make the code more readable rather than > ll the assignment statements followed by calls to pain. Thanks for the suggestions, I've modified the code and added a helper method. Now, the code seems much better to me. > > Source/WebCore/rendering/RenderFlexibleBox.cpp:336 > > + if (!paintChild(*child, paintInfo, paintOffset, paintInfoForChild, usePrintRect, true)) > > We use enums rather than booleans in cases like this to avoid mysterious > “true” like this one. Please do that here even though an enum might take a > few more characters. Done.

Comment on attachment 248923 [details] Patch for landing Clearing flags on attachment: 248923 Committed r181691: <http://trac.webkit.org/changeset/181691>

All reviewed patches have been landed. Closing bug.