[webkit-reviews] review denied: [Bug 67571] window.HTMLSpanElement does not exist : [Attachment 106273] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 67571: window.HTMLSpanElement does not exist
https://bugs.webkit.org/show_bug.cgi?id=67571

Attachment 106273: Patch
https://bugs.webkit.org/attachment.cgi?id=106273&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106273&action=review


> Source/WebCore/html/HTMLSpanElement.h:35
> +    static PassRefPtr<HTMLSpanElement> create(Document*);

While it would be nice to have this function if we ever wanted to create a span
element in general purpose code, I doubt that will come up. I’d suggest leaving
it out.

> Source/WebCore/html/HTMLSpanElement.h:38
> +protected:

I don’t think we expect to derive classes from this one, so I suggest private
rather than public.

> Source/WebCore/html/HTMLSpanElement.idl:2
> + * Copyright (C) 2006, 2010 Apple Inc. All rights reserved.

Probably the wrong copyright here.

> LayoutTests/ChangeLog:11
> +	   * fast/dom/html-span-element-exists-expected.txt: Added.
> +	   * fast/dom/html-span-element-exists.html: Added.

While it’s OK to add a new test, I think there are existing tests that cover
all the different element types, it would be better to add to those rather than
adding a whole new test just for the span element. Specifically,
fast/dom/wrapper-classes.html is about this very subject. In case that test
does not have sufficient coverage, things like the constructor and prototype
should be tested along with the other DOM constructors and prototypes.

fast/dom/wrapper-classes.html will start failing so I am going to say review-
on this patch because I presume that means we haven’t succeeded at running the
tests with this patch yet