HTML input type objects should be managed through std::unique_ptr
Created attachment 214851 [details] Patch
Created attachment 214852 [details] Patch
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.
Created attachment 214889 [details] Patch
Comment on attachment 214889 [details] Patch Attachment 214889 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/9418009
Created attachment 214935 [details] EWS try
Comment on attachment 214935 [details] EWS try Attachment 214935 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/7768088
Created attachment 214945 [details] Patch
Comment on attachment 214945 [details] Patch Attachment 214945 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/8358069
Created attachment 214965 [details] Patch
Comment on attachment 214965 [details] Patch Attachment 214965 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/9918012
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); }
(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.
Created attachment 215257 [details] Patch
Comment on attachment 215257 [details] Patch Attachment 215257 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/13878003
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
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.
Created attachment 215303 [details] EWS try
(In reply to comment #18) > Created an attachment (id=215303) [details] > EWS try Using an unrolled loop somehow helped, apparently.
Created attachment 215314 [details] Patch Uploading for a final(?) review, thanks in advance.
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.
Created attachment 215324 [details] Patch
Comment on attachment 215324 [details] Patch Clearing flags on attachment: 215324 Committed r158171: <http://trac.webkit.org/changeset/158171>
All reviewed patches have been landed. Closing bug.