Bug 117508 - [CSS Regions] Add new regionOversetChange event
Summary: [CSS Regions] Add new regionOversetChange event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Radu Stavila
URL:
Keywords:
Depends on: 117833
Blocks: 57312
  Show dependency treegraph
 
Reported: 2013-06-11 07:01 PDT by Radu Stavila
Modified: 2013-07-02 00:54 PDT (History)
12 users (show)

See Also:


Attachments
Patch (78.04 KB, patch)
2013-06-20 05:42 PDT, Radu Stavila
no flags Details | Formatted Diff | Diff
Patch without inspector changes (63.14 KB, patch)
2013-06-20 06:43 PDT, Radu Stavila
koivisto: review+
zalan: commit-queue-
Details | Formatted Diff | Diff
Final patch (63.14 KB, patch)
2013-06-20 07:47 PDT, Radu Stavila
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Final patch (added reviewed by to change log) (63.14 KB, patch)
2013-06-20 08:01 PDT, Radu Stavila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>