[webkit-reviews] review denied: [Bug 93443] Web Inspector: Protocol Extension: Add "regionLayoutUpdate" event : [Attachment 160934] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 31 02:08:29 PDT 2012


Pavel Feldman <pfeldman at chromium.org> has denied Andrei Poenaru
<poenaru at adobe.com>'s request for review:
Bug 93443: Web Inspector: Protocol Extension: Add "regionLayoutUpdate" event
https://bugs.webkit.org/show_bug.cgi?id=93443

Attachment 160934: Patch
https://bugs.webkit.org/attachment.cgi?id=160934&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=160934&action=review


Overall looks good. A number of renaming suggestions inline.

> Source/WebCore/dom/NamedFlowCollection.cpp:98
> +    InspectorInstrumentation::didRemoveNamedFlow(m_document, namedFlow);

willRemoveNamedFlow? (it is now happening before the actual removal)

> Source/WebCore/inspector/Inspector.json:2446
> +		   "name": "regionLayoutUpdate",

regionLayoutUpdated?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:542
> +    int documentNodeId = m_domAgent->boundNodeId(document);

Sounds like this is checked 3 times. A helper method
documentNodeWithRequestedFlowsId?

> Source/WebCore/inspector/front-end/CSSStyleModel.js:381
> +	   if
(!this.hasEventListeners(WebInspector.CSSStyleModel.Events.NamedFlowCreated))

It is too late for this check, it does not save anything.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:398
>	   if
(!this.hasEventListeners(WebInspector.CSSStyleModel.Events.NamedFlowRemoved))

ditto

> Source/WebCore/inspector/front-end/CSSStyleModel.js:415
> +	   if
(!this.hasEventListeners(WebInspector.CSSStyleModel.Events.RegionLayoutUpdate))


ditto

> Source/WebCore/inspector/front-end/CSSStyleModel.js:488
> +	   this._namedFlowCollections = {};

Should this fire an event?

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1436
> +    getFlowByName: function(flowName)

flowByName


More information about the webkit-reviews mailing list