Leverage new union type support for HTMLSelectElement.add() / HTMLOptionsCollection.add(): - https://html.spec.whatwg.org/#htmlselectelement - https://html.spec.whatwg.org/#htmloptionscollection
Created attachment 291965 [details] Patch
Created attachment 291969 [details] Patch
Created attachment 291975 [details] Patch
Created attachment 291979 [details] Patch
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.
(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 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.
(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 on attachment 291979 [details] Patch Clearing flags on attachment: 291979 Committed r207497: <http://trac.webkit.org/changeset/207497>
All reviewed patches have been landed. Closing bug.