Bug 49971

Summary: Web Inspector: pass style id to front-end as Object, not string.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, buildbot, bweinstein, eric, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed change.
none
[PATCH] Review comments addressed.
none
[PATCH] Review comments addressed. yurys: review+

Description Pavel Feldman 2010-11-23 06:55:55 PST
Patch to follow.
Comment 1 Pavel Feldman 2010-11-23 07:00:53 PST
Created attachment 74658 [details]
[PATCH] Proposed change.
Comment 2 Alexander Pavlov (apavlov) 2010-11-23 07:17:25 PST
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
Comment 3 Build Bot 2010-11-23 07:17:39 PST
Attachment 74658 [details] did not build on win:
Build output: http://queues.webkit.org/results/6300020
Comment 4 Pavel Feldman 2010-11-23 08:27:38 PST
Created attachment 74671 [details]
[PATCH] Review comments addressed.
Comment 5 WebKit Review Bot 2010-11-23 08:30:13 PST
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.
Comment 6 Alexander Pavlov (apavlov) 2010-11-23 08:38:50 PST
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
Comment 7 Build Bot 2010-11-23 10:09:46 PST
Attachment 74671 [details] did not build on win:
Build output: http://queues.webkit.org/results/6356012
Comment 8 Pavel Feldman 2010-11-24 01:49:30 PST
Created attachment 74734 [details]
[PATCH] Review comments addressed.
Comment 9 Yury Semikhatsky 2010-11-24 02:04:07 PST
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.
Comment 10 Pavel Feldman 2010-11-24 02:11:58 PST
(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
Comment 11 WebKit Review Bot 2010-11-24 02:43:24 PST
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