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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Sep 3 21:00:38 PDT 2011

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

Attachment 106273: Patch

------- 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

More information about the webkit-reviews mailing list