[webkit-reviews] review denied: [Bug 44620] Web Inspector: reflect changes to styles when they happen outside inspector. : [Attachment 82797] [PATCH] The timer-based attribute revalidation approach

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 44620: Web Inspector: reflect changes to styles when they happen outside
inspector.
https://bugs.webkit.org/show_bug.cgi?id=44620

Attachment 82797: [PATCH] The timer-based attribute revalidation approach
https://bugs.webkit.org/attachment.cgi?id=82797&action=review

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

> LayoutTests/inspector/styles/styles-update-from-js.html:21
> +	   InspectorTest.evaluateInPage("modifyStyleAttribute()", f);

You should not rely upon pending dispatches - use sniffers instead.

> LayoutTests/inspector/styles/styles-update-from-js.html:28
> +	       InspectorTest.runAfterPendingDispatches(setParsedAttribute);

ditto

> LayoutTests/inspector/styles/styles-update-from-js.html:41
> +	       InspectorTest.runAfterPendingDispatches(done);

ditto

> LayoutTests/inspector/styles/styles-update-from-js.html:70
> +	   var innerMapping = WebInspector.domAgent._idToDOMNode;

treeOutline.findTreeElement(InspectorTest.expandedNodeWithId("container"))

> LayoutTests/inspector/styles/styles-update-from-js.html:80
> +    InspectorTest.expandElementsTree(selectContainerElementContinuation);

1) Tests now flow top->bottom.
2) No need to expand tree, start with selectNode

> LayoutTests/inspector/styles/styles-update-from-js.html:83
> +function modifyStyleAttribute()

test functions go before test()

> LayoutTests/inspector/styles/styles-update-from-js.html:110
> +<div id="other">

Remove this div.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:475
> +		  
InspectorInstrumentation::didInvalidateStyleAttr(m_node->document(),
static_cast<StyledElement*>(m_node));

Simply pass node here, do all the casts and gets in the instrumentation code.

> Source/WebCore/dom/StyledElement.cpp:255
> +	   setNeedsStyleRecalc(InlineStyleChange);

Please revert or ask for Dave's blessing.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:238
> +    , m_revalidateStyleAttrTask(this)

You should only create this timer when needed.

> Source/WebCore/inspector/InspectorDOMAgent.h:148
> +    class RevalidateStyleAttrTask {

No abbreviations in WebCore. You should also do a forward declaration here with
private implementation in cpp.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1202
> +const String& InspectorStyleSheetForInlineStyle::styleTextFromElement()
const

elementStyleText ?