Bug 59005

Created attachment 95935 [details] Patch

Comment on attachment 95935 [details] Patch Attachment 95935 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8757833

Created attachment 95952 [details] Oh good lord I hate DOMUtilitiesPrivate.

Comment on attachment 95952 [details] Oh good lord I hate DOMUtilitiesPrivate. View in context: https://bugs.webkit.org/attachment.cgi?id=95952&action=review I’m saying review- because I want the toXXX function to match our other toXXX functions (the ones in WebCore, not in Chromium WebKit). Otherwise this looks great > Source/WebCore/css/html.css:525 > +input[type=file]::-webkit-file-upload-button { Don’t you need quote marks around “file” as with the other rules in this file? > Source/WebCore/html/HTMLInputElement.cpp:1846 > +HTMLInputElement* toHTMLInputElement(Node* node) > +{ > + return node && node->hasTagName(inputTag) ? static_cast<HTMLInputElement*>(node) : 0; > +} These toXXX functions normally assert rather than returning 0. They are the equivalent of static_cast, not of dynamic_cast.

Comment on attachment 95952 [details] Oh good lord I hate DOMUtilitiesPrivate. View in context: https://bugs.webkit.org/attachment.cgi?id=95952&action=review >> Source/WebCore/css/html.css:525 >> +input[type=file]::-webkit-file-upload-button { > > Don’t you need quote marks around “file” as with the other rules in this file? Nope. The quote marks aren't necessary there. >> Source/WebCore/html/HTMLInputElement.cpp:1846 >> +} > > These toXXX functions normally assert rather than returning 0. They are the equivalent of static_cast, not of dynamic_cast. Yeah, you're right. But I really could use this type of function, because it makes the callsites cleaner. Perhaps we could name it asHTMLInputElement?

Comment on attachment 95952 [details] Oh good lord I hate DOMUtilitiesPrivate. View in context: https://bugs.webkit.org/attachment.cgi?id=95952&action=review >>> Source/WebCore/css/html.css:525 >>> +input[type=file]::-webkit-file-upload-button { >> >> Don’t you need quote marks around “file” as with the other rules in this file? > > Nope. The quote marks aren't necessary there. Perhaps you could remove the others, then. The inconsistency is irritating. >>> Source/WebCore/html/HTMLInputElement.cpp:1846 >>> +} >> >> These toXXX functions normally assert rather than returning 0. They are the equivalent of static_cast, not of dynamic_cast. > > Yeah, you're right. But I really could use this type of function, because it makes the callsites cleaner. Perhaps we could name it asHTMLInputElement? I think that the difference between the two would be unclear if one was “to” and the other was “as”. I agree that if we come up with the right naming scheme we can have both the static_cast-like and dynamic_cast-like versions.

Comment on attachment 95952 [details] Oh good lord I hate DOMUtilitiesPrivate. View in context: https://bugs.webkit.org/attachment.cgi?id=95952&action=review >>>> Source/WebCore/css/html.css:525 >>>> +input[type=file]::-webkit-file-upload-button { >>> >>> Don’t you need quote marks around “file” as with the other rules in this file? >> >> Nope. The quote marks aren't necessary there. > > Perhaps you could remove the others, then. The inconsistency is irritating. Sure thing. I'll make a separate patch. >>>> Source/WebCore/html/HTMLInputElement.cpp:1846 >>>> +} >>> >>> These toXXX functions normally assert rather than returning 0. They are the equivalent of static_cast, not of dynamic_cast. >> >> Yeah, you're right. But I really could use this type of function, because it makes the callsites cleaner. Perhaps we could name it asHTMLInputElement? > > I think that the difference between the two would be unclear if one was “to” and the other was “as”. I agree that if we come up with the right naming scheme we can have both the static_cast-like and dynamic_cast-like versions. I can just localize this problem as a helper function in DragController for now.

Comment on attachment 95952 [details] Oh good lord I hate DOMUtilitiesPrivate. View in context: https://bugs.webkit.org/attachment.cgi?id=95952&action=review >>>>> Source/WebCore/html/HTMLInputElement.cpp:1846 >>>>> +} >>>> >>>> These toXXX functions normally assert rather than returning 0. They are the equivalent of static_cast, not of dynamic_cast. >>> >>> Yeah, you're right. But I really could use this type of function, because it makes the callsites cleaner. Perhaps we could name it asHTMLInputElement? >> >> I think that the difference between the two would be unclear if one was “to” and the other was “as”. I agree that if we come up with the right naming scheme we can have both the static_cast-like and dynamic_cast-like versions. > > I can just localize this problem as a helper function in DragController for now. FYI: We have Node::toInputElement(). I think it's a leftover of HTML/WML abstraction.

(In reply to comment #8) > (From update of attachment 95952 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95952&action=review > > >>>>> Source/WebCore/html/HTMLInputElement.cpp:1846 > >>>>> +} > >>>> > >>>> These toXXX functions normally assert rather than returning 0. They are the equivalent of static_cast, not of dynamic_cast. > >>> > >>> Yeah, you're right. But I really could use this type of function, because it makes the callsites cleaner. Perhaps we could name it asHTMLInputElement? > >> > >> I think that the difference between the two would be unclear if one was “to” and the other was “as”. I agree that if we come up with the right naming scheme we can have both the static_cast-like and dynamic_cast-like versions. > > > > I can just localize this problem as a helper function in DragController for now. > > FYI: We have Node::toInputElement(). I think it's a leftover of HTML/WML abstraction. That's a neat idea: toFoo(Bar*) is like static_cast Bar::toFoo() is like dynamic_cast Darin, what do you think?

Created attachment 96024 [details] Uses toInputElement.

(In reply to comment #10) > Created an attachment (id=96024) [details] > Uses toInputElement. I also changed to use double-quotes in html.css, for consistency.

(In reply to comment #9) > (In reply to comment #8) > > FYI: We have Node::toInputElement(). I think it's a leftover of HTML/WML abstraction. > > That's a neat idea: > > toFoo(Bar*) is like static_cast > Bar::toFoo() is like dynamic_cast > > Darin, what do you think? I think the distinction there is too subtle.

I don’t think the dynamic_cast version is all that great, because null pointers are not all that great. Having to say if (isInputElement) first before calling toInputElement seems better than dynamic_cast style.

The one good thing about the dynamic_cast style is that you can’t “use it wrong”. But that benefit goes away if you also have static_cast, especially if the two are hard to tell apart.

Committed r88115: <http://trac.webkit.org/changeset/88115>