Bug 67175

Summary: Get rid of toInputElement()
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: FormsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, adele, darin, dglazkov, gustavo.noronha, gustavo, tkent, webkit.review.bot, xan.lopez
Priority: P2 Keywords: EasyFix
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 67262    
Bug Blocks:    
Attachments:
Description Flags
work in progress
webkit-ews: commit-queue-
work in progress 2
none
Patch darin: review+, webkit.review.bot: commit-queue-

Ryosuke Niwa
Reported Tuesday, August 30, 2011 2:48:43 AM UTC
Yet another left over from WML :(
Attachments
work in progress (18.85 KB, patch)
2011-08-30 11:22 PDT, Ryosuke Niwa
webkit-ews: commit-queue-
work in progress 2 (20.77 KB, patch)
2011-08-30 13:03 PDT, Ryosuke Niwa
no flags
Patch (33.38 KB, patch)
2011-08-30 14:30 PDT, Ryosuke Niwa
darin: review+
webkit.review.bot: commit-queue-
Ryosuke Niwa
Comment 1 Tuesday, August 30, 2011 7:22:34 PM UTC
Created attachment 105656 [details] work in progress It's clear to me that we need some sort of a helper function.
Early Warning System Bot
Comment 2 Tuesday, August 30, 2011 7:48:50 PM UTC
Comment on attachment 105656 [details] work in progress Attachment 105656 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9566459
Gyuyoung Kim
Comment 3 Tuesday, August 30, 2011 7:52:11 PM UTC
Comment on attachment 105656 [details] work in progress Attachment 105656 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9564521
WebKit Review Bot
Comment 4 Tuesday, August 30, 2011 8:07:12 PM UTC
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
Ryosuke Niwa
Comment 5 Tuesday, August 30, 2011 9:03:19 PM UTC
Created attachment 105677 [details] work in progress 2
Ryosuke Niwa
Comment 6 Tuesday, August 30, 2011 10:30:11 PM UTC
Darin Adler
Comment 7 Tuesday, August 30, 2011 10:45:10 PM UTC
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.
Ryosuke Niwa
Comment 8 Wednesday, August 31, 2011 12:03:46 AM UTC
(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.
Ryosuke Niwa
Comment 9 Wednesday, August 31, 2011 12:28:00 AM UTC
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.
WebKit Review Bot
Comment 10 Wednesday, August 31, 2011 12:30:09 AM UTC
Comment on attachment 105698 [details] Patch Attachment 105698 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9565603
Ryosuke Niwa
Comment 11 Wednesday, August 31, 2011 2:19:21 AM UTC
Ryosuke Niwa
Comment 12 Wednesday, August 31, 2011 5:46:31 AM UTC
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.
Ryosuke Niwa
Comment 13 Wednesday, August 31, 2011 5:47:09 AM UTC
The patch was reverted in http://trac.webkit.org/changeset/94149.
Collabora GTK+ EWS bot
Comment 14 Wednesday, August 31, 2011 12:11:02 PM UTC
Darin Adler
Comment 15 Wednesday, August 31, 2011 6:12:11 PM UTC
Sorry for leading you astray, Ryosuke. I still don't like the virtual function so much, but we can probably live with this.
WebKit Review Bot
Comment 16 Thursday, September 1, 2011 2:54:18 AM UTC
Comment on attachment 105698 [details] Patch Attachment 105698 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9580034
WebKit Review Bot
Comment 17 Thursday, September 1, 2011 4:05:11 AM UTC
Comment on attachment 105698 [details] Patch Attachment 105698 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9578194
Note You need to log in before you can comment on or make changes to this bug.