54105 2011-02-09 07:32:25 -0800 Add convenience method toHTMLElement(Node*) 2011-02-09 13:33:11 -0800 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 yael webkit-unassigned abarth commit-queue eric webkit.review.bot oldest_to_newest 348223 0 yael 2011-02-09 07:32:25 -0800 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. 348225 1 81812 yael 2011-02-09 07:35:35 -0800 Created attachment 81812 Patch. 348229 2 81812 tonikitoo 2011-02-09 07:42:33 -0800 Comment on attachment 81812 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());? 348234 3 81812 darin 2011-02-09 07:48:05 -0800 Comment on attachment 81812 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. 348240 4 yael 2011-02-09 07:55:25 -0800 (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() :) 348325 5 81812 commit-queue 2011-02-09 09:46:53 -0800 Comment on attachment 81812 Patch. Clearing flags on attachment: 81812 Committed r78073: <http://trac.webkit.org/changeset/78073> 348326 6 commit-queue 2011-02-09 09:46:58 -0800 All reviewed patches have been landed. Closing bug. 348510 7 webkit.review.bot 2011-02-09 13:23:18 -0800 http://trac.webkit.org/changeset/78073 might have broken GTK Linux 64-bit Debug 348521 8 yael 2011-02-09 13:33:11 -0800 (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 :) 81812 2011-02-09 07:35:35 -0800 2011-02-09 09:46:53 -0800 Patch. 54105.patch text/plain 1387 yael SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDc4MDUyKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEtMDItMDkgIFlhZWwgQWhh cm9uICA8eWFlbC5haGFyb25Abm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIEFkZCBjb252ZW5pZW5jZSBtZXRob2QgdG9IVE1MRWxlbWVu dChOb2RlKikKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTU0MTA1CisKKyAgICAgICAgTm8gbmV3IHRlc3RzIHNpbmNlIG5vIG5ldyBmdW5jdGlvbmFsaXR5 IGludHJvZHVjZWQuCisKKyAgICAgICAgKiBodG1sL0hUTUxFbGVtZW50Lmg6CisgICAgICAgIChX ZWJDb3JlOjp0b0hUTUxFbGVtZW50KToKKwogMjAxMS0wMi0wOSAgUGF2ZWwgRmVsZG1hbiAgPHBm ZWxkbWFuQGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBZdXJ5IFNlbWlraGF0 c2t5LgpJbmRleDogU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRWxlbWVudC5oCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTEVsZW1lbnQuaAkocmV2aXNpb24gNzgwNTEpCisr KyBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxFbGVtZW50LmgJKHdvcmtpbmcgY29weSkKQEAgLTEw OCw2ICsxMDgsMTggQEAgcHJpdmF0ZToKICAgICBIVE1MRm9ybUVsZW1lbnQqIHNoYWRvd0FuY2Vz dG9yT3duZXJGb3JtKCk7CiB9OwogCitpbmxpbmUgSFRNTEVsZW1lbnQqIHRvSFRNTEVsZW1lbnQo Tm9kZSogbm9kZSkKK3sKKyAgICBBU1NFUlQoIW5vZGUgfHwgbm9kZS0+aXNIVE1MRWxlbWVudCgp KTsKKyAgICByZXR1cm4gc3RhdGljX2Nhc3Q8SFRNTEVsZW1lbnQqPihub2RlKTsKK30KKworaW5s aW5lIGNvbnN0IEhUTUxFbGVtZW50KiB0b0hUTUxFbGVtZW50KGNvbnN0IE5vZGUqIG5vZGUpCit7 CisgICAgQVNTRVJUKCFub2RlIHx8IG5vZGUtPmlzSFRNTEVsZW1lbnQoKSk7CisgICAgcmV0dXJu IHN0YXRpY19jYXN0PGNvbnN0IEhUTUxFbGVtZW50Kj4obm9kZSk7Cit9CisKIGlubGluZSBIVE1M RWxlbWVudDo6SFRNTEVsZW1lbnQoY29uc3QgUXVhbGlmaWVkTmFtZSYgdGFnTmFtZSwgRG9jdW1l bnQqIGRvY3VtZW50KQogICAgIDogU3R5bGVkRWxlbWVudCh0YWdOYW1lLCBkb2N1bWVudCwgQ3Jl YXRlSFRNTEVsZW1lbnQpCiB7Cg==