Bug 117508 – [CSS Regions] Add new regionOversetChange event

• Component: CSS
• Status: RESOLVED FIXED

[Radu Stavila]

Add new regionOversetChange event according to http://www.w3.org/TR/2013/WD-css3-regions-20130528/#named-flow-events

[Radu Stavila]

Created attachment 205076 [details]
Patch

[Radu Stavila]

Created attachment 205083 [details]
Patch without inspector changes

[Antti Koivisto]

Comment on attachment 205083 [details]
Patch without inspector changes

View in context: https://bugs.webkit.org/attachment.cgi?id=205083&action=review

> Source/WebCore/rendering/RenderRegion.cpp:136
> +    if (isValid() && node() && node()->isElementNode())

Can node ever actually be anything else than Element? If not then the element test should be removed (toElement() asserts).

It would be nicer to reverse the conditions and bail out fast on error case.

> Source/WebCore/rendering/RenderRegion.cpp:145
> +    if (node() && node()->isElementNode())
> +        toElement(node())->setRegionOversetState(state);

Here too.

[Radu Stavila]

Comment on attachment 205083 [details]
Patch without inspector changes

View in context: https://bugs.webkit.org/attachment.cgi?id=205083&action=review

>> Source/WebCore/rendering/RenderRegion.cpp:136
>> +    if (isValid() && node() && node()->isElementNode())
> 
> Can node ever actually be anything else than Element? If not then the element test should be removed (toElement() asserts).
> 
> It would be nicer to reverse the conditions and bail out fast on error case.

Yes, it is possible for it not to be an element, when using regions-based multicol.

[zalan]

Comment on attachment 205083 [details]
Patch without inspector changes

View in context: https://bugs.webkit.org/attachment.cgi?id=205083&action=review

> Source/WebCore/rendering/RenderFlowThread.cpp:783
> +        updatePreviousRegionCount();

any particular reason why just not do 'm_previousRegionCount = m_regionList.size();' instead?

> Source/WebCore/rendering/RenderFlowThread.h:194
> +    bool shouldDispatchRegionOversetChangeEvent() { return m_dispatchRegionOversetChangeEvent; }

missing const

>> Source/WebCore/rendering/RenderRegion.cpp:145
>> +        toElement(node())->setRegionOversetState(state);
> 
> Here too.

Shouldn't this check against the same set of conditions as regionOversetState does? (diff: isValid())

[Radu Stavila]

Comment on attachment 205083 [details]
Patch without inspector changes

View in context: https://bugs.webkit.org/attachment.cgi?id=205083&action=review

>> Source/WebCore/rendering/RenderFlowThread.cpp:783
>> +        updatePreviousRegionCount();
> 
> any particular reason why just not do 'm_previousRegionCount = m_regionList.size();' instead?

No, no particular reason... just thought it looked cleaner.

>> Source/WebCore/rendering/RenderFlowThread.h:194
>> +    bool shouldDispatchRegionOversetChangeEvent() { return m_dispatchRegionOversetChangeEvent; }
> 
> missing const

Will fix.

>>> Source/WebCore/rendering/RenderRegion.cpp:145
>>> +        toElement(node())->setRegionOversetState(state);
>> 
>> Here too.
> 
> Shouldn't this check against the same set of conditions as regionOversetState does? (diff: isValid())

This is part of the old code, I did not change it (I just moved it from RenderRegion.h to RenderRegion.cpp).

[Radu Stavila]

Created attachment 205090 [details]
Final patch

[WebKit Commit Bot]

Comment on attachment 205090 [details]
Final patch

Rejecting attachment 205090 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 205090, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/908131

[Radu Stavila]

Created attachment 205091 [details]
Final patch (added reviewed by to change log)

[WebKit Commit Bot]

Comment on attachment 205091 [details]
Final patch (added reviewed by to change log)

Clearing flags on attachment: 205091

Committed r151777: <http://trac.webkit.org/changeset/151777>