[webkit-reviews] review canceled: [Bug 93581] Web Inspector: Feature Request - Adding mouse gesture for editing attribute values in elements/css panel : [Attachment 157707] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has canceled sam
<dsam2912 at gmail.com>'s request for review:
Bug 93581: Web Inspector: Feature Request - Adding mouse gesture for editing
attribute values in elements/css panel
https://bugs.webkit.org/show_bug.cgi?id=93581

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

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157707&action=review


> Source/WebCore/inspector/front-end/TextPrompt.js:228
> +	   return;

Such "return"s usually do not have rich semantics in JS :)

> Source/WebCore/inspector/front-end/UIUtils.js:314
> +  */

add "@return {string}" here

> Source/WebCore/inspector/front-end/UIUtils.js:315
> +WebInspector._valueModificationDirection = function(event)

This needs to be fixed to account for the zero event.wheelDeltaY value (as we
discussed in chat, you don't want to handle the horizontal scrolling). See
comments below.

> Source/WebCore/inspector/front-end/UIUtils.js:317
> +    var direction = false;

Please use "null" instead of "false" to be type-consistent

> Source/WebCore/inspector/front-end/UIUtils.js:319
> +	   if (Math.abs(event.wheelDeltaY) == event.wheelDeltaY)

Please use simpler checks:

if (event.wheelDeltaY > 0)
    direction = "Up";
else if (event.wheelDeltaY < 0)
    direction = "Down";

> Source/WebCore/inspector/front-end/UIUtils.js:326
> +	   else if(event.keyIdentifier === "Down" || event.keyIdentifier ===
"PageDown")

missing whitespace after "if"

> Source/WebCore/inspector/front-end/UIUtils.js:339
>      var number = parseInt(hexString, 16);

Add

if (!direction)
    return hexString;

before this line. You don't want to handle horizontal mouse scrolling.

> Source/WebCore/inspector/front-end/UIUtils.js:375
> +    var arrowKeyOrMouseWheelEvent = (event.keyIdentifier === "Up" ||
event.keyIdentifier === "Down" || event.type === "mousewheel");

Add

if (!direction)
    return number;

before this line. You don't want to handle horizontal mouse scrolling.

> Source/WebCore/inspector/front-end/UIUtils.js:410
> +    var mouseWheelScroll = (event.type === "mousewheel");

You don't need this var.

> Source/WebCore/inspector/front-end/UIUtils.js:411
> +    if (!arrowKeyPressed && !pageKeyPressed && !mouseWheelScroll)

if (!arrowKeyOrMouseWheelEvent && !pageKeyPressed)