[webkit-reviews] review denied: [Bug 166993] Web Inspector: checkboxes in Settings screen use inappropriate layout : [Attachment 305521] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 166993: Web Inspector: checkboxes in Settings screen use inappropriate
layout
https://bugs.webkit.org/show_bug.cgi?id=166993

Attachment 305521: Patch

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




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

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

>> Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:113
>> +		let keyValuePairs = [];
> 
> NIT: this should be var 
> 
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/le
t#Temporal_dead_zone_and_errors_with_let

I forgot about this! Is the issue that all cases share a block? Does a real TDZ
risk exist with the code as written, or is this a future-proofing comment?

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.css:117
>> +body[dir=rtl] .content-view.settings > .settings-page > .container >
.value-editor input[type="number"] {
> 
> I don't think "value-editor" applies to anything.
> 
> body[dir=rtl] .content-view.settings > .settings-page > .container .editor >
input[type="number"]

Right! I also need to change `setting-page` to `setting-view`.

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:95
>> +	    this._selectedSettingsView.updateLayout();
> 
> Should you give this a LayoutReason?

Not needed. In the future if a subview implements View.prototype.layout we can
add a resize reason.

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:100
>> +	addSettingsView(SettingsView)
> 
> Style: capitalization.

Good grief.