[webkit-reviews] review denied: [Bug 99826] Web Inspector: Crash when adding a keyframes rule in the Styles pane : [Attachment 169605] Patch

[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 99826: Web Inspector: Crash when adding a keyframes rule in the Styles pane
https://bugs.webkit.org/show_bug.cgi?id=99826

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

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


> Source/WebCore/inspector/InspectorStyleSheet.cpp:816
> +    CSSParser p(m_pageStyleSheet->ownerDocument() ?
CSSParserContext(m_pageStyleSheet->ownerDocument()) :
strictCSSParserContext());

We use different parser constructors in this class. Why? Could we introduce
factory methods with descriptive names?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:821
> +	   return 0;

Do you want to extract this into a separate method?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:845
> +	   m_pageStyleSheet->deleteRule(lastRuleIndex, ec);

You should assign exception code there.

> LayoutTests/inspector/styles/styles-add-new-rule.html:59
> +    function step6()

Please add test per case.