[webkit-reviews] review denied: [Bug 129032] [CSS Regions] Overset computation is incorrect in some cases : [Attachment 225250] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Mar 3 02:22:38 PST 2014
Mihnea Ovidenie <mihnea at adobe.com> has denied Andrei Bucur <abucur at adobe.com>'s
request for review:
Bug 129032: [CSS Regions] Overset computation is incorrect in some cases
https://bugs.webkit.org/show_bug.cgi?id=129032
Attachment 225250: Patch
https://bugs.webkit.org/attachment.cgi?id=225250&action=review
------- Additional Comments from Mihnea Ovidenie <mihnea at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225250&action=review
r-
I like the patch and i think is moving in the right direction.However, i would
like you to take another look at it.
As a comment, i would have probably created another patch before with the
movement of regionOversetChange methods from RenderRegion to
RenderNamedFlowFragment, without functionality change, making the review
easier.
> Source/WebCore/ChangeLog:10
> + 1. Regions overflow no longer trigger an overset changed event. This
is beacuse
beacuse -> because
> Source/WebCore/ChangeLog:25
> +
5. The regionOverset property is moved from RenderRegion to
RenderNamedFlowFragment.
> Source/WebCore/dom/WebKitNamedFlow.cpp:77
> + if (namedFlowFragment->generatingElement()->regionOversetState() ==
RegionOverset)
No need for exposing the generatingElement() here. If you write something like:
return (namedFlowFragment->regionOversetState() == RegionOverset)
you achieve the same result since the method you moved to
RenderNamedFlowFragment, regionOversetState() has the assertion in place. I
also prefer using a ternary operator here.
> Source/WebCore/dom/WebKitNamedFlow.cpp:119
> + if (namedFlowFragment->generatingElement()->regionOversetState() ==
RegionEmpty)
No need for assert here if you write:
if (namedFlowFragment->regionOversetState() == RegionEmpty)
> Source/WebCore/rendering/FlowThreadController.cpp:249
> + flowRenderer->dispatchRegionEvents();
Worth a comment in the changelog explaining why you chose this layout phase for
dispatching the event(s).
> Source/WebCore/rendering/RenderNamedFlowThread.cpp:307
> +void RenderNamedFlowThread::dispatchRegionEvents()
This deserves a comment in the changelog since you are talking about events,
while right now you are using this function to dispatch only the regions
overset change event. I assume you want this function to be used also when we
support the regionFragmentChange.
Btw, i would rename this function to dispatchRegionChainEvents or
dispatchNamedFlowEvents since you are not dispatching an event for a region but
rather you are dispatching the events for the named flow.
I also think it is worth explaining in the ChangeLog why regionLayoutUpdate is
not triggered here, in the same place as regionOversetChange.
> Source/WebCore/rendering/RenderNamedFlowThread.h:153
> + // Used to store the content bottom until the moment we need to update
the overset state.
I would move this comment into the changelog.
> LayoutTests/fast/regions/cssom/webkit-named-flow-overset.html:62
> + // Overset should be false since the shadow of the content overflows the
last region.
Maybe we can make this comment clearer. Something like:
"Overset should be false even if the content shadow overflows the last region
since the visual overflow does not influence the overset property.
More information about the webkit-reviews
mailing list