| Summary: | Convert file <input> to use the new shadow DOM model | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dominic Cooney <dominicc> | ||||||||
| Component: | DOM | Assignee: | Dimitri Glazkov (Google) <dglazkov> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | darin, dglazkov, morrita, rolandsteiner, tkent, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Bug Depends on: | 61892, 61979 | ||||||||||
| Bug Blocks: | 48698, 52788 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Dominic Cooney
2011-04-20 09:45:09 PDT
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> |