Bug 229001

<rdar://problem/81792379>

Created attachment 435345 [details] Patch

Comment on attachment 435345 [details] Patch I feel like it's a bit odd for it to even be shown in the Changes panel at all 🤔

Created attachment 435607 [details] Patch v2

Comment on attachment 435607 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=435607&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:37 > + this._readOnly = options.readOnly; > + this._showDocumentation = options.showDocumentation; These probably still need to be `||`-ed and/or `&&`-ed with their default value, since if the caller only provided the `readOnly` option, or only provided the `showDocumentation` option, the other option would still be undefined. This is also why it is preferable for options in an `options` parameter to default to `false`. (e.g. instead of a `showDocumentation` option, a `hideDocumentation` option.)

Comment on attachment 435607 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=435607&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:37 >> + this._showDocumentation = options.showDocumentation; > > These probably still need to be `||`-ed and/or `&&`-ed with their default value, since if the caller only provided the `readOnly` option, or only provided the `showDocumentation` option, the other option would still be undefined. This is also why it is preferable for options in an `options` parameter to default to `false`. (e.g. instead of a `showDocumentation` option, a `hideDocumentation` option.) Oh wow! I had the mistaken expectation that JS merges the two objects, the default and provided one. Today I learned it does not. I'll revert to ` ||` `hideDocumentation` is a bit weird, since we check this option _before_ generating the DOM for it. But I understand the requirement: not to have to update `WI.SpreadSheetStyleProperty` callers elsewhere.

Created attachment 435615 [details] Patch v2.1 Add option to prevent showing contextual documentation button for CSS declarations

Comment on attachment 435615 [details] 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`?

Comment on attachment 435615 [details] Patch v2.1 View in context: https://bugs.webkit.org/attachment.cgi?id=435615&action=review >> 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 :) Ok. Let's not generate the elements at all and make that obvious. Generating and appending to document, then hiding them with CSS can get confusing during debugging in the future. >> 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. Good point. >> 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`? Yes, that sounds reasonable.

Created attachment 435685 [details] [fast-cq] Patch v2.2 Address code review comments

Committed r281139 (240593@main): <https://commits.webkit.org/240593@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435685 [details].