Bug 54105 - Add convenience method toHTMLElement(Node*)
Summary: Add convenience method toHTMLElement(Node*)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-09 07:32 PST by Yael
Modified: 2011-02-09 13:33 PST (History)
4 users (show)

See Also:


Attachments
Patch. (1.35 KB, patch)
2011-02-09 07:35 PST, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2011-02-09 07:32:25 PST
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.
Comment 1 Yael 2011-02-09 07:35:35 PST
Created attachment 81812 [details]
Patch.
Comment 2 Antonio Gomes 2011-02-09 07:42:33 PST
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());?
Comment 3 Darin Adler 2011-02-09 07:48:05 PST
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.
Comment 4 Yael 2011-02-09 07:55:25 PST
(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() :)
Comment 5 WebKit Commit Bot 2011-02-09 09:46:53 PST
Comment on attachment 81812 [details]
Patch.

Clearing flags on attachment: 81812

Committed r78073: <http://trac.webkit.org/changeset/78073>
Comment 6 WebKit Commit Bot 2011-02-09 09:46:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 WebKit Review Bot 2011-02-09 13:23:18 PST
http://trac.webkit.org/changeset/78073 might have broken GTK Linux 64-bit Debug
Comment 8 Yael 2011-02-09 13:33:11 PST
(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 :)