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

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

------- Additional Comments from Darin Adler <darin at apple.com>
Looks great! I almost want to do review+, but I have so many comments (although
each one is minor) that I'm going to review- this one more time:

It's a bit unfortunate introducing all these uses of QString -- we're trying to
remove all use of QString. Ideally all these displayString methods would share
a single function. But in any case, WebCore::String has a perfectly good
replace function, which we would use and can convert itself to NSString without
a function, so we could write:

+    return [self _HTMLElementImpl]->title().replace(QChar('\\'), [self
_elementImpl]->getDocument()->backslashAsCurrencySymbol());

+    QChar DocumentImpl::backslashAsCurrencySymbol() const;

The above is wrong. Should not include the class name DocumentImpl there.

Maybe we should change URLElement to actually be an ElementImpl instead of a
NodeImpl. It's annoying to have the code here know that it's always an element,
but not have the code it's bridging know. (Probably not something you have to
fix in this patch though; just cleanup for later.)

WebElementDictionary.h should have a copyright date of 2006, not 2003, and
WebElementDictionary.m have 2006 instead of 2005 unless there is code in those
files published earlier which requires more than one year to be listed.

We should probably import <Foundation/Foundation.h> rather than individual
Foundation headers in WebKit code.

I prefer a blank line after:

+#import "WebElementDictionary.h"

and before the other includes.

I don't think addLookupKey needs an "if (!lookupTable)" at the start of it;
there's no practical case where that can be nil and if it's nil a crash is
probably the best we can do.

There's no reason that addLookupKey needs to use CFAllocatorAllocate instead of
malloc, is there?

Since we never plan to delete any values out of the lookup table, nor delete
the table itself, we can pass NULL for the value callbacks instead of &vcb.

I think it's a little fragile to have the hardcoded constant 12 and then 12
calls to addLookupKey below. It seems highly likely that someone would add a
new call without noticing the number 12 they should be updating. I'm not sure
what the best fix is for this. Passing 0 is probably OK, even though we will
give up a tiny bit of efficiency by doing so.

Since all callers check _cacheComplete before calling _fillCache, I think it
should not also check _cacheComplete. An assert perhaps, but not an if ...
return.

_fillCache does not need to initialize the key to nil.

I think that _fillCache should use CFDictionaryApplyFunction instead of casting
to NSDictionary and using keyEnumerator. It would be pretty easy to do that,
passing self through as the context, and then we would not be relying on
toll-free bridging at all for the lookup table.

+    id value = [_cache objectForKey:key];
+    if (value || _cacheComplete || (_nilValues && [_nilValues
containsObject:key]))
+	 return value;

It's a little inconsistent here to just use _cache without checking it for nil
and then checking _nilValues for nil before calling containsObject:. I think
you should check either both or neither.

I think you should do:

    typedef struct { ... } WebElementMethod;

and then refer to it as WebElementMethod everywhere instead of struct
WebElementMethod.

Since CFDictionaryGetCount is used no matter what in objectForKey: I suggest
putting it into a local variable. The local variable can be an unsigned type,
which will obviate the need for the (unsigned long) cast in the _cacheComplete
check.

This:

    _cacheComplete = ([_cache count] + [_nilValues count]) == lookupTableCount

will generate slightly better code than if (x) y = YES, so I suggest using that
form.