JSDOMBindings' toArguments() returns a std::pair of argumentIndex and Optional. In JS bindings code generator, use of ".first" and ".second" to access to members is hard to read. We should use a struct instead to be able to name the members.
Created attachment 288400 [details] Patch
Comment on attachment 288400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288400&action=review > Source/WebCore/bindings/js/JSDOMBinding.h:357 > +struct VariadicHelperResult { Would something simpler work? E.g. struct Result { size_t argumentIndex; Optional<Container> arguments; } Also, could we declare this struct inside the VariadicHelper struct? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-1107 > - In the future, please try to avoid this kind of unrelated spacing changes.
Comment on attachment 288400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288400&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3993 > + $value = "WTFMove($name.arguments.value())"; Why is this change needed? wouldn't this work? $value = "WTFMove(*$name.arguments)";
Created attachment 288681 [details] Patch
Thanks for the review. > > Source/WebCore/bindings/js/JSDOMBinding.h:357 > > +struct VariadicHelperResult { > > Would something simpler work? E.g. > > struct Result { > size_t argumentIndex; > Optional<Container> arguments; > } > > Also, could we declare this struct inside the VariadicHelper struct? Ok, done in the uploaded patch. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-1107 > > - > > In the future, please try to avoid this kind of unrelated spacing changes. Ok, I thought it was good to remove trailing whitespaces. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3993 > > + $value = "WTFMove($name.arguments.value())"; > > Why is this change needed? wouldn't this work? > $value = "WTFMove(*$name.arguments)"; Yes, it works also but it seems that value() is generally used in WebKit instead of * with Optional value (c.f. Bug 158835#c14).
Comment on attachment 288681 [details] Patch Clearing flags on attachment: 288681 Committed r206090: <http://trac.webkit.org/changeset/206090>
All reviewed patches have been landed. Closing bug.
Comment on attachment 288681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288681&action=review > Source/WebCore/bindings/js/JSDOMBinding.h:363 > + Result(size_t argumentIndex, Optional<Container>&& arguments) > + : argumentIndex(argumentIndex) > + , arguments(WTFMove(arguments)) > + { > + } We should not include this constructor. Member-wise initialization should accomplish the same thing.
(In reply to comment #8) > Comment on attachment 288681 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=288681&action=review > > > Source/WebCore/bindings/js/JSDOMBinding.h:363 > > + Result(size_t argumentIndex, Optional<Container>&& arguments) > > + : argumentIndex(argumentIndex) > > + , arguments(WTFMove(arguments)) > > + { > > + } > > We should not include this constructor. Member-wise initialization should > accomplish the same thing. Ok, I filed bug 162298 for this.