Bug 105285 - Web Inspector: [Styles] Implement navigation to UI locations of property names/values in the source code
: Web Inspector: [Styles] Implement navigation to UI locations of property name...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Alexander Pavlov (apavlov)
:
Depends on: 105764 109168 111175
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-18 06:47 PST by Alexander Pavlov (apavlov)
Modified: 2013-03-01 06:49 PST (History)
10 users (show)

See Also:


Attachments
Patch (23.79 KB, patch)
2012-12-18 08:08 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (24.57 KB, patch)
2013-01-13 23:50 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (26.38 KB, patch)
2013-01-24 02:30 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (25.46 KB, patch)
2013-01-28 03:49 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2013-02-11 06:42 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[Image] Screenshot of a navigable rule section hovered (11.86 KB, image/png)
2013-02-11 06:50 PST, Alexander Pavlov (apavlov)
no flags Details
Patch (11.17 KB, patch)
2013-02-12 04:15 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (6.08 KB, patch)
2013-03-01 04:11 PST, Alexander Pavlov (apavlov)
vsevik: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2012-12-18 06:47:07 PST
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.
Comment 1 Alexander Pavlov (apavlov) 2012-12-18 08:08:18 PST
Created attachment 179943 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2013-01-13 23:50:49 PST
Created attachment 182519 [details]
Patch
Comment 3 Vsevolod Vlasov 2013-01-15 11:31:44 PST
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.
Comment 4 Vsevolod Vlasov 2013-01-15 11:57:45 PST
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.
Comment 5 Vsevolod Vlasov 2013-01-15 11:58:40 PST
Comment on attachment 182519 [details]
Patch

Please remove unrelated changes, logic duplicates and make sure showUISourceCode is never called with null.
Comment 6 Alexander Pavlov (apavlov) 2013-01-24 02:30:53 PST
Created attachment 184448 [details]
Patch
Comment 7 Vsevolod Vlasov 2013-01-28 00:12:16 PST
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.
Comment 8 Alexander Pavlov (apavlov) 2013-01-28 03:49:12 PST
Created attachment 184966 [details]
Patch
Comment 9 Vsevolod Vlasov 2013-01-28 04:35:19 PST
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.
Comment 10 Alexander Pavlov (apavlov) 2013-02-11 06:42:40 PST
Created attachment 187558 [details]
Patch
Comment 11 Alexander Pavlov (apavlov) 2013-02-11 06:50:26 PST
Created attachment 187560 [details]
[Image] Screenshot of a navigable rule section hovered
Comment 12 Pavel Feldman 2013-02-11 08:10:12 PST
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.
Comment 13 Alexander Pavlov (apavlov) 2013-02-12 04:15:40 PST
Created attachment 187830 [details]
Patch
Comment 14 Alexander Pavlov (apavlov) 2013-02-12 05:18:36 PST
(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).
Comment 15 Alexander Pavlov (apavlov) 2013-03-01 04:11:15 PST
Created attachment 190930 [details]
Patch
Comment 16 Vsevolod Vlasov 2013-03-01 05:07:48 PST
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?
Comment 17 Alexander Pavlov (apavlov) 2013-03-01 06:14:37 PST
Committed r144449: <http://trac.webkit.org/changeset/144449>
Comment 18 WebKit Review Bot 2013-03-01 06:31:25 PST
Re-opened since this is blocked by bug 111175
Comment 19 Alexander Pavlov (apavlov) 2013-03-01 06:49:01 PST
Committed r144451: <http://trac.webkit.org/changeset/144451>