Bug 67571 – window.HTMLSpanElement does not exist

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

window.HTMLSpanElement does not exist

[Adam Barth]

Created attachment 106271 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 106271 [details]
Patch

Attachment 106271 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9589336

[Gyuyoung Kim]

Comment on attachment 106271 [details]
Patch

Attachment 106271 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9590265

[Early Warning System Bot]

Comment on attachment 106271 [details]
Patch

Attachment 106271 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9584961

[Adam Barth]

Created attachment 106273 [details]
Patch

[Adam Barth]

(It helps if I "svn add" the code.)

[WebKit Review Bot]

Comment on attachment 106273 [details]
Patch

Attachment 106273 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9589354

New failing tests:
fast/dom/gc-6.html
fast/js/toString-and-valueOf-override.html
editing/selection/extend-after-mouse-selection.html
fast/dom/gc-7.html
fast/dom/html-span-element-exists.html
fast/dom/wrapper-classes.html
fast/tokenizer/external-script-document-write.html
fast/events/mouseclick-target-and-positioning.html

[Darin Adler]

Comment on attachment 106273 [details]
Patch

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

[Adam Barth]

(In reply to comment #8)
> (From update of attachment 106273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106273&action=review
> 
> > Source/WebCore/html/HTMLSpanElement.idl:2
> > + * Copyright (C) 2006, 2010 Apple Inc. All rights reserved.
> 
> Probably the wrong copyright here.

I wasn't sure what to do about the copyright block here.  I based it on HTMLDivElement.idl (which is where this copyright block comes from), but it's also so simple that it seems like it should get a fresh block.

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

Will do.  Thanks for the review.

[Adam Barth]

Created attachment 106275 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106275 [details]
Patch

Attachment 106275 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9591233

[Early Warning System Bot]

Comment on attachment 106275 [details]
Patch

Attachment 106275 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9584987

[Gyuyoung Kim]

Comment on attachment 106275 [details]
Patch

Attachment 106275 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9591234

[Adam Barth]

Comment on attachment 106275 [details]
Patch

Today is not my day for uploading patches!

[Adam Barth]

Created attachment 106280 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106280 [details]
Patch

Attachment 106280 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9591251

New failing tests:
fast/js/toString-and-valueOf-override.html
fast/dom/wrapper-classes.html

[Sam Weinig]

Any idea if there are others we are missing?

[Adam Barth]

> Any idea if there are others we are missing?

Yeah.  There are a bunch more.  You can read them off from HTMLTagNames.in.

[Adam Barth]

Created attachment 106366 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 106366 [details]
Patch for landing

Clearing flags on attachment: 106366

Committed r94545: <http://trac.webkit.org/changeset/94545>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

*** Bug 12333 has been marked as a duplicate of this bug. ***