[webkit-reviews] review granted: [Bug 7827] Fixed some crashes in event dispatch and settled the question of when getDocument() can return NULL : [Attachment 7135] Slightly improved changelog, removed erroneous fixme

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 7827: Fixed some crashes in event dispatch and settled the question of when
getDocument() can return NULL
http://bugzilla.opendarwin.org/show_bug.cgi?id=7827

Attachment 7135: Slightly improved changelog, removed erroneous fixme
http://bugzilla.opendarwin.org/attachment.cgi?id=7135&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I'm not so happy with the name EventTarget for the cast to EventTargetNodeImpl.
We should figure out whether we're going to start doing these for new classes
and if so, what our naming scheme will be. For example, if this was named
EventTargetNode it would confict with the class name (after the Impl removal
happens). So if you wanted one of these for HTMLElement, you'd want to name it
HTMLElement, but you couldn't.

And in fact, I believe the KDOM EventTargetImpl will conflict with your new
EventTarget function, so this is going to prevent me from doing the rename
tonight!

Should EventTargetNode be EventTarget instead? Should the cast to
EventTargetNodeImpl be named toEventTarget or asEventTarget or
castToEventTarget instead of EventTarget?

Ideally the getDocument() that can never return nil would be defined only on
EventTargetNodeImpl, and the one that can sometimes return nil would have a
different name and be defined on NodeImpl. That way if you errneously assumed
it was non-nil you would be likely to fail compiling. Maybe we could take "get"
out of the name too.

For the JavaScript things you moved into DOMElement, are you sure they don't
work on text nodes?

Also a note: The long-term way to do the updating of layout is that it should
be done *in the DOM*, not in the JavaScript bindings.

Fine to land as-is, but we will have to resolve the issue that prevents me from
landing tonight. Maybe just eliminating KDOM::EventTargetImpl is the best
solution to that.