[webkit-reviews] review denied: [Bug 170779] Web Inspector: Modify CSS number values with up key and down key : [Attachment 323845] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 15 15:33:56 PDT 2017


Matt Baker <mattbaker at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 170779: Web Inspector: Modify CSS number values with up key and down key
https://bugs.webkit.org/show_bug.cgi?id=170779

Attachment 323845: Patch

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




--- Comment #7 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 323845
  --> https://bugs.webkit.org/attachment.cgi?id=323845
Patch

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

r-, only because this needs a test for the new EditingSupport function.

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:244
> +	       } else

I think this would be cleaner as:

if (!modified)
    return;

event.preventDefault();
...

Actually, preventDefault gets called on line 250 below, so you should only need
the early return.

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:280
> +WI.modifyNumericValue = function(element, modifyValue)

This was existing code, but it would still be good to add a layout test now
that it's included in EditingSupport.

I think it could use a better name too. I didn't really know what `modifyValue`
was, or what this function did to `element` until I got to the new code in
SpreadsheetTextField later. What do you think about:

WI.incrementElementValue = function(element, amount)
{
    ....
}

I think "numeric" is implied, but it could be `incrementNumericElementValue` if
you think that's more descriptive. Kind of a long name though.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:236
> +		   modifyValue /= 10;

Instead of multiplying and dividing, you could do:

let amount;
if (event.metaKey)
    amount = 100;
else if (event.shiftKey)
    amount = 10;
else if (event.altKey)
    amount = 0.1;
else
    amount = 1;

if (event.key === "ArrowDown")
    amount = -amount;


More information about the webkit-reviews mailing list