[webkit-reviews] review denied: [Bug 118896] Web Inspector: support click-and-drag editing of CSS numeric values : [Attachment 207392] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied Antoine Quint
<graouts at apple.com>'s request for review:
Bug 118896: Web Inspector: support click-and-drag editing of CSS numeric values
https://bugs.webkit.org/show_bug.cgi?id=118896

Attachment 207392: Patch
https://bugs.webkit.org/attachment.cgi?id=207392&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=207392&action=review


r- for now. Looking good!

> Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:289
> +	   var foundNumber = WebInspector.alterNumberInRange(amount,
codeMirror, startPosition, endPosition, true);

This is a layering violation, since this file is meant to be standalone from
the Inspector (if anyone else ever wanted to use it or merge things back to
CodeMirror.)

You should keep alterNumberInRange in this file and use
CodeMirror.defineExtension to add it to CodeMirror instances. Then the
codeMirror parameter would be removed, and you would use 'this' for the CM
instance.

> Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:340
> +    CodeMirror.defineOption("dragToAdjustNumbers", false,
function(codeMirror, value, oldValue) {
> +	   if (!codeMirror.dragToAdjustController)
> +	       codeMirror.dragToAdjustController = new
WebInspector.DragToAdjustController(codeMirror);
> +	   codeMirror.dragToAdjustController.enabled = value;
> +    });

This should live in DragToAdjustController.js like the other add-on options,
they all self-define. You could also define it as default true, so you don't
need to touch all instances and we won't forget it if we add more later.

> Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:355
> +WebInspector.alterNumberInRange = function(amount, codeMirror,
startPosition, endPosition, affectsSelection)

See previous comment.

> Source/WebInspectorUI/UserInterface/CodeMirrorOverrides.css:87
> +.CodeMirror.drag-to-adjust .CodeMirror-lines {
> +    cursor: col-resize;
> +}

This should be in a DragToAdjustController.css file to be better associated
with the class. CodeMirrorOverrides.css is only meant to correct/override some
of the default CodeMirror styles to better match our platform.

> Source/WebInspectorUI/UserInterface/ConsolePrompt.js:42
> +	   dragToAdjustNumbers: true

See comment about making this the default so you don't need to do this
everywhere.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:26
> +WebInspector.DragToAdjustController = function(codeMirror)

I feel the name DragToAdjustController is too generic. We also try to put
CodeMirror in the name for these, since they are deeply integrated with
CodeMirror and not a higher level.

I would suggest CodeMirrorDragToAlterNumberController or
CodeMirrorAlterNumberController if you want to move the rest of the alter
number code into this one file, and all under the option.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:40
> +    set active(active)

I am torn about having just setters that look like they are public (no
underscore). I would prefer them to be underscored, but then that conflicts
with the internal property names. Could these just be private methods since
there is no getter (even if it makes the call sites uglier)? Or switch the
internal properties to double underscore prefixes and have the setters have a
single underscore?

You should add a // Private comment too to separate the methods.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:49
> +	   }
> +	   else {

One line.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:67
> +	       WebInspector.elementDragEnd(window.event);

You should assert window.event somewhere. Or add event as a parameter if this
becomes a private method.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:84
> +	       this._element.addEventListener("mouseenter", this, false);
> +	       this._element.addEventListener("mouseleave", this, false);
> +	   } else {
> +	       this._element.removeEventListener("mouseenter", this, false);
> +	       this._element.removeEventListener("mouseleave", this, false);

We don't include the optional capture parameter if it is false.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:110
> +    handleEvent: function(event)

I would put this above the private methods in a "protected section" labeled
with a // Protected comment.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:152
> +	   if (this._hoveredTokenInfo &&
> +	       this._hoveredTokenInfo.line === position.line &&
> +	       this._hoveredTokenInfo.token.start === token.start &&
> +	       this._hoveredTokenInfo.token.end === token.end)
> +	       return;

Excessive wrapping. Could be two lines:

if (this._hoveredTokenInfo && this._hoveredTokenInfo.line === position.line &&
    this._hoveredTokenInfo.token.start === token.start &&
this._hoveredTokenInfo.token.end === token.end)

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:154
> +	   var containsNumber = token.type.indexOf("number") !== -1;

This works, but it also differs from the arrow key alter number logic. This way
makes more sense, but it still has the issue of <rdar://problem/13877085> where
up/down (and now drag) over the unit does not work. We should fix and
consolidate the logic for both when we fix <rdar://problem/13877085>.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:192
> +	   if (event.metaKey)

I think control would be better than command, it is closer in the key cluster
of option, shift and control. Command also seems more sacred… *waves hands*