[webkit-reviews] review granted: [Bug 227411] Web Inspector: Styles: Autocomplete should support mid-line completions : [Attachment 443703] Patch v1.1.1 - Remove stray commented-out code

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 227411: Web Inspector: Styles: Autocomplete should support mid-line
completions
https://bugs.webkit.org/show_bug.cgi?id=227411

Attachment 443703: Patch v1.1.1 - Remove stray commented-out code

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




--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 443703
  --> https://bugs.webkit.org/attachment.cgi?id=443703
Patch v1.1.1 - Remove stray commented-out code

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

r=me, neat! :)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:393
> +	   if (!this._suggestionsView ||
this._preventDiscardingCompletionsOnKeyUp || this._keyDownCaretPosition ===
this._getCaretPosition() || this._keyDownCaretPosition === -1)

I feel like only the things related to `_keyDownCaretPosition` are actually
related to this comment.  Perhaps we can make a separate `if
(this._preventDiscardingCompletionsOnKeyUp)` and `if (!this._suggestionsView)`
so that things are a bit nicer to read?

NIT: I'd put `this._keyDownCaretPosition === -1` earlier so that we only do
`this._getCaretPosition()` as a last resort

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:475
> +	   // The window's selection range will only contain the current line's
positioning in multiline text, so a new

NIT: Can we move this comment down a bit so it's closer to the relevant code? 
The early-return doesn't appear to be related to this.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:548
> +	       console.assert(textChildNode instanceof Text, textChildNode);

I don't think it's actually possible for this to not be true given that you
just set the `textContent` ��