[webkit-reviews] review granted: [Bug 123160] HTML input type objects should be managed through std::unique_ptr : [Attachment 214965] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 123160: HTML input type objects should be managed through std::unique_ptr
https://bugs.webkit.org/show_bug.cgi?id=123160

Attachment 214965: Patch
https://bugs.webkit.org/attachment.cgi?id=214965&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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);
    }