[Webkit-unassigned] [Bug 3248] Mouse events on OPTION element seem to be ignored

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 5 12:57:54 PST 2009


https://bugs.webkit.org/show_bug.cgi?id=3248


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27357|review?                     |review-
               Flag|                            |




------- Comment #10 from darin at apple.com  2009-02-05 12:57 PDT -------
(From update of attachment 27357)
> +        * 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-.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.



More information about the webkit-unassigned mailing list