[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
bugzilla-daemon at webkit.org
Fri Aug 10 02:57:14 PDT 2012
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.
More information about the webkit-reviews
mailing list