Bug 163608

Summary: Leverage new union type support for HTMLSelectElement.add() / HTMLOptionsCollection.add()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, rniwa, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-10-18 11:58:45 PDT
Leverage new union type support for HTMLSelectElement.add() / HTMLOptionsCollection.add():
- https://html.spec.whatwg.org/#htmlselectelement
- https://html.spec.whatwg.org/#htmloptionscollection
Comment 1 Chris Dumez 2016-10-18 12:06:40 PDT
Created attachment 291965 [details]
Patch
Comment 2 Chris Dumez 2016-10-18 12:44:50 PDT
Created attachment 291969 [details]
Patch
Comment 3 Chris Dumez 2016-10-18 12:59:29 PDT
Created attachment 291975 [details]
Patch
Comment 4 Chris Dumez 2016-10-18 13:32:52 PDT
Created attachment 291979 [details]
Patch
Comment 5 Ryosuke Niwa 2016-10-18 15:17:15 PDT
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.
Comment 6 Chris Dumez 2016-10-18 15:34:35 PDT
(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.
Comment 7 Ryosuke Niwa 2016-10-18 15:37:12 PDT
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.
Comment 8 Chris Dumez 2016-10-18 15:54:37 PDT
(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(
Comment 9 Chris Dumez 2016-10-18 16:21:03 PDT
Comment on attachment 291979 [details]
Patch

Clearing flags on attachment: 291979

Committed r207497: <http://trac.webkit.org/changeset/207497>
Comment 10 Chris Dumez 2016-10-18 16:21:08 PDT
All reviewed patches have been landed.  Closing bug.