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: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 117833 | ||||||||||||
Bug Blocks: | 57312 | ||||||||||||
Attachments: |
|
Description
Radu Stavila
2013-06-11 07:01:04 PDT
Created attachment 205076 [details]
Patch
Created attachment 205083 [details]
Patch without inspector changes
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 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 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 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). Created attachment 205090 [details]
Final patch
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 Created attachment 205091 [details]
Final patch (added reviewed by to change log)
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> |