[webkit-reviews] review denied: [Bug 45825] Web Inspector: Factor out InspectorCSSAgent : [Attachment 69647] [PRELIMINARY PATCH] Comments addressed

[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 45825: Web Inspector: Factor out InspectorCSSAgent
https://bugs.webkit.org/show_bug.cgi?id=45825

Attachment 69647: [PRELIMINARY PATCH] Comments addressed
https://bugs.webkit.org/attachment.cgi?id=69647&action=review

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

> WebCore/inspector/InspectorCSSAgent.cpp:130
> +    m_domAgent->setDOMListener(this);

Listeners are generally added, not set.

> WebCore/inspector/InspectorCSSAgent.cpp:143
> +Element* InspectorCSSAgent::elementForId(long nodeId)

Implementation order should match declaration.

> WebCore/inspector/InspectorCSSAgent.cpp:224
> +	   return; // ???

We don't have proper error handling yet, so just return here.

> WebCore/inspector/InspectorCSSAgent.cpp:303
> +    if (!inspectorStyleSheet && createIfAbsent) {

nit: replace if with guard to avoid nesting.

> WebCore/inspector/InspectorCSSAgent.cpp:317
> +	   String id = String::number(m_lastStyleSheetId++);

These four lines look a lot like bindStyleSheet. Should you call it from here
instead?

> WebCore/inspector/InspectorCSSAgent.cpp:339
> +    *styleSheetObject =
inspectorStyleSheet->buildObjectForStyleSheet(domAgent()->idForDocumentNode(doc
), domAgent()->documentURLString(doc), inspectorStyleSheetForDocument(doc,
false));

Why does this method accept so many parameters? InspectorStyleSheet should have
buildObjectForStyleSheet() with no parameters at all. Last one concerns me
most, see more comments on it.

> WebCore/inspector/InspectorCSSAgent.cpp:368
> +{

Please add FIXME here.

> WebCore/inspector/InspectorCSSAgent.cpp:458
> +	   if (styleSheetForDoc && styleSheet ==
styleSheetForDoc->pageStyleSheet())

So you pass it all the way to this place to set the origin? But type of the
InspectorStyleSheet already gives you complete information on it. Just add an
abstract getter for origin or accept it in the InspectorStyleSheet constructor.


> WebCore/inspector/InspectorStyleSheet.cpp:365
> +PassRefPtr<InspectorObject>
InspectorStyleSheet::buildObjectForStyleSheet(long ownerDocumentNodeId, const
String& documentURL, InspectorStyleSheet* styleSheetForDocument)

styleSheetForDocument->viaInspectorStyleSheet

> WebCore/inspector/InspectorStyleSheet.cpp:377
> +	   RefPtr<InspectorArray> cssRules =
internalBuildArrayForRuleList(cssRuleList.get(), documentURL,
styleSheetForDocument);

no need for 'internal' prefix - just make sure it is private.

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

I don't think you need these includes.

> WebCore/inspector/InspectorStyleSheet.h:54
> +class ParsedStyleSheet {

You should be able to define this in .cpp - no need to expose this internal
structure to clients.

> WebCore/inspector/InspectorStyleSheet.h:88
> +    virtual Document* ownerDocument() const { return
m_pageStyleSheet->document(); }

Do you need to expose this information from stylesheet?

> WebCore/inspector/InspectorStyleSheet.h:89
> +    virtual bool text(String* result) const;

In fact, I think you can get away with only few of these being virtual.

> WebCore/inspector/InspectorStyleSheet.h:99
> +    virtual PassRefPtr<InspectorObject> buildObjectForStyleSheet(long
ownerDocumentNodeId, const String& documentURL, InspectorStyleSheet*
styleSheetForDocument);

I don't think build methods should be virtual.

> WebCore/inspector/InspectorStyleSheet.h:105
> +    String ruleOrStyleId(CSSStyleDeclaration* style) const { unsigned index
= ruleIndexByStyle(style); return index == UINT_MAX ? "" :
String::number(index); }

It is not clear to me why we are mixing rule and style identifiers. In fact,
should we even bother having style identifiers?