[webkit-reviews] review denied: [Bug 177314] Web Inspector: Styles Redesign: support undo/redo of manual edits : [Attachment 322220] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 177314: Web Inspector: Styles Redesign: support undo/redo of manual edits
https://bugs.webkit.org/show_bug.cgi?id=177314

Attachment 322220: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 322220
  --> https://bugs.webkit.org/attachment.cgi?id=322220
Patch

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

> Source/WebInspectorUI/ChangeLog:13
> +	   Make sure WI._undoKeyboardShortcut don't call WI.undo() when editing
inside of a contentEditable element.

Grammar: "Make sure ... don't" => "Make sure ... doesn't"

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:68
> +	   if (this._style)
> +	      
this._style.removeEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged,
this._propertiesChanged, this);
> +
> +	   if (style)
> +	      
style.addEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged,
this._propertiesChanged, this);
>  
> +	   this._style = style || null;

Nice. We normally order this:

    if (this._style)
	this._style.removeEventListener(...);

    this._style = style || null;

    if (this._style)
	this._style.addEventListener(...);

Given this is a View that is adding an event listener someone should probably
be removing it when the view goes away.

    WI.SpreadsheetRulesStyleDetailsPanel
      * - WI.SpreadsheetCSSStyleDeclarationSection
	    1 - WI.SpreadsheetCSSStyleDeclarationEditor

When the Sections are destroyed, do they clear the style on the editor? If not,
then we may be abandoning a bunch of editors that are event listeners of a
WI.CSSStyleDeclaration. I didn't see anywhere where this could be cleared up.

r- for the potential leak, but if you can explain how it goes away then set
back to r? and I'll re-review.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:91
> +	   let isFocused = false;
> +
> +	   let focusedElement = document.activeElement;
> +	   if (focusedElement &&
focusedElement.isSelfOrDescendant(this.element))
> +	       isFocused = true;
> +
> +	   if (!isFocused)
> +	       this.needsLayout();

You can simplify the boolean initialization and it'll read better:

    let focusedElement = document.activeElement;
    let isFocused = focusedElement &&
focusedElement.isSelfOrDescendant(this.element);
    if (!isFocused)
	this.needsLayout()