[webkit-reviews] review granted: [Bug 224318] Web Inspector: Audit Tab: Edits are not committed when leaving edit mode unless you first click into another text field : [Attachment 426253] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 224318: Web Inspector: Audit Tab: Edits are not committed when leaving edit
mode unless you first click into another text field
https://bugs.webkit.org/show_bug.cgi?id=224318

Attachment 426253: Patch

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:329
> +	   if (!this.representedObject.editable)
> +	       return;

NIT: Rather than repeating this from `super.saveEditedData`, could we move this
check to `_handleEditingChanged` so that we don't even call `saveEditedData`
unless `this.representedObject.editable` (you could `console.assert` this
inside `WI.AuditTestContentView`)?

> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:331
> +	   console.assert(this._testCodeMirror);

NIT: IMO these kinds of `console.assert` really don't serve any purpose because
if it fails we'd immediately throw an error when trying to use it on the next
line.  I think `console.assert` really are more useful for knowing things
_about_ the object (e.g. checking that the parameter is of a particular type),
not just that its truthy.  I'd just drop it.

> Source/WebInspectorUI/UserInterface/Views/AuditTestContentView.js:322
> +	   console.assert(this._setupCodeMirror);

ditto (AuditTestCaseContentView.js:331)