Bug 117508

Summary: [CSS Regions] Add new regionOversetChange event
Product: WebKit Reporter: Radu Stavila <stavila>
Component: CSSAssignee: Radu Stavila <stavila>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, esprehn+autocc, glenn, graouts, joepeck, kangil.han, mihnea, stavila, syoichi, timothy, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 117833    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
none
Patch without inspector changes
koivisto: review+, zalan: commit-queue-
Final patch
commit-queue: commit-queue-
Final patch (added reviewed by to change log) none

Description Radu Stavila 2013-06-11 07:01:04 PDT
Add new regionOversetChange event according to http://www.w3.org/TR/2013/WD-css3-regions-20130528/#named-flow-events
Comment 1 Radu Stavila 2013-06-20 05:42:13 PDT
Created attachment 205076 [details]
Patch
Comment 2 Radu Stavila 2013-06-20 06:43:56 PDT
Created attachment 205083 [details]
Patch without inspector changes
Comment 3 Antti Koivisto 2013-06-20 06:51:05 PDT
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.
Comment 4 Radu Stavila 2013-06-20 06:53:53 PDT
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.
Comment 5 zalan 2013-06-20 07:30:23 PDT
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())
Comment 6 Radu Stavila 2013-06-20 07:35:57 PDT
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).
Comment 7 Radu Stavila 2013-06-20 07:47:56 PDT
Created attachment 205090 [details]
Final patch
Comment 8 WebKit Commit Bot 2013-06-20 07:54:18 PDT
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
Comment 9 Radu Stavila 2013-06-20 08:01:17 PDT
Created attachment 205091 [details]
Final patch (added reviewed by to change log)
Comment 10 WebKit Commit Bot 2013-06-20 09:13:33 PDT
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>