[webkit-reviews] review granted: [Bug 75345] Enable the [Supplemental] IDL on CMake : [Attachment 120802] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 75345: Enable the [Supplemental] IDL on CMake
https://bugs.webkit.org/show_bug.cgi?id=75345

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120802&action=review


This looks good to me.

> Source/WebCore/ChangeLog:35
> +	   * UseJSC.cmake: Described the above build.

Nit: This remark doesn't read well (*). I would either remove this remark or
revise it. One suggestion is: "Modified to reflect the new build flow as
described above."

(*) The remark doesn't read well because the word "describe" neither clarifies
whether this patch modifies the file UseJSC.cmake nor what the modification is.
(Although the presence of this line in the commit message indicates that either
UseJSC.cmake was modified or newly added.) Moreover, the past tense usage of
the word "describe" gives the impression that UseJSC.cmake either already
implements or has a comment that describes the supplemental build flow ("new
build flow"). Together these issues make it difficult to understand this
remark.

> Source/WebCore/ChangeLog:36
> +	   * UseV8.cmake: Exactly the same change as UseJSC.cmake, except for
"JS" or "V8".

Nit: Since the purpose of this remark is to indicate that a similar change to
the build flow was made to file UseV8.cmake, I would just write "Ditto."

> Source/WebCore/CMakeLists.txt:373
>      webaudio/DelayNode.idl
> +    webaudio/DOMWindowWebAudio.idl

Nit: These lines should be swapped such that this section of the list of IDL
files is in sorted order according to the UNIX sort command.