[webkit-reviews] review granted: [Bug 229001] Web Inspector: Do not show contextual documentation popup in the Changes panel : [Attachment 435615] Patch v2.1

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 229001: Web Inspector: Do not show contextual documentation popup in the
Changes panel
https://bugs.webkit.org/show_bug.cgi?id=229001

Attachment 435615: Patch v2.1

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




--- Comment #8 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 435615
  --> https://bugs.webkit.org/attachment.cgi?id=435615
Patch v2.1

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

r=me

> Source/WebInspectorUI/ChangeLog:7
> +	   Add config option to WI.SpreadsheetStyleProperty to prevent showing
a contextual documentation button.

Alternatively, you maybe could just use CSS to always `display: none` the
button.  I don't have a preference as to the approach, so I'll leave it up to
you :)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:37
> +	   this._hideDocumentation = options.hideDocumentation || false;

NIT: While there's nothing wrong with this, it does have the possibility of
keeping alive an object if the caller leverages falsy-conversion (e.g. if
someone did this for some reason `new WI.SpreadsheetStyleProperty(delegate,
cssProperty, {readOnly: true, hideDocumentation: delegate})` to only hide the
documentation if there's a delegate).  For boolean values I usually prefer to
`!!options.hideDocumentation` instead as that ensures we don't keep anything
alive.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:220
> +	   if (!this._hideDocumentation)

NIT: Rather than having this check outside, can we put it inside
`_addContextualDocumentationButton` so that we don't have to worry about some
future engineer adding it without checking `_hideDocumentation`?