Currently InspectorDOMAgent does a bit too much wrt CSS handling (using InspectorCSSStore which has become too bloated, too). The absolute majority of the CSS-related code should go into a new InspectorCSSAgent class.
CSS rule selector handling changes should be added in a bug 46367 patch
Created attachment 69032 [details] [PATCH] Suggested solution without disabled properties. Contains integrated bug 46367 patch until one is landed
Created attachment 69063 [details] [PATCH] Refactored solution according to pfeldman's advice. No ChangeLogs, not ready for Mac - preliminary review only
Created attachment 69173 [details] [PATCH] The correct preliminary patch in place of the previous one
Created attachment 69482 [details] [PATCH] Introduced InspectorStyleSheet variations to avoid multiple maps, as suggested by pfeldman
Attachment 69482 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/inspector/InspectorCSSAgent.h:141: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/inspector/InspectorUtilities.cpp:37: Alphabetical sorting problem. [build/include_order] [4] WebCore/inspector/InspectorCSSAgent.cpp:619: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/inspector/InspectorCSSAgent.cpp:939: Missing space after , [whitespace/comma] [3] WebCore/inspector/InspectorCSSAgent.cpp:968: Missing space after , [whitespace/comma] [3] WebCore/inspector/InspectorCSSAgent.cpp:57: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] WebCore/inspector/InspectorCSSAgent.cpp:60: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] WebCore/inspector/InspectorCSSAgent.cpp:67: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] WebCore/inspector/InspectorCSSAgent.cpp:68: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] WebCore/inspector/InspectorCSSAgent.cpp:69: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] WebCore/inspector/InspectorCSSAgent.cpp:70: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] WebCore/inspector/InspectorCSSAgent.cpp:75: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] Total errors found: 12 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 69484 [details] [PATCH] Preliminary patch style fixed
Attachment 69484 [details] did not build on mac: Build output: http://queues.webkit.org/results/4220039
Comment on attachment 69484 [details] [PATCH] Preliminary patch style fixed View in context: https://bugs.webkit.org/attachment.cgi?id=69484&action=review Could you please submit a change with new InspectorStyleSheet and InspectorCSSAgent components. I believe rest if orthogonal to the change. > WebCore/inspector/InspectorCSSAgent.h:92 > +class InspectorStyleSheet : public RefCounted<InspectorStyleSheet> { InspectorStyleSheet should be defined in its own class. > WebCore/inspector/InspectorUtilities.h:50 > + static CSSStyleSheet* parentStyleSheet(StyleBase*); CSS-specific utilities should belong to InspectorCSSAgent.
(In reply to comment #9) > (From update of attachment 69484 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69484&action=review > > Could you please submit a change with new InspectorStyleSheet and InspectorCSSAgent components. I believe rest if orthogonal to the change. Refactoring the change to only contain the InspectorStyleSheet and InspectorCSSAgent {cpp,h} files and changes for these to be included into all builds.
Comment on attachment 69484 [details] [PATCH] Preliminary patch style fixed View in context: https://bugs.webkit.org/attachment.cgi?id=69484&action=review Some InspectorStyleSheet API notes. > WebCore/inspector/InspectorCSSAgent.h:67 > +class ParsedStyleSheet { Move this into InspectorStyleSheet. > WebCore/inspector/InspectorCSSAgent.h:69 > + typedef Vector<RefPtr<CSSStyleSourceData> > SourceData; You should instead introduce CSSRuleSourceData that would own this style source data and operate on it. > WebCore/inspector/InspectorCSSAgent.h:72 > + CSSStyleSheet* cssStyleSheet() const { return m_styleSheetByParser; } m_styleSheetByParser -> m_parserOutput ? > WebCore/inspector/InspectorCSSAgent.h:79 > + bool ensureSourceData(Node* ownerNode); I don't think ParsedStyleSheet should be node-aware. > WebCore/inspector/InspectorCSSAgent.h:94 > + static PassRefPtr<InspectorStyleSheet> create(const String& id, CSSStyleSheet* liveStyleSheet) liveStyleSheet -> domStyleSheet? > WebCore/inspector/InspectorCSSAgent.h:102 > + virtual CSSStyleSheet* liveStyleSheet() const { return m_liveStyleSheet; } domStyleSheet? > WebCore/inspector/InspectorCSSAgent.h:105 > + virtual bool setStyleText(CSSStyleDeclaration*, const String&); Where would you get CSSStyleDeclaration to pass in? Should you use style id instead? > WebCore/inspector/InspectorCSSAgent.h:106 > + virtual Document* ownerDocument() const; I can't come up with the scenario where ownerDocument needs to be exposed. > WebCore/inspector/InspectorCSSAgent.h:107 > + virtual RefPtr<CSSStyleSourceData> styleSourceDataFor(CSSStyleDeclaration* style) const { return m_parsedStyleSheet.styleSourceDataAt(ruleIndexByStyle(style)); } As we agreed, you should not expose CSSStyleSourceData. > WebCore/inspector/InspectorCSSAgent.h:108 > + virtual String fullRuleOrStyleId(CSSStyleDeclaration* style) const { return id() + ":" + ruleOrStyleId(style); } I don't think we should mix rules and styles. > WebCore/inspector/InspectorCSSAgent.h:110 > + virtual CSSStyleRule* setRuleSelector(const String& id, const String& selector); id -> ruleId? Also, why do you return new style rule instance?
Comment on attachment 69484 [details] [PATCH] Preliminary patch style fixed Implementation looks good. View in context: https://bugs.webkit.org/attachment.cgi?id=69484&action=review > WebCore/inspector/InspectorCSSAgent.cpp:369 > +void InspectorStyleSheet::didStyleSheetTextChange(const String& newText) You should simply call it setStyleSheetText. did* is only used for clients in WebCore.
(In reply to comment #11) > (From update of attachment 69484 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69484&action=review > > Some InspectorStyleSheet API notes. > > > WebCore/inspector/InspectorCSSAgent.h:67 > > +class ParsedStyleSheet { > > Move this into InspectorStyleSheet. Done. > > WebCore/inspector/InspectorCSSAgent.h:69 > > + typedef Vector<RefPtr<CSSStyleSourceData> > SourceData; > > You should instead introduce CSSRuleSourceData that would own this style source data and operate on it. Done. > > WebCore/inspector/InspectorCSSAgent.h:72 > > + CSSStyleSheet* cssStyleSheet() const { return m_styleSheetByParser; } > > m_styleSheetByParser -> m_parserOutput ? Done. > > WebCore/inspector/InspectorCSSAgent.h:79 > > + bool ensureSourceData(Node* ownerNode); > > I don't think ParsedStyleSheet should be node-aware. Refactored. > > WebCore/inspector/InspectorCSSAgent.h:94 > > + static PassRefPtr<InspectorStyleSheet> create(const String& id, CSSStyleSheet* liveStyleSheet) > > liveStyleSheet -> domStyleSheet? > > > WebCore/inspector/InspectorCSSAgent.h:102 > > + virtual CSSStyleSheet* liveStyleSheet() const { return m_liveStyleSheet; } > > domStyleSheet? > We seem to have agreed on pageStyleSheet, as CSS is only slightly related to DOM. > > WebCore/inspector/InspectorCSSAgent.h:105 > > + virtual bool setStyleText(CSSStyleDeclaration*, const String&); > > Where would you get CSSStyleDeclaration to pass in? Should you use style id instead? CSSStyleSelector::styleRulesForElement() (discussed offline) > > WebCore/inspector/InspectorCSSAgent.h:106 > > + virtual Document* ownerDocument() const; > > I can't come up with the scenario where ownerDocument needs to be exposed. Please see throughout the patch. In brief: - documentURL (to calculate relative URLs in frames: WebInspector.resourceURLForRelatedNode) - inspectorStyleSheetForDocument (to retrieve a "via inspector" stylesheet for the same document as the InspectorStyleSheet belongs in) - idForDocumentNode (to retrieve a document node id, documentElementId. Unused ATM - can we remove it altogether? Will make things much simpler) > > WebCore/inspector/InspectorCSSAgent.h:107 > > + virtual RefPtr<CSSStyleSourceData> styleSourceDataFor(CSSStyleDeclaration* style) const { return m_parsedStyleSheet.styleSourceDataAt(ruleIndexByStyle(style)); } > > As we agreed, you should not expose CSSStyleSourceData. Done. > > WebCore/inspector/InspectorCSSAgent.h:108 > > + virtual String fullRuleOrStyleId(CSSStyleDeclaration* style) const { return id() + ":" + ruleOrStyleId(style); } > > I don't think we should mix rules and styles. Introduced separate API methods for these (relying on the same private method as of now). > > WebCore/inspector/InspectorCSSAgent.h:110 > > + virtual CSSStyleRule* setRuleSelector(const String& id, const String& selector); > > id -> ruleId? > Also, why do you return new style rule instance? Done.
Created attachment 69647 [details] [PRELIMINARY PATCH] Comments addressed I need an approval of the CSS{Style,Rule}SourceData + API changes, hence the full patch. NOT MAC-READY
Attachment 69647 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/inspector/InspectorStyleSheet.cpp:26: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 69647 [details] did not build on mac: Build output: http://queues.webkit.org/results/4248031
Comment on attachment 69647 [details] [PRELIMINARY PATCH] Comments addressed 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?
(In reply to comment #17) > (From update of attachment 69647 [details]) > 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. Right - in the event when there may be several of those. In this case, we can handle only one. Please see Document::setWindowAttributeEventListener(), EventTarget::setAttributeEventListener() and v8::Debug::SetDebugEventListener() for examples. > > WebCore/inspector/InspectorCSSAgent.cpp:143 > > +Element* InspectorCSSAgent::elementForId(long nodeId) > > Implementation order should match declaration. Done. > > WebCore/inspector/InspectorCSSAgent.cpp:224 > > + return; // ??? > > We don't have proper error handling yet, so just return here. Done. > > WebCore/inspector/InspectorCSSAgent.cpp:303 > > + if (!inspectorStyleSheet && createIfAbsent) { > > nit: replace if with guard to avoid nesting. Done. > > 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? As you can see, the created inspectorStyleSheet instances are of different types, so this doesn't work. I could extract the 3 or 4 lines into a utility method, though... > > 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. Feel free to examine the code a bit closer. Basically, there are 2 primary solutions: 1. Pass an Inspector[CSS|DOM]Agent (either-or) instance into the InspectorStyleSheet constructor, which seems wasteful, or 2. Fill the related InspectorObject fields here, outside of buildObjectForStyleSheet(). Which one looks better to you? > > WebCore/inspector/InspectorCSSAgent.cpp:368 > > +{ > > Please add FIXME here. Done. > > 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. Done. It seems a bit of waste to store the origin inside... > > WebCore/inspector/InspectorStyleSheet.cpp:365 > > +PassRefPtr<InspectorObject> InspectorStyleSheet::buildObjectForStyleSheet(long ownerDocumentNodeId, const String& documentURL, InspectorStyleSheet* styleSheetForDocument) > > styleSheetForDocument->viaInspectorStyleSheet Done. > > WebCore/inspector/InspectorStyleSheet.cpp:377 > > + RefPtr<InspectorArray> cssRules = internalBuildArrayForRuleList(cssRuleList.get(), documentURL, styleSheetForDocument); > > no need for 'internal' prefix - just make sure it is private. Done. > > WebCore/inspector/InspectorStyleSheet.h:28 > > +#include "CSSPropertySourceData.h" > > I don't think you need these includes. I do, since some of the simple operations/setters/getters are defined here. I can move them into cpp and get rid of the includes. Does it make sense? > > 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. Done. > > WebCore/inspector/InspectorStyleSheet.h:88 > > + virtual Document* ownerDocument() const { return m_pageStyleSheet->document(); } > > Do you need to expose this information from stylesheet? I don't, once we solve the buildObjectForStyleSheet() parameters issue. > > 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. See next comment. > > 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. Please see the patch. I'm trying to have assertions in InspectorStyleSheetForInlineStyle for all methods that don't make sense being called on its instances. Do you think we should allow forbidden operations (like buildObjectForStyleSheet() on an inline style)? > > 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? We are not mixing them. Why does this implementation detail bother you? Does the way of id generation for different domains make any difference? Please see the protocol doc for the styleId need reasoning. I will attach a fixed patch once we are done with the open issues.
Created attachment 69790 [details] [PRELIMINARY PATCH] Comments addressed This patch addresses the second batch of comments (the solutions for the questionable ones are also implemented)
(In reply to comment #19) > Created an attachment (id=69790) [details] > [PRELIMINARY PATCH] Comments addressed > > This patch addresses the second batch of comments (the solutions for the questionable ones are also implemented) ChangeLog is missing.
Attachment 69790 [details] did not build on mac: Build output: http://queues.webkit.org/results/4236054
Attachment 69790 [details] did not build on qt: Build output: http://queues.webkit.org/results/4202071
Attachment 69790 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4212086
Attachment 69790 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4242061
Created attachment 69923 [details] [PRELIMINARY PATCH] Short version with WebCore/css changes removed. NOT FOR LANDING
Attachment 69923 [details] did not build on mac: Build output: http://queues.webkit.org/results/4236078
Attachment 69923 [details] did not build on qt: Build output: http://queues.webkit.org/results/4204095
Attachment 69923 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4258006
Comment on attachment 69923 [details] [PRELIMINARY PATCH] Short version with WebCore/css changes removed. NOT FOR LANDING 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.
Committed with suggested changes: http://trac.webkit.org/changeset/69284