Bug 49971 – Web Inspector: pass style id to front-end as Object, not string.

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

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 74658 [details]
[PATCH] Proposed change.

[Alexander Pavlov (apavlov)]

Comment on attachment 74658 [details]
[PATCH] Proposed change.

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

> WebCore/inspector/InspectorStyleSheet.h:75
> +    unsigned int ordinal() const { return m_ordinal; }

unsigned int -> unsigned as per the common webkit style

> WebCore/inspector/InspectorStyleSheet.h:85
> +        return result;

return result.release(); is better

> WebCore/inspector/InspectorStyleSheet.h:90
> +    unsigned int m_ordinal;

unsigned int -> unsigned

[Build Bot]

Attachment 74658 [details] did not build on win:
Build output: http://queues.webkit.org/results/6300020

[Pavel Feldman]

Created attachment 74671 [details]
[PATCH] Review comments addressed.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/inspector/styles-new-API.html', u'WebCore/ChangeLog', u'WebCore/inspector/Inspector.idl', u'WebCore/inspector/InspectorCSSAgent.cpp', u'WebCore/inspector/InspectorCSSAgent.h', u'WebCore/inspector/InspectorStyleSheet.cpp', u'WebCore/inspector/InspectorStyleSheet.h', u'WebCore/inspector/front-end/CSSStyleModel.js']" exit_code: 1
WebCore/inspector/InspectorStyleSheet.h:241:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


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

[Alexander Pavlov (apavlov)]

Comment on attachment 74671 [details]
[PATCH] Review comments addressed.

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

I suggest that the bug be renamed to include the styleSheetChanged aspect of the patch

> WebCore/inspector/InspectorCSSAgent.cpp:284
> +    *url = inspectorStyleSheet->finalURL();

I'm wary of hitting the multiple inline stylesheets case....

> WebCore/inspector/InspectorStyleSheet.h:241
> +    virtual CSSStyleDeclaration* styleForId(const InspectorCSSId& id) const { ASSERT_UNUSED(id, id.ordinal() == 0); return inlineStyle(); }

id.ordinal() == 0 -> !id.ordinal()

> WebCore/inspector/front-end/CSSStyleModel.js:122
> +            WebInspector.cssModel._styleSheetChanged(rule.id.styleSheetId);

Why not bind the two callbacks to "this" and invoke this._styleSheetChanged() ? We know this is the best practice.

> WebCore/inspector/front-end/CSSStyleModel.js:143
> +            WebInspector.cssModel._styleSheetChanged(rule.id.styleSheetId);

ditto

> WebCore/inspector/front-end/CSSStyleModel.js:326
> +                WebInspector.cssModel._styleSheetChanged(this.id.styleSheetId);

ditto

> WebCore/inspector/front-end/CSSStyleModel.js:450
> +                WebInspector.cssModel._styleSheetChanged(style.id.styleSheetId);

ditto (this callback already seems to be bound to "this", so nothing extra is necessary)

> WebCore/inspector/front-end/CSSStyleModel.js:483
> +                WebInspector.cssModel._styleSheetChanged(this.ownerStyle.id.styleSheetId);

ditto, callback already bound

> WebCore/inspector/front-end/CSSStyleModel.js:535
> +                WebInspector.cssModel._styleSheetChanged(this.id);

ditto, callback already bound

[Build Bot]

Attachment 74671 [details] did not build on win:
Build output: http://queues.webkit.org/results/6356012

[Pavel Feldman]

Created attachment 74734 [details]
[PATCH] Review comments addressed.

[Yury Semikhatsky]

Comment on attachment 74734 [details]
[PATCH] Review comments addressed.

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

> WebCore/inspector/InspectorCSSAgent.cpp:285
> +    inspectorStyleSheet->text(result);

this call may fail, should we report such failures to the caller?

> WebCore/inspector/InspectorStyleSheet.h:58
> +        if (!value->getString("styleSheetId", &m_styleSheetId))

"styleSheetId" is used in at least to places in the code. Please use constants for property names like this.

[Pavel Feldman]

(In reply to comment #9)
> (From update of attachment 74734 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74734&action=review
> 
> > WebCore/inspector/InspectorCSSAgent.cpp:285
> > +    inspectorStyleSheet->text(result);
> 
> this call may fail, should we report such failures to the caller?
> 

We default to empty results on errors in entire CSSAgent. Needs to be fixed everywhere.

> > WebCore/inspector/InspectorStyleSheet.h:58
> > +        if (!value->getString("styleSheetId", &m_styleSheetId))
> 
> "styleSheetId" is used in at least to places in the code. Please use constants for property names like this.

Needs to be fixed everywhere.


Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/inspector/styles-new-API.html
	M	WebCore/ChangeLog
	M	WebCore/inspector/Inspector.idl
	M	WebCore/inspector/InspectorCSSAgent.cpp
	M	WebCore/inspector/InspectorCSSAgent.h
	M	WebCore/inspector/InspectorStyleSheet.cpp
	M	WebCore/inspector/InspectorStyleSheet.h
	M	WebCore/inspector/front-end/CSSStyleModel.js
Committed r72652

[WebKit Review Bot]

http://trac.webkit.org/changeset/72652 might have broken GTK Linux 32-bit Release
The following tests are not passing:
inspector/audits-panel-functional.html