[webkit-reviews] review denied: [Bug 105285] Web Inspector: [Styles] Implement navigation to UI locations of property names/values in the source code : [Attachment 184448] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 105285: Web Inspector: [Styles] Implement navigation to UI locations of
property names/values in the source code
https://bugs.webkit.org/show_bug.cgi?id=105285

Attachment 184448: Patch
https://bugs.webkit.org/attachment.cgi?id=184448&action=review

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
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.