[webkit-reviews] review denied: [Bug 3248] Mouse events on OPTION element seem to be ignored : [Attachment 27357] First attempt

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 3248: Mouse events on OPTION element seem to be ignored
https://bugs.webkit.org/show_bug.cgi?id=3248

Attachment 27357: First attempt
https://bugs.webkit.org/attachment.cgi?id=27357&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   * html/HTMLSelectElement.cpp:
> +	   (WebCore::HTMLSelectElement::listBoxDefaultEventHandler):
> +	   * rendering/RenderListBox.cpp:
> +	   (WebCore::RenderListBox::nodeAtPoint):
> +	   * rendering/RenderListBox.h:

Per-function and per-file comments are preferred.

> @@ -1743,6 +1760,7 @@
>	   (WebCore::RenderLayer::setHasCompositingDescendant):
>	   Maintain a bit to tell if this layer has composited descendants.
>  
> +
>  2009-02-03  Simon Fraser  <simon.fraser at apple.com>

Why add a blank line here?

> +	   // convert to coords relative to the list box if needed

Sentence-form comments with capital letters and periods are preferred.

>	   MouseEvent* mEvt = static_cast<MouseEvent*>(evt);

The name mouseEvent seems much better than "mEvt"; maybe you should change it
while modifying this code.

> +	   Node* targ = evt->target()->toNode();

How about "target" rather than "targ"?

> +	   if (targ != this) {
> +	       FloatPoint absPos =
renderer()->absoluteToLocal(FloatPoint(offsetX, offsetY), false, true);
> +	       offsetX = absPos.x();
> +	       offsetY = absPos.y();
> +	   }

Do we really need to optimize the "targ == this" case? Is there a downside to
always calling absoluteToLocal?

> +bool RenderListBox::nodeAtPoint(const HitTestRequest& request,
HitTestResult& result, int _x, int _y, int _tx, int _ty, HitTestAction
hitTestAction)

Why the underscores in the argument names here? Just "x" and "y" would be
better than "_x" and "_y". And it's bizarre to have both "_tx" and "tx" as
names in the same function.

> +    bool ret = RenderBlock::nodeAtPoint( request, result, _x, _y, _tx, _ty,
hitTestAction );
> +    if ( !ret )
> +	   return false;

Extra spaces here. The name "ret" is too short. Maybe just put the nodeAtPoint
call inside the if instead of including a local variable.

> +    int size = numItems();

The right type for the size of a vector is size_t. Why are we calling
numItems() rather than calling size() on the vector? I worry that we might walk
off the end of the vector.

> +    for (int listIndex = 0;listIndex < size;++listIndex) {

The rights size of a vector index is size_t. Need spaces after semicolons here.
The name listIndex seems kind of long. How about just "i"? It's strange that
for the index you chose a long name, but for the rectangle you chose "r".

> +	       HTMLElement *node = listItems[listIndex];

The * needs to go next to the type name, not the variable name.

Enough things need fixing that I'll say review-.