[webkit-reviews] review granted: [Bug 45825] Web Inspector: Factor out InspectorCSSAgent : [Attachment 69923] [PRELIMINARY PATCH] Short version with WebCore/css changes removed. NOT FOR LANDING

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has granted Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 45825: Web Inspector: Factor out InspectorCSSAgent
https://bugs.webkit.org/show_bug.cgi?id=45825

Attachment 69923: [PRELIMINARY PATCH] Short version with WebCore/css changes
removed. NOT FOR LANDING
https://bugs.webkit.org/attachment.cgi?id=69923&action=review

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

r+ with nits. this has been up in the air for too long. lets land and fix
remaining stuff later. This agent is not yet wired / used in the code, so we
are fine.

> WebCore/inspector/InspectorCSSAgent.cpp:344
> +    bool success = inspectorStyleSheet->setRuleSelector(ruleId, selector);

You might want to return the rule pointer here and use it below (as in addRule)


> WebCore/inspector/InspectorCSSAgent.h:28
> +#include "CSSPropertySourceData.h"

These are not used.

> WebCore/inspector/InspectorCSSAgent.h:59
> +    static PassRefPtr<InspectorCSSAgent> create(InspectorFrontend* frontend,
InspectorDOMAgent* domAgent)

Swap the parameters order?

> WebCore/inspector/InspectorCSSAgent.h:91
> +    typedef HashMap<RefPtr<Document>, RefPtr<InspectorStyleSheet> >
DocumentToInspectorStyleSheetMap; // "via inspector" stylesheets

DocumentToViaInspectorStyleSheet

> WebCore/inspector/InspectorCSSAgent.h:101
> +    InspectorDOMAgent* domAgent() const { return m_domAgent.get(); }

Why do you need a private getter? Just access m_domAgent inline.

> WebCore/inspector/InspectorDOMAgent.h:154
> +	   long idForDocumentNode(Node*);

This method is unused and dangerous. One should use pushNodeToFrontend instead
in order to obtain the id.

> WebCore/inspector/InspectorStyleSheet.h:28
> +#include "CSSPropertySourceData.h"

Please remove unused includes.

> WebCore/inspector/InspectorUtilities.cpp:34
> +#include "CSSRule.h"

I don't think you need theses includes here.