Bug 59005 - Convert file <input> to use the new shadow DOM model
Summary: Convert file <input> to use the new shadow DOM model
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: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 61892 61979
Blocks: 48698 52788
  Show dependency treegraph
 
Reported: 2011-04-20 09:45 PDT by Dominic Cooney
Modified: 2011-06-04 10:14 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.12 KB, patch)
2011-06-03 11:48 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Oh good lord I hate DOMUtilitiesPrivate. (22.37 KB, patch)
2011-06-03 13:10 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Uses toInputElement. (18.94 KB, patch)
2011-06-04 09:17 PDT, Dimitri Glazkov (Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 2011-04-20 09:45:09 PDT
RenderFileUploadControl uses setShadowHost via ShadowInputElement. Only ShadowRoot instances should have shadow hosts.
Comment 1 Dimitri Glazkov (Google) 2011-06-03 11:48:51 PDT
Created attachment 95935 [details]
Patch
Comment 2 WebKit Review Bot 2011-06-03 12:18:34 PDT
Comment on attachment 95935 [details]
Patch

Attachment 95935 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8757833
Comment 3 Dimitri Glazkov (Google) 2011-06-03 13:10:31 PDT
Created attachment 95952 [details]
Oh good lord I hate DOMUtilitiesPrivate.
Comment 4 Darin Adler 2011-06-03 17:46:20 PDT
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 5 Dimitri Glazkov (Google) 2011-06-03 17:52:55 PDT
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 6 Darin Adler 2011-06-03 18:01:53 PDT
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 7 Dimitri Glazkov (Google) 2011-06-03 18:19:53 PDT
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 8 Kent Tamura 2011-06-03 19:37:09 PDT
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.
Comment 9 Dimitri Glazkov (Google) 2011-06-03 20:52:39 PDT
(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?
Comment 10 Dimitri Glazkov (Google) 2011-06-04 09:17:40 PDT
Created attachment 96024 [details]
Uses toInputElement.
Comment 11 Dimitri Glazkov (Google) 2011-06-04 09:18:36 PDT
(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.
Comment 12 Darin Adler 2011-06-04 09:36:40 PDT
(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.
Comment 13 Darin Adler 2011-06-04 09:39:26 PDT
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.
Comment 14 Darin Adler 2011-06-04 09:40:08 PDT
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.
Comment 15 Dimitri Glazkov (Google) 2011-06-04 10:14:31 PDT
Committed r88115: <http://trac.webkit.org/changeset/88115>