[webkit-reviews] review denied: [Bug 7450] elementAtPoint is expensive and should return a smart dictionary : [Attachment 6747] Implementation using a smart dictionary (round 4)

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 7450: elementAtPoint is expensive and should return a smart dictionary
http://bugzilla.opendarwin.org/show_bug.cgi?id=7450

Attachment 6747: Implementation using a smart dictionary (round 4)
http://bugzilla.opendarwin.org/attachment.cgi?id=6747&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The _targetWebFrame method assumes that URLElement is a DOMHTMLAnchorElement,
but that's not right. If you look at the code in WebCore you can see that a
URLElement can be an HTMLAreaElement, an HTMLImageElement with a usemap
attribute, an HTMLAnchorElement with an href attribute, or an SVGAElement with
an XLink::href attribute. So we can get method-not-found if we just call target
without checking the type.

I'm setting this review- just for the _targetWebFrame issue. If you fix that
it's fine to land it with "Reviewed by Darin" without an additional round of
review.

Here are a few other minor nitpicks:

In all those replace calls, it can just be '\\', doesn't have to be
QChar('\\').

In getInnerNonSharedNode:, when there's no renderer, it should probably set the
results to nil instead of leaving them untouched. The single caller is
initializing all of them to nil anyway, so it won't matter in practice for us.

You apparently didn't heed my complaint about the inconsistency between
_nilValues and _cache in objectForKey: -- there's still a nil check for
_nilValues and no nil check for _cache. I suggest checking neither for nil or
checking both for nil.

I preferred the old version of objectForKey: that would only look at the
_nilValues set if [_cache objectForKey:] returned nil.