Bug 105285

Created attachment 179943 [details] Patch

Created attachment 182519 [details] Patch

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 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 on attachment 182519 [details] Patch Please remove unrelated changes, logic duplicates and make sure showUISourceCode is never called with null.

Created attachment 184448 [details] Patch

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.

Created attachment 184966 [details] Patch

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.

Created attachment 187558 [details] Patch

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

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.

Created attachment 187830 [details] Patch

(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).

Created attachment 190930 [details] Patch

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?

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

Re-opened since this is blocked by bug 111175

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