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
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 105764 109168 111175
:
  Show dependency treegraph
 
Reported: 2012-12-18 06:47 PST by
Modified: 2013-03-01 06:49 PST (History)


Attachments
Patch (23.79 KB, patch)
2012-12-18 08:08 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (24.57 KB, patch)
2013-01-13 23:50 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.38 KB, patch)
2013-01-24 02:30 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.46 KB, patch)
2013-01-28 03:49 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2013-02-11 06:42 PST, Alexander Pavlov (apavlov)
no flags Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Patch (6.08 KB, patch)
2013-03-01 04:11 PST, Alexander Pavlov (apavlov)
vsevik: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2012-12-18 08:08:18 PST -------
Created an attachment (id=179943) [details]
Patch
------- Comment #2 From 2013-01-13 23:50:49 PST -------
Created an attachment (id=182519) [details]
Patch
------- Comment #3 From 2013-01-15 11:31:44 PST -------
(From update of attachment 182519 [details])
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 From 2013-01-15 11:57:45 PST -------
(From update of attachment 182519 [details])
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 From 2013-01-15 11:58:40 PST -------
(From update of attachment 182519 [details])
Please remove unrelated changes, logic duplicates and make sure showUISourceCode is never called with null.
------- Comment #6 From 2013-01-24 02:30:53 PST -------
Created an attachment (id=184448) [details]
Patch
------- Comment #7 From 2013-01-28 00:12:16 PST -------
(From update of attachment 184448 [details])
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 From 2013-01-28 03:49:12 PST -------
Created an attachment (id=184966) [details]
Patch
------- Comment #9 From 2013-01-28 04:35:19 PST -------
(From update of attachment 184966 [details])
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 From 2013-02-11 06:42:40 PST -------
Created an attachment (id=187558) [details]
Patch
------- Comment #11 From 2013-02-11 06:50:26 PST -------
Created an attachment (id=187560) [details]
[Image] Screenshot of a navigable rule section hovered
------- Comment #12 From 2013-02-11 08:10:12 PST -------
(From update of attachment 187558 [details])
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 From 2013-02-12 04:15:40 PST -------
Created an attachment (id=187830) [details]
Patch
------- Comment #14 From 2013-02-12 05:18:36 PST -------
(In reply to comment #12)
> (From update of attachment 187558 [details] [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 From 2013-03-01 04:11:15 PST -------
Created an attachment (id=190930) [details]
Patch
------- Comment #16 From 2013-03-01 05:07:48 PST -------
(From update of attachment 190930 [details])
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 From 2013-03-01 06:14:37 PST -------
Committed r144449: <http://trac.webkit.org/changeset/144449>
------- Comment #18 From 2013-03-01 06:31:25 PST -------
Re-opened since this is blocked by bug 111175
------- Comment #19 From 2013-03-01 06:49:01 PST -------
Committed r144451: <http://trac.webkit.org/changeset/144451>