Bug 72701 – Web Inspector: [Protocol] Brush up retrieval of CSS styles for elements

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 115765 [details]
Patch

[WebKit Review Bot]

Attachment 115765 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:240:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:67:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 115765 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115765&action=review

> Source/WebCore/inspector/Inspector.json:1438
>                  "type": "object",

I don't see why we need this class - you should return these from getStylesFromNode.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:256
> +        // Early return to avoid unnecessary style recalcs.

This early return confuses me - we should treat all the flags equally.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:262
>      bool needStyleRecalc = element != m_lastElementWithPseudoState || forcePseudoState != m_lastPseudoState;

I would just add a check here to not recalc in case of inline query.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:311
> +                    parentStyle->setObject("inlineStyle", styleSheet->buildObjectForStyle(styleSheet->styleForId(InspectorCSSId(styleSheet->id(), 0))));

So asking for inherited styles will return all: inline, matched, etc? This does not sound right.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:88
> +            if ("computedStyle" in payload)

Here and below, use "if (payload.computedStyle)" for compilability.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:150
> +        CSSAgent.getStylesForNode(nodeId, [], WebInspector.CSSStyleModel.StyleFlags.COMPUTED, callback.bind(null, userCallback));

Computed style should take forced pseudo classes into consideration.

[Timothy Hatcher]

Comment on attachment 115765 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115765&action=review

> Source/WebCore/inspector/front-end/CSSStyleModel.js:59
> +    INLINE: 1 << 0,
> +    COMPUTED: 1 << 1,
> +    MATCHED: 1 << 2,
> +    ATTRIBUTES: 1 << 3,
> +    PSEUDO: 1 << 4,
> +    INHERITED: 1 << 5,
> +    ALL: (1 << 6) - 1

We don't use ALLCAPS style for enums. You didn't do it in InspectorCSSAgent.h. There should also be a comment about this needing to match InspectorCSSAgent.h.

[Timothy Hatcher]

Comment on attachment 115765 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115765&action=review

> Source/WebCore/ChangeLog:8
> +        * inspector/Inspector.json:

Do we have any rules about bumping protocol version when removing methods (or any other compatability change)?

[Alexander Pavlov (apavlov)]

Created attachment 115813 [details]
Patch

[WebKit Review Bot]

Attachment 115813 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:240:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:240:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:240:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:297:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:297:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:311:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:67:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:68:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:68:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:69:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:69:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:69:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 12 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

> Do we have any rules about bumping protocol version when removing methods (or any other compatability change)?

CSS domain is currently marked as "hidden" as a whole. We don't generate documentation for it. Breaking backwards compatibility for published domains results in a build error generated by generate-inspector-protocol-version script. See http://code.google.com/chrome/devtools/docs/remote-debugging.html#protocol for public compatibility definition.

[Alexander Pavlov (apavlov)]

@Timothy: this is one step toward cleaning up the CSS domain to prepare it for opening to the public.

[Pavel Feldman]

Comment on attachment 115813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115813&action=review

> Source/WebCore/inspector/InspectorCSSAgent.cpp:317
> +    unsigned forcePseudoState = computePseudoClassMask(forcedPseudoClasses ? forcedPseudoClasses->get() : 0);

This logic is not trivial. Please do not copy-paste it.

> Source/WebCore/inspector/front-end/AuditRules.js:746
> +            function inlineCallback(targetResult, inlineStyle, styleAttributes)

You can define targetResult as a variable in the getStyles scope.

> LayoutTests/inspector/styles/styles-new-API.html:53
> +        function computedCallback(error, computedStyle)

Why do we need this?

[Alexander Pavlov (apavlov)]

(In reply to comment #10)
> (From update of attachment 115813 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115813&action=review
> 
> > Source/WebCore/inspector/InspectorCSSAgent.cpp:317
> > +    unsigned forcePseudoState = computePseudoClassMask(forcedPseudoClasses ? forcedPseudoClasses->get() : 0);
> 
> This logic is not trivial. Please do not copy-paste it.

Done.

> > Source/WebCore/inspector/front-end/AuditRules.js:746
> > +            function inlineCallback(targetResult, inlineStyle, styleAttributes)
> 
> You can define targetResult as a variable in the getStyles scope.

Done.

> > LayoutTests/inspector/styles/styles-new-API.html:53
> > +        function computedCallback(error, computedStyle)
> 
> Why do we need this?

Do you mean I can remove the entire test (styles-new-API.html) altogether? It's a bare API unit test, not an integration test.

[Alexander Pavlov (apavlov)]

Created attachment 116061 [details]
Patch

[WebKit Review Bot]

Attachment 116061 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:251:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:251:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:251:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:303:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:303:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:317:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:67:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:68:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:68:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:69:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:69:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:69:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 12 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Pavlov (apavlov)]

Committed r100900: <http://trac.webkit.org/changeset/100900>