Bug 123160 – HTML input type objects should be managed through std::unique_ptr

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

HTML input type objects should be managed through std::unique_ptr

[Zan Dobersek]

Created attachment 214851 [details]
Patch

[Zan Dobersek]

Created attachment 214852 [details]
Patch

[Anders Carlsson]

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]

Created attachment 214889 [details]
Patch

[Build Bot]

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]

Created attachment 214935 [details]
EWS try

[Build Bot]

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]

Created attachment 214945 [details]
Patch

[Build Bot]

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]

Created attachment 214965 [details]
Patch

[Build Bot]

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 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]

(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]

Created attachment 215257 [details]
Patch

[Build Bot]

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 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 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]

Created attachment 215303 [details]
EWS try

[Zan Dobersek]

(In reply to comment #18)
> Created an attachment (id=215303) [details]
> EWS try

Using an unrolled loop somehow helped, apparently.

[Zan Dobersek]

Created attachment 215314 [details]
Patch

Uploading for a final(?) review, thanks in advance.

[Darin Adler]

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]

Created attachment 215324 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 215324 [details]
Patch

Clearing flags on attachment: 215324

Committed r158171: <http://trac.webkit.org/changeset/158171>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.