[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