[webkit-reviews] review granted: [Bug 17513] Add ability for image maps to be focused via tabbing : [Attachment 47365] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 25 17:14:32 PST 2010


Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 17513: Add ability for image maps to be focused via tabbing
https://bugs.webkit.org/show_bug.cgi?id=17513

Attachment 47365: patch
https://bugs.webkit.org/attachment.cgi?id=47365&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +void HTMLAreaElement::dispatchBlurEvent()
> +{
> +    Node::dispatchBlurEvent();
> +    
> +    // On a blur, we might need to remove our focus rings by repainting.
> +    updateFocusAppearance(false);
> +}

If you are intentionally skipping base class implementations of
dispatchBlurEvent, then you should include a comment saying why.

If you are not, then the call should be to
HTMLAnchorElement::dispatchBlurEvent, rather than Node::dispatchBlurEvent, even
though it will end up calling the same function. In case someone overrides
later in between.

>  bool HTMLAreaElement::supportsFocus() const
>  {
> +    // As long as this is a link and the tabIndex was not explicitly set to
-1.
> +    return isLink();
>  }

The comment here is inconsistent with the code. Please fix that by changing the
comment somehow. The comment should briefly explain both why isLink is the
right function to call and why we can't just use the version of the
supportsFocus function we inherit from the base class.

> +    String mapName = getName().string();

Since this is a member, I suggest just using m_name instead of getName(). And I
don't think you need a local variable for it.

> +    CGContextBeginTransparencyLayer(context, NULL);

WebKit code uses 0 instead of NULL.

> +    RetainPtr<CGColorRef> colorRef;
> +    if (color.isValid())
> +	   colorRef.adoptCF(createCGColor(color));

We really should change createCGColor to return a RetainPtr.

> +	   Path path = areaElement->getPath(this);
> +	   Vector<Path> focusRingPaths;
> +	   focusRingPaths.append(path);

I don't think we need the local variable "path" here.

r=me


More information about the webkit-reviews mailing list