[webkit-reviews] review denied: [Bug 17513] Add ability for image maps to be focused via tabbing : [Attachment 47250] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 47250: Patch
https://bugs.webkit.org/attachment.cgi?id=47250&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   if
(reinterpret_cast<AccessibilityImageMapLink*>(child)->areaElement() ==
areaElement)

This should be static_cast, not reinterpret_cast. We should almost never use a
reinterpret_cast.

> +	   return
focusedImageMapUIElement(reinterpret_cast<HTMLAreaElement*>(focusedNode));

Ditto.

> +    static AccessibilityObject* focusedImageMapUIElement(HTMLAreaElement*
areaElement);

This argument should not have a name.

> +    virtual bool isImageMapLink() const { return true; }

This sort of function should be private, because it’s a programming error to
call it if you have a pointer of the specific type. 

> +    return reinterpret_cast<HTMLMapElement*>(mapElement)->imageElement();

static_cast

> +void HTMLAreaElement::updateFocusAppearance(bool restorePreviousSelection)
> +{
> +    Node* parent = parentNode();
> +    if (parent && parent->hasTagName(mapTag)) {
> +	   HTMLImageElement* imageElement =
reinterpret_cast<HTMLMapElement*>(parent)->imageElement();
> +
> +	   // This will handle scrolling to the image if necessary.
> +	   imageElement->updateFocusAppearance(restorePreviousSelection);
> +	   
> +	   if (imageElement) {
> +	       RenderObject* imageRenderer = imageElement->renderer();
> +	       if (imageRenderer)
> +		   imageRenderer->setNeedsLayout(true);
> +	   }
> +    }
> +}

We normally prefer early returns rather than nesting the function inside if
statements.

The cast needs to be a static_cast.

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

Are you sure this can’t just be a call to tabIndex()? As far as I can tell it
should give you the same value, and the code would be less strange.

I’m not sure this is the right rule. Will this change tabbing for all
documents? Why is this different from the rule for links?

> +    Path getPath(RenderObject*) const;

Normally we don’t use the word “get” in the names of functions that return a
value. We reserve it for functions that return values in unusual ways, with out
arguments. I think the name of the function is not clear enough. Sure it's a
path, but what does the path represent?

> +    String mapName = getName().string().lower();
> +    RefPtr<HTMLCollection> coll = renderer()->document()->images();
> +    for (Node* curr = coll->firstItem(); curr; curr = coll->nextItem()) {
> +	   if (!curr->hasTagName(imgTag))
> +	       continue;
> +	   
> +	   // The HTMLImageElement's useMap() value includes the '#' symbol at
the beginning,
> +	   // which has to be stripped off.
> +	   HTMLImageElement* imageElement =
static_cast<HTMLImageElement*>(curr);
> +	   String useMapName =
imageElement->getAttribute(usemapAttr).string().substring(1).lower();
> +	   if (useMapName == mapName)
> +	       return imageElement;
> +    }

I know this is not new code, but it seems strange to call lower on both strings
instead of equalIgnoringCase.

> +    Document* doc = mapElement->document();
> +    if (!doc)
> +	   return;

Please use the name "document" for the local variable instead of the
abbreviation "doc".

> +    // FIXME: Clip the paths to the image bounding box.

Why aren't you doing that? Seems easy to do.

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

You don't need the local variable "path" here.

> +    virtual void paint(PaintInfo& paintInfo, int tx, int ty);

I would make this private unless there's a reason not to. Or protected at
least.

Please omit the argument name "paintInfo".

> +    void paintFocusRings(PaintInfo&, const RenderStyle* style);

Please omit the argument name "style".

> -	       graphicsContext->drawFocusRing(focusRingRects, ow,
style()->outlineOffset(), oc);
> +	       graphicsContext->drawFocusRingRects(focusRingRects, ow,
style()->outlineOffset(), oc);

C++ has overloading, so you don't have to rename the function. You can just
provide two versions, one that works with a vector of paths and the other that
works with a vector of rects.

> +    friend class RenderImage;

This friendship seems really unfortunate. Can we avoid this somehow?

Did we discuss whether the change in tabbing behavior is good all the time? Did
you talk to anyone else who works on WebKit about that change?