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

[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 6708: Implementation using a smart dictionary (round 2)
http://bugzilla.opendarwin.org/attachment.cgi?id=6708&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
If you look at Frame::backslashAsCurrencySymbol, you'll see that it's all done
through the document. So this code can be written so that it works even when
there's no Frame. Sorry I didn't notice that before. Please do that. Perhaps
you could put the function in DocumentImpl, or you could just do the
document()->decoder()->codec() thing yourself.

Identifiers like _WebElementSelf should not start with an underscore. First of
all, there's no need to worry about conflict with end-user stuff since they are
internal to the framework and the source file. Second, underscore followed by
capital letter is reserved for the system and library.

In WebElementMethodMap, the "object" should be the enumeration type, not
unsigned. I also suggest naming that structure WebElementMethod, since it's not
a map.

I think NSMapTable is deprecated -- they encourage using CFDictionary instead,
and that seems pretty easy to do. Or you could use an NSDictionary with NSValue
for the values. But I could be wrong on this.

Inside WebKit files, #import "" is fine for WebKit headers. Some older files do
#import <WebKit/>, but that's overkill.

All those checks after calling malloc on each node in initializeLookupTable
aren't needed. When you're doing a small malloc during initialization checking
the return value of malloc isn't really helpful. I don't think that function
needs to have a return value.

Since the pattern malloc, set up, insert into map is repeated over and over
again, I suggest making a function to do it, or putting the values into an
array and then looping through the array to insert into the map.

The dealloc method doesn't have to nil things out, probably best not to.

+    else if(mapItem->object == _WebElementInnerNonSharedNode)

Needs a space after the if.

Also, the different mapItem->object values should be a switch statement.

Why the check for object == _WebElementSelf before checking
respondsToSelector:? It seems fine to call respondsToSelector: on self.

+    if (!_URLElement) return nil;

Should be broken up over two lines.

I'm concerned that the nodes are released in dealloc and returned from methods.
They might need to be autoreleased by the methods or by dealloc in case the
caller gets the node then releases the dictionary. Not sure about this one.

Why doesn't this patch remove all the WebCoreXXXKey constants from
WebCoreFrameBridge?