WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
123160
HTML input type objects should be managed through std::unique_ptr
https://bugs.webkit.org/show_bug.cgi?id=123160
Summary
HTML input type objects should be managed through std::unique_ptr
Zan Dobersek
Reported
2013-10-22 06:26:22 PDT
HTML input type objects should be managed through std::unique_ptr
Attachments
Patch
(44.69 KB, patch)
2013-10-22 06:34 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(45.22 KB, patch)
2013-10-22 06:39 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(48.21 KB, patch)
2013-10-22 14:58 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
EWS try
(48.18 KB, patch)
2013-10-23 00:34 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(48.14 KB, patch)
2013-10-23 05:11 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(48.37 KB, patch)
2013-10-23 08:26 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(49.23 KB, patch)
2013-10-26 12:17 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
EWS try
(49.12 KB, patch)
2013-10-28 07:12 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(49.12 KB, patch)
2013-10-28 09:51 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(49.31 KB, patch)
2013-10-28 12:38 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2013-10-22 06:34:50 PDT
Created
attachment 214851
[details]
Patch
Zan Dobersek
Comment 2
2013-10-22 06:39:33 PDT
Created
attachment 214852
[details]
Patch
Anders Carlsson
Comment 3
2013-10-22 06:52:57 PDT
Comment on
attachment 214852
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=214852&action=review
> Source/WebCore/html/ButtonInputType.cpp:40 > - return adoptPtr(new ButtonInputType(element)); > + return std::unique_ptr<ButtonInputType>(new ButtonInputType(element));
instead of this pattern, you should just use std::make_unique. Get rid of the create functions and make the constructors public instead.
Zan Dobersek
Comment 4
2013-10-22 14:58:44 PDT
Created
attachment 214889
[details]
Patch
Build Bot
Comment 5
2013-10-22 15:55:18 PDT
Comment on
attachment 214889
[details]
Patch
Attachment 214889
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/9418009
Zan Dobersek
Comment 6
2013-10-23 00:34:46 PDT
Created
attachment 214935
[details]
EWS try
Build Bot
Comment 7
2013-10-23 01:20:08 PDT
Comment on
attachment 214935
[details]
EWS try
Attachment 214935
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/7768088
Zan Dobersek
Comment 8
2013-10-23 05:11:17 PDT
Created
attachment 214945
[details]
Patch
Build Bot
Comment 9
2013-10-23 05:54:30 PDT
Comment on
attachment 214945
[details]
Patch
Attachment 214945
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/8358069
Zan Dobersek
Comment 10
2013-10-23 08:26:09 PDT
Created
attachment 214965
[details]
Patch
Build Bot
Comment 11
2013-10-23 09:07:32 PDT
Comment on
attachment 214965
[details]
Patch
Attachment 214965
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/9918012
Darin Adler
Comment 12
2013-10-23 17:50:43 PDT
Comment on
attachment 214965
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=214965&action=review
review+ as long as you remove the peculiar use of std::move in populateInputTypeFactoryMap. Would be nice to eliminate the unrolled loop that builds the map. Use the pattern I’ve been using in other populate functions, with a C array data table and a loop to add to the map.
> Source/WebCore/html/ButtonInputType.h:40 > + ButtonInputType(HTMLInputElement& element) : BaseButtonInputType(element) { }
explicit
> Source/WebCore/html/CheckboxInputType.h:40 > + CheckboxInputType(HTMLInputElement& element) : BaseCheckableInputType(element) { }
explicit
> Source/WebCore/html/HTMLInputElement.cpp:459 > + std::unique_ptr<InputType> newType = InputType::create(*this, fastGetAttribute(typeAttr));
I think auto would be good here, instead of explicitly saying std::unique_ptr<InputType>.
> Source/WebCore/html/InputType.cpp:98 > + map.add(InputTypeNames::button(), std::move(createInputType<ButtonInputType>));
Why are we calling std::move on the function pointer here? That doesn’t seem helpful. This loop should not be unrolled. Instead there should be a C array data table with function pointers in it, and the map should be populated in a loop. Separate calls to map.add bloat the code.
> Source/WebCore/html/InputType.cpp:151 > + InputTypeFactoryFunction factory = typeName.isEmpty() ? nullptr : factoryMap.get().get(typeName); > + if (factory) > + return factory(element);
I would write this: if (!typeName.isEmpty()) { if (auto factory = factoryMap.get().get(typeName)) return factory(element); }
Zan Dobersek
Comment 13
2013-10-26 12:08:51 PDT
(In reply to
comment #12
)
> (From update of
attachment 214965
[details]
) > > Source/WebCore/html/InputType.cpp:98 > > + map.add(InputTypeNames::button(), std::move(createInputType<ButtonInputType>)); > > Why are we calling std::move on the function pointer here? That doesn’t seem helpful.
This was just another attempt to somehow get MSVC to compile this code. It still doesn't compile, so it's not useful at all.
Zan Dobersek
Comment 14
2013-10-26 12:17:50 PDT
Created
attachment 215257
[details]
Patch
Build Bot
Comment 15
2013-10-26 12:48:43 PDT
Comment on
attachment 215257
[details]
Patch
Attachment 215257
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/13878003
Darin Adler
Comment 16
2013-10-27 12:22:14 PDT
Comment on
attachment 215257
[details]
Patch 1>..\html\InputType.cpp(86): error C2143: syntax error : missing ';' before '=' 1>..\html\InputType.cpp(86): error C2873: 'InputTypeNameFunction' : symbol cannot be used in a using-declaration 1>..\html\InputType.cpp(86): error C2513: 'int' : no variable declared before '=' 1>..\html\InputType.cpp(86): error C2059: syntax error : '<cv-qualifer>' 1>..\html\InputType.cpp(87): error C2143: syntax error : missing ';' before '=' 1>..\html\InputType.cpp(87): error C2873: 'InputTypeFactoryFunction' : symbol cannot be used in a using-declaration 1>..\html\InputType.cpp(87): error C2513: 'int' : no variable declared before '=' 1>..\html\InputType.cpp(87): error C2059: syntax error : ')' 1>..\html\InputType.cpp(88): error C2143: syntax error : missing ';' before '=' 1>..\html\InputType.cpp(88): error C2873: 'InputTypeFactoryMap' : symbol cannot be used in a using-declaration 1>..\html\InputType.cpp(88): error C2513: 'int' : no variable declared before '=' 1>..\html\InputType.cpp(88): error C2065: 'InputTypeFactoryFunction' : undeclared identifier 1>..\html\InputType.cpp(88): error C2955: 'WTF::HashMap' : use of class template requires template argument list C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\include\private\wtf/HashMap.h(34) : see declaration of 'WTF::HashMap' 1>..\html\InputType.cpp(96): error C2065: 'InputTypeFactoryMap' : undeclared identifier 1>..\html\InputType.cpp(96): error C2065: 'map' : undeclared identifier 1>..\html\InputType.cpp(97): error C2448: 'populateInputTypeFactoryMap' : function-style initializer appears to be a function definition 1>..\html\InputType.cpp(156): error C2065: 'InputTypeFactoryMap' : undeclared identifier 1>..\html\InputType.cpp(156): error C2133: 'factoryMap' : unknown size 1>..\html\InputType.cpp(156): error C2512: 'WTF::NeverDestroyed' : no appropriate default constructor available 1>..\html\InputType.cpp(157): error C2662: 'WTF::NeverDestroyed<T>::get' : cannot convert 'this' pointer from 'WTF::NeverDestroyed' to 'WTF::NeverDestroyed<T> &' Reason: cannot convert from 'WTF::NeverDestroyed' to 'WTF::NeverDestroyed<T>' Conversion requires a second user-defined-conversion operator or constructor 1>..\html\InputType.cpp(157): error C2228: left of '.isEmpty' must have class/struct/union 1>..\html\InputType.cpp(158): error C3861: 'populateInputTypeFactoryMap': identifier not found 1>..\html\InputType.cpp(161): error C2662: 'WTF::NeverDestroyed<T>::get' : cannot convert 'this' pointer from 'WTF::NeverDestroyed' to 'WTF::NeverDestroyed<T> &' Reason: cannot convert from 'WTF::NeverDestroyed' to 'WTF::NeverDestroyed<T>' Conversion requires a second user-defined-conversion operator or constructor 1>..\html\InputType.cpp(161): error C2228: left of '.get' must have class/struct/union 1>..\html\InputType.cpp(161): fatal error C1903: unable to recover from previous error(s); stopping compilation
Darin Adler
Comment 17
2013-10-27 12:23:59 PDT
Comment on
attachment 215257
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=215257&action=review
> Source/WebCore/html/InputType.cpp:88 > +using InputTypeNameFunction = const AtomicString& (*)(); > +using InputTypeFactoryFunction = std::unique_ptr<InputType> (*)(HTMLInputElement&); > +using InputTypeFactoryMap = HashMap<AtomicString, InputTypeFactoryFunction, CaseFoldingHash>;
Looks like the version of Visual Studio that we support do not support this syntax yet; we need to use old fashioned typedef here instead.
Zan Dobersek
Comment 18
2013-10-28 07:12:25 PDT
Created
attachment 215303
[details]
EWS try
Zan Dobersek
Comment 19
2013-10-28 09:38:44 PDT
(In reply to
comment #18
)
> Created an attachment (id=215303) [details] > EWS try
Using an unrolled loop somehow helped, apparently.
Zan Dobersek
Comment 20
2013-10-28 09:51:18 PDT
Created
attachment 215314
[details]
Patch Uploading for a final(?) review, thanks in advance.
Darin Adler
Comment 21
2013-10-28 10:08:03 PDT
Comment on
attachment 215314
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=215314&action=review
> Source/WebCore/html/InputType.cpp:110 > + { RuntimeEnabledFeatures::sharedFeatures().inputTypeDateEnabled(), > + &InputTypeNames::date, &createInputType<DateInputType> },
This RuntimeEnabledFeatures part isn’t great. Because we are calling functions like RuntimeEnabledFeatures::sharedFeatures().inputTypeDateEnabled() to compute values in the array, this inputTypes array won’t be put into constant data. So there will be a bunch of code compiled just to create the array. To get the improvement we are looking for by creating the array, we don’t want any code to have to run to create the array. The way to fix that would be to use pointer to member function to store RuntimeEnabledFeatures::inputTypeDateEnabled in the array and then call it inside the loop. We would store a nullptr for non-conditional features. But pointer to member function is pretty awkward to use. I’d like to see us consider dropping RuntimeEnabledFeatures entirely. It makes things like this that should be simple unnecessarily complex. If the RuntimeEnabledFeatures object was instead a namespace, then these would be plain functions, not member functions, which would help a little.
Zan Dobersek
Comment 22
2013-10-28 12:38:02 PDT
Created
attachment 215324
[details]
Patch
WebKit Commit Bot
Comment 23
2013-10-28 21:18:46 PDT
Comment on
attachment 215324
[details]
Patch Clearing flags on attachment: 215324 Committed
r158171
: <
http://trac.webkit.org/changeset/158171
>
WebKit Commit Bot
Comment 24
2013-10-28 21:18:49 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug