[Webkit-unassigned] [Bug 188742] [GLIB] Expose JavaScriptCore options in GLib public API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=188742

--- Comment #8 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 349005
  --> https://bugs.webkit.org/attachment.cgi?id=349005
WIP

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

Informal r+ from me, with nits: there's a few typos in the documentation
comments, and a couple of places where formatting could be improved, but
functionality wise the patch LGTM.

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:36
> + * API call. Most of the options are only useful for testing and degungging.

Typo: deggunging → debugging

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:38
> + * your own risk (you can find the list of options in WebKit source code).

Grammar: …in WebKit source code → …in the WebKit source code.

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:484
> + * format [!]<low>[:<high>] where low and high are #guint values.

Probably I would set the format string with monospaced font, to make
it stand out in the generated documentation, putting the string inside
a <tt> or <code> block would do.

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:486
> + * the range, unless ! is used to invert the range.

Same, I would use <code>!</code> so the sign does not go unnoticed when reading
through the documentation.

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:512
> + * the range, unless ! is used to invert the range.

Same as above regarding using <tt> or <code> for the format string and the ! sign.

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:587
> + * @description: (nullable): the option description, or %NULL

Probably it would be worth adding a note mentioning that the option
descriptions are only in English and not localized (I think that's
the case, right?)

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:592
> + * Returns: %TRUE to top the iteration, or %FALSE otherwise

Typo: top → stop

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:641
> + * jsc-<option>=<value>. The group is set up to automatically set the #JSCOptions

I think this is missing the double-dash at the front of the option
name: --jsc-<option>=<value> — and as with the others above, I would
put the command line example string into <tt> or <code> element.

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:642
> + * on parsing, applications only need to pass the group to g_option_context_add_group().

Maybe it's only me, by “automatically set the JSCOptions on parsing” did not
make me realize what is exactly done, and only after reading the code below
and seeing that each generated GOptionEntry is assigned a callback I realized
what it means. If I may suggest a different wording, I would use something
like: “Each entry in the returned #GOptionGroup is configured to apply the
corresponding setting during command line parsing. Applications only need to
pass the returned group to g_option_context_add_group(), and the rest will
be taken care for automatically.”

Dunno, I am not a native English speaker anyway, so maybe your wording is
fine, or a better one than ours is possible ;-)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180906/fcf98ca9/attachment.html>