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

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied 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 157686: Patch
https://bugs.webkit.org/attachment.cgi?id=157686&action=review

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


> Source/WebCore/inspector/front-end/TextPrompt.js:116
> +	   this._element.addEventListener("mousewheel",
this._boundOnMouseWheel, false);

You never remove this listener (see the removal code for this._boundOnKeyDown
in detach()). We had a major glitch in one of Chrome releases due to a similar
bug (hint: that is the reason why we store the bound listeners as fields). r-
for this.

> Source/WebCore/inspector/front-end/UIUtils.js:318
> +    if (event.type && event.type === "mousewheel") {

The first check (event.type) is not necessary here.

> Source/WebCore/inspector/front-end/UIUtils.js:319
> +	   if (event.wheelDelta == 120)

This check can fail in certain cases, since 120 is a single tick multiplier,
and it is possible for a wheelDelta to contain several ticks. Also, 120 is
based on the Windows port, and this may break in the future/for other ports.
E.g. chrome/src/ui/views/events/event.cc:

#if defined(OS_WIN)
// This value matches windows WHEEL_DELTA.
// static
const int MouseWheelEvent::kWheelDelta = 120;
#else
// This value matches GTK+ wheel scroll amount.
const int MouseWheelEvent::kWheelDelta = 53;
#endif

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

The "event.type" check is extraneous.
Also, a trailing semicolon is missing.