[webkit-reviews] review denied: [Bug 36965] Web Inspector: start editing DOM and styles on click-and-pause. : [Attachment 52301] [PATCH] Proposed change.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 1 09:46:22 PDT 2010
Timothy Hatcher <timothy at hatcher.name> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 36965: Web Inspector: start editing DOM and styles on click-and-pause.
https://bugs.webkit.org/show_bug.cgi?id=36965
Attachment 52301: [PATCH] Proposed change.
https://bugs.webkit.org/attachment.cgi?id=52301&action=review
------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> + this._clickAndPauseHelper = new
ClickAndPauseHelper(this.listItemElement, this._clickAndPause.bind(this), true,
true);
I think a better name for this would be ClickAndPauseGestureRecognizer.
> - if (this.isEventWithinDisclosureTriangle(event))
> - return;
Why isn't this needed anymore?
> _dblclickSelector: function(event)
> {
> + this._clickAndPauseHelper.dblClick(event);
Why do you need to call dblClick when ClickAndPauseHelper listens for dblclick?
> ondblclick: function(event)
> {
> + this._clickAndPauseHelper.dblClick(event);
Ditto.
> +function ClickAndPauseHelper(node, listener, externalClickDispatch,
externalDblClickDispatch)
I'd like to see this in a new file. Utilities.js should just be DOM additions
and simple functions. No state machines.
> + if (!externalClickDispatch)
> + if (!externalDblClickDispatch)
Oh I guess this is why dblClick needs called.
> + click: function(e)
I think handleClick or processClick would be better names.
> + this._clickX = e.pageX;
> + this._clickY = e.pageY;
I don't think you need these, see below.
> + this._timer = setTimeout(this._fireEvent.bind(this), 300);
You want to call reset before making a new timer, if the use clicks twice but
isn't a double click.
> + dblClick: function(e)
I think handleDoubleClick or processDoubleClick would be better names.
> + _mouseMove: function(e)
> + {
> + if (this._clickX !== e.pageX || this._clickY !== e.pageY)
> + this.reset();
> + },
Resetting on any mouse movement is bad, since some people are twitchy with the
mouse after a click. Either have some slop room (like 5-10 pixels in any
direction) or remove this altogether. The Finder allows you to move the mouse
any distance after the click, so I say remove the mousemove code.
We are not fully safe from click-drag to select, because you operate on click
not mouse down. So you can click then quickly mouse down and darg and editing
wont be prevented. So I thin you need to call reset on mousedown too. You will
also need to make sure the mouseup after drag is not counded as a click, so it
wont start the timer.
More information about the webkit-reviews
mailing list