Bug 117508

Summary:

[CSS Regions] Add new regionOversetChange event

Product:

WebKit

Reporter:

Radu Stavila <stavila>

Component:

CSS

Assignee:

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:

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

Radu Stavila

Reported 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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radu Stavila

Comment 1 2013-06-20 05:42:13 PDT

Created attachment 205076 [details] Patch

Radu Stavila

Comment 2 2013-06-20 06:43:56 PDT

Created attachment 205083 [details] Patch without inspector changes

Antti Koivisto

Comment 3 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.

Radu Stavila

Comment 4 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.

alan

Comment 5 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())

Radu Stavila

Comment 6 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).

Radu Stavila

Comment 7 2013-06-20 07:47:56 PDT

Created attachment 205090 [details] Final patch

WebKit Commit Bot

Comment 8 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

Radu Stavila

Comment 9 2013-06-20 08:01:17 PDT

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

WebKit Commit Bot

Comment 10 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>

Note You need to log in before you can comment on or make changes to this bug.