[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