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
Patch (45.22 KB, patch)
2013-10-22 06:39 PDT, Zan Dobersek
no flags
Patch (48.21 KB, patch)
2013-10-22 14:58 PDT, Zan Dobersek
no flags
EWS try (48.18 KB, patch)
2013-10-23 00:34 PDT, Zan Dobersek
no flags
Patch (48.14 KB, patch)
2013-10-23 05:11 PDT, Zan Dobersek
no flags
Patch (48.37 KB, patch)
2013-10-23 08:26 PDT, Zan Dobersek
no flags
Patch (49.23 KB, patch)
2013-10-26 12:17 PDT, Zan Dobersek
no flags
EWS try (49.12 KB, patch)
2013-10-28 07:12 PDT, Zan Dobersek
no flags
Patch (49.12 KB, patch)
2013-10-28 09:51 PDT, Zan Dobersek
no flags
Patch (49.31 KB, patch)
2013-10-28 12:38 PDT, Zan Dobersek
no flags
Zan Dobersek
Comment 1 2013-10-22 06:34:50 PDT
Zan Dobersek
Comment 2 2013-10-22 06:39:33 PDT
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
Build Bot
Comment 5 2013-10-22 15:55:18 PDT
Zan Dobersek
Comment 6 2013-10-23 00:34:46 PDT
Build Bot
Comment 7 2013-10-23 01:20:08 PDT
Zan Dobersek
Comment 8 2013-10-23 05:11:17 PDT
Build Bot
Comment 9 2013-10-23 05:54:30 PDT
Zan Dobersek
Comment 10 2013-10-23 08:26:09 PDT
Build Bot
Comment 11 2013-10-23 09:07:32 PDT
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
Build Bot
Comment 15 2013-10-26 12:48:43 PDT
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
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
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.