Bug 54105 – Add convenience method toHTMLElement(Node*)

• Component: DOM
• Status: RESOLVED FIXED

[Yael]

While working on https://bugs.webkit.org/show_bug.cgi?id=50916 I noticed that we are missing this convenience method.
There are numerous places in WebCore editing and accessibility that cast a Node to HTMLElement, and would benefit from this convenience method.

[Yael]

Created attachment 81812 [details]
Patch.

[Antonio Gomes]

Comment on attachment 81812 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=81812&action=review

Yeal, you should probably convert the callsites and add the helper altogether?

> Source/WebCore/html/HTMLElement.h:113
> +    ASSERT(!node || node->isHTMLElement());

would not it be clearer if the assert was ASSERT(node && node->isHTMLElement());?

[Darin Adler]

Comment on attachment 81812 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=81812&action=review

This seems OK, but I have two thoughts.

One is that we want to do this for multiple element classes, not just HTMLElement. The other is that we want to use this type of checked cast whenever we cast. This helps us find bad casts.

I wouldn’t call it a “convenience method”, I would call it a “checked cast function”.

>> Source/WebCore/html/HTMLElement.h:113
>> +    ASSERT(!node || node->isHTMLElement());
> 
> would not it be clearer if the assert was ASSERT(node && node->isHTMLElement());?

Clearer, but incorrect, because we want to allow 0.

Also, when an assert involves && we normally write it as two asserts.

[Yael]

(In reply to comment #2)
> (From update of attachment 81812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81812&action=review
> 
> Yeal, you should probably convert the callsites and add the helper altogether?
> 
> > Source/WebCore/html/HTMLElement.h:113
> > +    ASSERT(!node || node->isHTMLElement());
> 
> would not it be clearer if the assert was ASSERT(node && node->isHTMLElement());?

Perhaps, I simply copied this from toElement() :)

[WebKit Commit Bot]

Comment on attachment 81812 [details]
Patch.

Clearing flags on attachment: 81812

Committed r78073: <http://trac.webkit.org/changeset/78073>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78073 might have broken GTK Linux 64-bit Debug

[Yael]

(In reply to comment #7)
> http://trac.webkit.org/changeset/78073 might have broken GTK Linux 64-bit Debug

This patch added 2 methods without using them. I think it is safe to assume it did not break the GTK bot :)