[webkit-reviews] review denied: [Bug 185930] Web Inspector: Styles: overridden CSS property should have go-to button to jump to effective property : [Attachment 363610] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied  review:
Bug 185930: Web Inspector: Styles: overridden CSS property should have go-to
button to jump to effective property
https://bugs.webkit.org/show_bug.cgi?id=185930

Attachment 363610: Patch

https://bugs.webkit.org/attachment.cgi?id=363610&action=review




--- Comment #9 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 363610
  --> https://bugs.webkit.org/attachment.cgi?id=363610
Patch

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

r- because of a couple general comments:

I'm worried that CSSProperty _overridden and _overriddenBy could get out of
sync. What about dropping _overridden and replacing the public property with:

get overridden()
{
    return !!this._overriddingProperty;
}

You'd need to eliminate the WI.CSSProperty constructor argument `overridden`,
or replaced it with `overriddingProperty`. This could be an issue in
DOMNodeStyles.prototype._parseStylePropertyPayload, which constructs a
WI.CSSProperty and sets overridden equal to true if payload.status.inactive. If
this is too annoying to refactor, you can disregard this but be sure to fix the
issue in CSSProperty.prototype.update.

My other comment is style.

1) The spacing between the property value, goto arrow, and overriding property
value feels too cramped.
2. The overriding property value hint should be shown with the arrow. It feels
odd having it appear only when the goto arrow is hovered.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:34
> +	   this._overriddenBy = null;

What about _overridingProperty?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:266
> +	       this._overriddenBy = null;

This should also be nulled out in update() when !overridden.