[webkit-reviews] review denied: [Bug 74132] [CSS Regions] RenderRegion should inherit from RenderBlock : [Attachment 186801] Patch 2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 6 11:19:17 PST 2013


Julien Chaffraix <jchaffraix at webkit.org> has denied Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 74132: [CSS Regions] RenderRegion should inherit from RenderBlock
https://bugs.webkit.org/show_bug.cgi?id=74132

Attachment 186801: Patch 2
https://bugs.webkit.org/attachment.cgi?id=186801&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186801&action=review


r- about the paintObject issue that needs to be investigated and justified /
documented (and tested!).

> LayoutTests/ChangeLog:18
> +	   *
platform/chromium-win/fast/regions/selecting-text-through-different-region-flow
s-expected.txt:
> +	   *
platform/efl/fast/regions/selecting-text-through-different-region-flows-expecte
d.txt:
> +	   *
platform/gtk/fast/regions/selecting-text-through-different-region-flows-expecte
d.txt:
> +	   *
platform/qt/fast/regions/selecting-text-through-different-region-flows-expected
.txt:

These updates are just magic to me. How come an <h1> disappears?

> Source/WebCore/rendering/RenderBox.cpp:2927
> +	   // with variable width regions (see
https://bugs.webkit.org/show_bug.cgi?id=69896 )

Missing final sentence dot.

> Source/WebCore/rendering/RenderRegion.cpp:147
> -    if (!m_flowThread || !isValid())
> +    if (!m_flowThread || !isValid() || (paintInfo.phase !=
PaintPhaseForeground))

By limiting yourself to the foreground phase, you are breaking a lot of cases!
(e.g. outline and selection on your flow thread's  content).

paintReplaced was also called for PaintPhaseSelection which the new code
doesn't handle, nor does it explain why it's OK to do so. If you are
temporarily breaking some paint phase, this should at least have a FIXME.


More information about the webkit-reviews mailing list