Bug 163608 – Leverage new union type support for HTMLSelectElement.add() / HTMLOptionsCollection.add()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Leverage new union type support for HTMLSelectElement.add() / HTMLOptionsCollection.add():
- https://html.spec.whatwg.org/#htmlselectelement
- https://html.spec.whatwg.org/#htmloptionscollection

[Chris Dumez]

Created attachment 291965 [details]
Patch

[Chris Dumez]

Created attachment 291969 [details]
Patch

[Chris Dumez]

Created attachment 291975 [details]
Patch

[Chris Dumez]

Created attachment 291979 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 291979 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291979&action=review

> Source/WebCore/html/HTMLOptionsCollection.h:46
> -    WEBCORE_EXPORT void add(HTMLElement&, HTMLElement* beforeElement, ExceptionCode&);
> -    WEBCORE_EXPORT void add(HTMLElement&, int beforeIndex, ExceptionCode&);
> +    using OptionOrOptGroupElement = std::experimental::variant<RefPtr<HTMLOptionElement>, RefPtr<HTMLOptGroupElement>>;
> +    using HTMLElementOrInt = std::experimental::variant<RefPtr<HTMLElement>, int>;
> +    WEBCORE_EXPORT ExceptionOr<void> add(const OptionOrOptGroupElement&, Optional<HTMLElementOrInt> before);

I'm not really sure if this is an improvement...
I think the old overloaded function was easier to read & understand.

[Chris Dumez]

(In reply to comment #5)
> Comment on attachment 291979 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291979&action=review
> 
> > Source/WebCore/html/HTMLOptionsCollection.h:46
> > -    WEBCORE_EXPORT void add(HTMLElement&, HTMLElement* beforeElement, ExceptionCode&);
> > -    WEBCORE_EXPORT void add(HTMLElement&, int beforeIndex, ExceptionCode&);
> > +    using OptionOrOptGroupElement = std::experimental::variant<RefPtr<HTMLOptionElement>, RefPtr<HTMLOptGroupElement>>;
> > +    using HTMLElementOrInt = std::experimental::variant<RefPtr<HTMLElement>, int>;
> > +    WEBCORE_EXPORT ExceptionOr<void> add(const OptionOrOptGroupElement&, Optional<HTMLElementOrInt> before);
> 
> I'm not really sure if this is an improvement...
> I think the old overloaded function was easier to read & understand.

But the new IDL matches the specification and the generator now generates more correct code as shown by the newly passing W3C test.

[Ryosuke Niwa]

Comment on attachment 291979 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291979&action=review

> Source/WebCore/html/HTMLSelectElement.cpp:233
> +        beforeElement = std::experimental::visit(visitor, before.value());

I think it's more readable to define visitor inside the call to std::experimental::visit since it's not used elsewhere.

> Source/WebCore/html/HTMLSelectElement.cpp:235
> +    HTMLElement& toInsert = std::experimental::visit([](const auto& htmlElement) -> HTMLElement& {

As in here.

[Chris Dumez]

(In reply to comment #7)
> Comment on attachment 291979 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291979&action=review
> 
> > Source/WebCore/html/HTMLSelectElement.cpp:233
> > +        beforeElement = std::experimental::visit(visitor, before.value());
> 
> I think it's more readable to define visitor inside the call to
> std::experimental::visit since it's not used elsewhere.
> 
> > Source/WebCore/html/HTMLSelectElement.cpp:235
> > +    HTMLElement& toInsert = std::experimental::visit([](const auto& htmlElement) -> HTMLElement& {
> 
> As in here.

Hmm, I personally think it gets too crowded if we pass more than 1 lambda to std::visit().

I followed the pattern used by Sam so far:
Source/WebCore/dom/MessageEvent.cpp:    auto visitor = WTF::makeVisitor(
Source/WebCore/dom/Node.cpp:    auto visitor = WTF::makeVisitor(
Source/WebCore/dom/Node.cpp:    auto visitor = WTF::makeVisitor(

[Chris Dumez]

Comment on attachment 291979 [details]
Patch

Clearing flags on attachment: 291979

Committed r207497: <http://trac.webkit.org/changeset/207497>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.