Bug 67175 - Get rid of toInputElement()
Summary: Get rid of toInputElement()
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: EasyFix
Depends on: 67262
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-29 18:48 PDT by Ryosuke Niwa
Modified: 2011-10-25 11:34 PDT (History)
9 users (show)

See Also:


Attachments
work in progress (18.85 KB, patch)
2011-08-30 11:22 PDT, Ryosuke Niwa
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
work in progress 2 (20.77 KB, patch)
2011-08-30 13:03 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (33.38 KB, patch)
2011-08-30 14:30 PDT, Ryosuke Niwa
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-08-29 18:48:43 PDT
Yet another left over from WML :(
Comment 1 Ryosuke Niwa 2011-08-30 11:22:34 PDT
Created attachment 105656 [details]
work in progress

It's clear to me that we need some sort of a helper function.
Comment 2 Early Warning System Bot 2011-08-30 11:48:50 PDT
Comment on attachment 105656 [details]
work in progress

Attachment 105656 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9566459
Comment 3 Gyuyoung Kim 2011-08-30 11:52:11 PDT
Comment on attachment 105656 [details]
work in progress

Attachment 105656 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9564521
Comment 4 WebKit Review Bot 2011-08-30 12:07:12 PDT
Comment on attachment 105656 [details]
work in progress

Attachment 105656 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9567495
Comment 5 Ryosuke Niwa 2011-08-30 13:03:19 PDT
Created attachment 105677 [details]
work in progress 2
Comment 6 Ryosuke Niwa 2011-08-30 14:30:11 PDT
Created attachment 105698 [details]
Patch
Comment 7 Darin Adler 2011-08-30 14:45:10 PDT
Comment on attachment 105698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105698&action=review

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:137
> +    if (toHTMLInputElement(host)->maxResults() < 0)
> +        return decorationId;
> +    if (toHTMLInputElement(host)->maxResults() > 0)
> +        return resultsId;

Seems a little weak to call maxResults twice here. But maybe it’s just an inlined accessor and so no additional performance cost.
Comment 8 Ryosuke Niwa 2011-08-30 16:03:46 PDT
(In reply to comment #7)
> (From update of attachment 105698 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105698&action=review
> 
> > Source/WebCore/html/shadow/TextControlInnerElements.cpp:137
> > +    if (toHTMLInputElement(host)->maxResults() < 0)
> > +        return decorationId;
> > +    if (toHTMLInputElement(host)->maxResults() > 0)
> > +        return resultsId;
> 
> Seems a little weak to call maxResults twice here. But maybe it’s just an inlined accessor and so no additional performance cost.

Huh, I didn't realize that.  Will fix before landing it.
Comment 9 Ryosuke Niwa 2011-08-30 16:28:00 PDT
Comment on attachment 105698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105698&action=review

> Source/WebCore/html/HTMLInputElement.h:362
> +    return static_cast<const HTMLInputElement*>(node);

Oops, I somehow have const here :(  Will fix also.
Comment 10 WebKit Review Bot 2011-08-30 16:30:09 PDT
Comment on attachment 105698 [details]
Patch

Attachment 105698 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9565603
Comment 11 Ryosuke Niwa 2011-08-30 18:19:21 PDT
Committed r94142: <http://trac.webkit.org/changeset/94142>
Comment 12 Ryosuke Niwa 2011-08-30 21:46:31 PDT
HTMLIsIndexElement inherits from HTMLInputElement.  Given that, I'm not sure if this refactoring is a good idea. For one, hasTagName is much slower, but also explicitly checking for the tag name might be more error-prone.
Comment 13 Ryosuke Niwa 2011-08-30 21:47:09 PDT
The patch was reverted in http://trac.webkit.org/changeset/94149.
Comment 14 Collabora GTK+ EWS bot 2011-08-31 04:11:02 PDT
Comment on attachment 105698 [details]
Patch

Attachment 105698 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9566719
Comment 15 Darin Adler 2011-08-31 10:12:11 PDT
Sorry for leading you astray, Ryosuke. I still don't like the virtual function so much, but we can probably live with this.
Comment 16 WebKit Review Bot 2011-08-31 18:54:18 PDT
Comment on attachment 105698 [details]
Patch

Attachment 105698 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9580034
Comment 17 WebKit Review Bot 2011-08-31 20:05:11 PDT
Comment on attachment 105698 [details]
Patch

Attachment 105698 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9578194