Bug 105285 – Web Inspector: [Styles] Implement navigation to UI locations of property names/values in the source code

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

[Alexander Pavlov (apavlov)]

We should be able to click-navigate to a UISourceCode location corresponding to the CSS property name and value (be it a source-mapped resource or a stylesheet proper) from the Styles sidebar.

[Alexander Pavlov (apavlov)]

Created attachment 179943 [details]
Patch

[Alexander Pavlov (apavlov)]

Created attachment 182519 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 182519 [details]
Patch

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

> Source/WebCore/inspector/front-end/CSSStyleModel.js:525
> +    createLiveLocation: function(rawLocation, updateDelegate)

Is this change necessary and related?

> Source/WebCore/inspector/front-end/Linkifier.js:95
> +        var liveLocation = WebInspector.cssModel.createLiveLocation(rule.rawLocation, this._updateAnchor.bind(this, anchor));

Looks  unrelated

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2059
> +        var uiSourceCode = uiLocation ? uiLocation.uiSourceCode : WebInspector.workspace.uiSourceCodeForURL(rawLocation.url);

This looks like a CSSStyleModel.rawLocationToUILocation logic duplicate, you could remove it.
You should not call showUISourceCode with possibly null uiSourceCode.
Actually I would like to be sure that rawLocationToUILocation never returns null,  but I am not sure we can guarantee that right now.

[Vsevolod Vlasov]

Comment on attachment 182519 [details]
Patch

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

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:168
> +        if (forceRebind || (sourceMap && !this._sourceMapByStyleSheetURL[cssURL])) {

what if (forceRebind && !sourceMap?)

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:261
> +            return this._identityMapping(location);

We should just return null here and below, default mapping in CSSStyleModel.rawLocationToUILocation will take care then.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2081
> +        var uiSourceCode = WebInspector.workspace.uiSourceCodeForURL(this.style.parentRule.sourceURL);

These 3 lines could be moved above.

[Vsevolod Vlasov]

Comment on attachment 182519 [details]
Patch

Please remove unrelated changes, logic duplicates and make sure showUISourceCode is never called with null.

[Alexander Pavlov (apavlov)]

Created attachment 184448 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 184448 [details]
Patch

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

Looks like you didn't address some of the previous comments.

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:170
> +        if (forceRebind || (sourceMap && !this._sourceMapByStyleSheetURL[cssURL])) {

what if (forceRebind && !sourceMap?)

I would rewrite this as:
if (sourceMap && (forceRebind || !this._sourceMapByStyleSheetURL[cssURL])) {

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:258
> +            return this._identityMapping(location);

We should just return null here and below, default mapping in CSSStyleModel.rawLocationToUILocation will take care then.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2060
> +        if (uiLocation) {

This looks like a CSSStyleModel.rawLocationToUILocation logic duplicate, you could remove it. You should not call showUISourceCode with possibly null uiSourceCode. Actually I would like to be sure that rawLocationToUILocation never returns null, but I am not sure we can guarantee that right now.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2088
> +        var propertyNameClicked = element === this.nameElement;

I would move this line below, to the place where propertyNameClicked is used.

[Alexander Pavlov (apavlov)]

Created attachment 184966 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 184966 [details]
Patch

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

I think we should split this into 2 separate changes - one for sass source maps and another one for UI feature.
It would be great to have a test for SASS source mappings also.

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:201
> +        sourceMapURL = WebInspector.SourceMap.prototype._canonicalizeURL(sourceMapURL, styleSheetURL);

https://bugs.webkit.org/show_bug.cgi?id=107939 removes _canonicalizeURL and uses completeURL instead.
You will need to update your patch to follow this way of resolving relative URLs.

[Alexander Pavlov (apavlov)]

Created attachment 187558 [details]
Patch

[Alexander Pavlov (apavlov)]

Created attachment 187560 [details]
[Image] Screenshot of a navigable rule section hovered

[Pavel Feldman]

Comment on attachment 187558 [details]
Patch

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

Also missing tests.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2156
> +            this._setLineEndingsForURL(this.style.parentRule.sourceURL, lineEndings);

I don't think this belongs to the styles sidebar pane. It should probably be a part of the model. This mapping should be properly encapsulated.

[Alexander Pavlov (apavlov)]

Created attachment 187830 [details]
Patch

[Alexander Pavlov (apavlov)]

(In reply to comment #12)
> (From update of attachment 187558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187558&action=review
> 
> Also missing tests.

The model tests were landed in r142445.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:2156
> > +            this._setLineEndingsForURL(this.style.parentRule.sourceURL, lineEndings);
> 
> I don't think this belongs to the styles sidebar pane. It should probably be a part of the model. This mapping should be properly encapsulated.

Moved into the model. Once bug 105828 is fixed, it will become possible to move the mappings into the CSS stylesheet descriptors stored in the frontend (akin to the Script entities that store their own source mappings).

[Alexander Pavlov (apavlov)]

Created attachment 190930 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 190930 [details]
Patch

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

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1110
> +    _isCSSNavigable: function(url)

Can we move this to WebInspector.CSSRule?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2195
> +        var range = this.property.range;

Can this piece of code be a method of Property?

[Alexander Pavlov (apavlov)]

Committed r144449: <http://trac.webkit.org/changeset/144449>

[WebKit Review Bot]

Re-opened since this is blocked by bug 111175

[Alexander Pavlov (apavlov)]

Committed r144451: <http://trac.webkit.org/changeset/144451>