Summary: | Add convenience method toHTMLElement(Node*) | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Yael
2011-02-09 07:32:25 PST
Created attachment 81812 [details]
Patch.
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 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. (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 on attachment 81812 [details] Patch. Clearing flags on attachment: 81812 Committed r78073: <http://trac.webkit.org/changeset/78073> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/78073 might have broken GTK Linux 64-bit Debug (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 :) |