Bug 188742 - [GLIB] Expose JavaScriptCore options in GLib public API
Summary: [GLIB] Expose JavaScriptCore options in GLib public API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-20 05:09 PDT by Carlos Garcia Campos
Modified: 2019-01-24 03:11 PST (History)
5 users (show)

See Also:


Attachments
WIP (38.38 KB, patch)
2018-09-06 00:21 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP (38.70 KB, patch)
2018-09-06 06:44 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (42.45 KB, patch)
2019-01-15 02:33 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-08-20 05:09:53 PDT
I think it would be useful to expose the JSC options in the API. Right now it's possible to change them using the env vars, but it would be better to provide some API. To ensure API/ABI stability we could use a common interface to set the options, that receive the option string identifier and the value, similar to the GtkPrintSettings API, for example. Main problem is that users don't know the options available, so we could provide names for the most important options, like we do in WebKit2 API with editing commands. We could also include API to get information about the options available (name, value type, default value and description). Applications could use that to create command line options to set them, for example.

JSCOptions* jsc_options_get (void); // Get the singleton instance
gboolean jsc_options_set_string (JSCOptions* options, const char* option, const char* value);
char* jsc_options_get_string (JSCOptions* options, const char* option);
gboolean jsc_options_set_int (JSCOptions* options, const char* option, gint value);
int jsc_options_get_int (JSCOptions* options, const char* option);
......
typedef gboolean (* JSCOptionsFunc) (JSCOptions* options, const char* option, GType type, GValue* default_value, const char* description, gpointer user_data);
void jsc_options_foreach (JSCOptions* options, JSCOptionsFunc function, gpointer user_data);

/**
 * JSC_OPTION_USE_JIT:
 *
 * Allows the executable pages to be allocated for JIT and thunks if %TRUE.
 * Type: %G_TYPE_BOOLEAN
 * Default: %TRUE
 */
#define JSC_OPTION_USE_JIT "useJIT"

It would be something like that.
Comment 1 Adrian Perez 2018-08-20 05:52:09 PDT
(In reply to Carlos Garcia Campos from comment #0)
> I think it would be useful to expose the JSC options in the API. Right now
> it's possible to change them using the env vars, but it would be better to
> provide some API. To ensure API/ABI stability we could use a common
> interface to set the options, that receive the option string identifier and
> the value, similar to the GtkPrintSettings API, for example. Main problem is
> that users don't know the options available, so we could provide names for
> the most important options, like we do in WebKit2 API with editing commands.
> We could also include API to get information about the options available
> (name, value type, default value and description). Applications could use
> that to create command line options to set them, for example.
> 
> JSCOptions* jsc_options_get (void); // Get the singleton instance
> gboolean jsc_options_set_string (JSCOptions* options, const char* option,
> const char* value);
> char* jsc_options_get_string (JSCOptions* options, const char* option);
> gboolean jsc_options_set_int (JSCOptions* options, const char* option, gint
> value);
> int jsc_options_get_int (JSCOptions* options, const char* option);
> ......
> typedef gboolean (* JSCOptionsFunc) (JSCOptions* options, const char*
> option, GType type, GValue* default_value, const char* description, gpointer
> user_data);
> void jsc_options_foreach (JSCOptions* options, JSCOptionsFunc function,
> gpointer user_data);

This being like this, it is like a mini-object system, so I wonder whether it
would make more sense to make JSCSettings a class which inherits from GObject
and make each option a property instead:

 * jsc_options_get_{string,int,bool,…} → g_object_get
 * jsc_options_set_{string,int,bool,…} → g_object_set
 * jsc_options_foreach → g_object_class_list_properties

...and so on.

> /**
>  * JSC_OPTION_USE_JIT:
>  *
>  * Allows the executable pages to be allocated for JIT and thunks if %TRUE.
>  * Type: %G_TYPE_BOOLEAN
>  * Default: %TRUE
>  */
> #define JSC_OPTION_USE_JIT "useJIT"
> 
> It would be something like that.

It looks to me that we could even generate the code of such JSCOptions
class making use of the JSC_OPTIONS() macro to autogenerate the parts of
the jsc_options_class_init() which install the properties, and have a
helper function to convert “someOptionName” internal camel-cased option
names to GObject style snake-case “some-option-name” strings. Note that
JSC_OPTIONS() even includes descriptions for the options, so those could
be used when instantiating the GParamSpecs for the properties and then
the option descriptions will automatically get picked by gtk-doc.

I think that even if the generated JSCOptions class does not have the
jsc_options_set_some_option() and jsc_options_get_some_option() methods,
the GObject properties functions will work well:

  JSCOptions *options = jsc_options_get ();
  g_object_set (options, "use-jit", FALSE, NULL);

If additional type safety there is desired, one can use g_object_setv()
and g_object_getv(), too.

Or is there any reason I am missing to not reuse the existing GObject
properties machinery?

Also: Maybe I would call this “JSCSettings”, to keep the name consist
with “WebKitSettings”.
Comment 2 Carlos Garcia Campos 2018-08-20 06:03:49 PDT
(In reply to Adrian Perez from comment #1)
> (In reply to Carlos Garcia Campos from comment #0)
> > I think it would be useful to expose the JSC options in the API. Right now
> > it's possible to change them using the env vars, but it would be better to
> > provide some API. To ensure API/ABI stability we could use a common
> > interface to set the options, that receive the option string identifier and
> > the value, similar to the GtkPrintSettings API, for example. Main problem is
> > that users don't know the options available, so we could provide names for
> > the most important options, like we do in WebKit2 API with editing commands.
> > We could also include API to get information about the options available
> > (name, value type, default value and description). Applications could use
> > that to create command line options to set them, for example.
> > 
> > JSCOptions* jsc_options_get (void); // Get the singleton instance
> > gboolean jsc_options_set_string (JSCOptions* options, const char* option,
> > const char* value);
> > char* jsc_options_get_string (JSCOptions* options, const char* option);
> > gboolean jsc_options_set_int (JSCOptions* options, const char* option, gint
> > value);
> > int jsc_options_get_int (JSCOptions* options, const char* option);
> > ......
> > typedef gboolean (* JSCOptionsFunc) (JSCOptions* options, const char*
> > option, GType type, GValue* default_value, const char* description, gpointer
> > user_data);
> > void jsc_options_foreach (JSCOptions* options, JSCOptionsFunc function,
> > gpointer user_data);
> 
> This being like this, it is like a mini-object system, so I wonder whether it
> would make more sense to make JSCSettings a class which inherits from GObject
> and make each option a property instead:
> 
>  * jsc_options_get_{string,int,bool,…} → g_object_get
>  * jsc_options_set_{string,int,bool,…} → g_object_set
>  * jsc_options_foreach → g_object_class_list_properties
> 
> ...and so on.

That would require to define all the options at compile time. Any option name change or type or removal would break the API.

> > /**
> >  * JSC_OPTION_USE_JIT:
> >  *
> >  * Allows the executable pages to be allocated for JIT and thunks if %TRUE.
> >  * Type: %G_TYPE_BOOLEAN
> >  * Default: %TRUE
> >  */
> > #define JSC_OPTION_USE_JIT "useJIT"
> > 
> > It would be something like that.
> 
> It looks to me that we could even generate the code of such JSCOptions
> class making use of the JSC_OPTIONS() macro to autogenerate the parts of
> the jsc_options_class_init() which install the properties, and have a
> helper function to convert “someOptionName” internal camel-cased option
> names to GObject style snake-case “some-option-name” strings. Note that
> JSC_OPTIONS() even includes descriptions for the options, so those could
> be used when instantiating the GParamSpecs for the properties and then
> the option descriptions will automatically get picked by gtk-doc.
> 
> I think that even if the generated JSCOptions class does not have the
> jsc_options_set_some_option() and jsc_options_get_some_option() methods,
> the GObject properties functions will work well:
> 
>   JSCOptions *options = jsc_options_get ();
>   g_object_set (options, "use-jit", FALSE, NULL);

Yes, without the public getter/setter, it might work.

> If additional type safety there is desired, one can use g_object_setv()
> and g_object_getv(), too.
> 
> Or is there any reason I am missing to not reuse the existing GObject
> properties machinery?

My initial idea was to use GObject properties indeed, similar to WebKitSettings. API stability was my main concern.

> Also: Maybe I would call this “JSCSettings”, to keep the name consist
> with “WebKitSettings”.

Also thought about it, but I'm not sure. JSC options are not like WebKit settings, so we don't really need to be consistent. In the case of JSC, options are normally set at the very beginning of the program execution and never changed again.
Comment 3 Adrian Perez 2018-08-20 07:21:19 PDT
(In reply to Carlos Garcia Campos from comment #2)
> (In reply to Adrian Perez from comment #1)
> > (In reply to Carlos Garcia Campos from comment #0)
> > > I think it would be useful to expose the JSC options in the API. Right now
> > > it's possible to change them using the env vars, but it would be better to
> > > provide some API. To ensure API/ABI stability we could use a common
> > > interface to set the options, that receive the option string identifier and
> > > the value, similar to the GtkPrintSettings API, for example. Main problem is
> > > that users don't know the options available, so we could provide names for
> > > the most important options, like we do in WebKit2 API with editing commands.
> > > We could also include API to get information about the options available
> > > (name, value type, default value and description). Applications could use
> > > that to create command line options to set them, for example.
> > > 
> > > JSCOptions* jsc_options_get (void); // Get the singleton instance
> > > gboolean jsc_options_set_string (JSCOptions* options, const char* option,
> > > const char* value);
> > > char* jsc_options_get_string (JSCOptions* options, const char* option);
> > > gboolean jsc_options_set_int (JSCOptions* options, const char* option, gint
> > > value);
> > > int jsc_options_get_int (JSCOptions* options, const char* option);
> > > ......
> > > typedef gboolean (* JSCOptionsFunc) (JSCOptions* options, const char*
> > > option, GType type, GValue* default_value, const char* description, gpointer
> > > user_data);
> > > void jsc_options_foreach (JSCOptions* options, JSCOptionsFunc function,
> > > gpointer user_data);
> > 
> > This being like this, it is like a mini-object system, so I wonder whether it
> > would make more sense to make JSCSettings a class which inherits from GObject
> > and make each option a property instead:
> > 
> >  * jsc_options_get_{string,int,bool,…} → g_object_get
> >  * jsc_options_set_{string,int,bool,…} → g_object_set
> >  * jsc_options_foreach → g_object_class_list_properties
> > 
> > ...and so on.
> 
> That would require to define all the options at compile time. Any option
> name change or type or removal would break the API.

If we provide a #define for each common option, those could also change
in between releases. If an option is renamed, we can change the #define,
but for options removed, added, or the option type changed, the API would
be broken with your suggested approach as well.

So no matter which of both approaches is implemented, the API can break
between releases. The only way of preventing that in both cases is avoiding
to merge JSC patches in stable release branches that include changes to the
JSC options — and allowing changes in between major releases.

> > > /**
> > >  * JSC_OPTION_USE_JIT:
> > >  *
> > >  * Allows the executable pages to be allocated for JIT and thunks if %TRUE.
> > >  * Type: %G_TYPE_BOOLEAN
> > >  * Default: %TRUE
> > >  */
> > > #define JSC_OPTION_USE_JIT "useJIT"
> > > 
> > > It would be something like that.
> > 
> > It looks to me that we could even generate the code of such JSCOptions
> > class making use of the JSC_OPTIONS() macro to autogenerate the parts of
> > the jsc_options_class_init() which install the properties, and have a
> > helper function to convert “someOptionName” internal camel-cased option
> > names to GObject style snake-case “some-option-name” strings. Note that
> > JSC_OPTIONS() even includes descriptions for the options, so those could
> > be used when instantiating the GParamSpecs for the properties and then
> > the option descriptions will automatically get picked by gtk-doc.
> > 
> > I think that even if the generated JSCOptions class does not have the
> > jsc_options_set_some_option() and jsc_options_get_some_option() methods,
> > the GObject properties functions will work well:
> > 
> >   JSCOptions *options = jsc_options_get ();
> >   g_object_set (options, "use-jit", FALSE, NULL);
> 
> Yes, without the public getter/setter, it might work.
> 
> > If additional type safety there is desired, one can use g_object_setv()
> > and g_object_getv(), too.
> > 
> > Or is there any reason I am missing to not reuse the existing GObject
> > properties machinery?
> 
> My initial idea was to use GObject properties indeed, similar to
> WebKitSettings. API stability was my main concern.

As I outlined above, the approach you suggested has exactly the same
potential API stability issues as using GObject properties. Therefore
I would go with the solution that feels more natural in a GLib/GObject
API and use object properties.

> > Also: Maybe I would call this “JSCSettings”, to keep the name consist
> > with “WebKitSettings”.
> 
> Also thought about it, but I'm not sure. JSC options are not like WebKit
> settings, so we don't really need to be consistent. In the case of JSC,
> options are normally set at the very beginning of the program execution and
> never changed again.

FWIW, I have nothing against the “JSCOptions” name. If you think using it
helps to make it clearer that they are intended to be set once at start
(in contrast to a “FooSettings” name), then let's use it :-)
Comment 4 Carlos Garcia Campos 2018-08-20 23:37:25 PDT
(In reply to Adrian Perez from comment #3)
> (In reply to Carlos Garcia Campos from comment #2)
> > (In reply to Adrian Perez from comment #1)
> > > (In reply to Carlos Garcia Campos from comment #0)
> > > > I think it would be useful to expose the JSC options in the API. Right now
> > > > it's possible to change them using the env vars, but it would be better to
> > > > provide some API. To ensure API/ABI stability we could use a common
> > > > interface to set the options, that receive the option string identifier and
> > > > the value, similar to the GtkPrintSettings API, for example. Main problem is
> > > > that users don't know the options available, so we could provide names for
> > > > the most important options, like we do in WebKit2 API with editing commands.
> > > > We could also include API to get information about the options available
> > > > (name, value type, default value and description). Applications could use
> > > > that to create command line options to set them, for example.
> > > > 
> > > > JSCOptions* jsc_options_get (void); // Get the singleton instance
> > > > gboolean jsc_options_set_string (JSCOptions* options, const char* option,
> > > > const char* value);
> > > > char* jsc_options_get_string (JSCOptions* options, const char* option);
> > > > gboolean jsc_options_set_int (JSCOptions* options, const char* option, gint
> > > > value);
> > > > int jsc_options_get_int (JSCOptions* options, const char* option);
> > > > ......
> > > > typedef gboolean (* JSCOptionsFunc) (JSCOptions* options, const char*
> > > > option, GType type, GValue* default_value, const char* description, gpointer
> > > > user_data);
> > > > void jsc_options_foreach (JSCOptions* options, JSCOptionsFunc function,
> > > > gpointer user_data);
> > > 
> > > This being like this, it is like a mini-object system, so I wonder whether it
> > > would make more sense to make JSCSettings a class which inherits from GObject
> > > and make each option a property instead:
> > > 
> > >  * jsc_options_get_{string,int,bool,…} → g_object_get
> > >  * jsc_options_set_{string,int,bool,…} → g_object_set
> > >  * jsc_options_foreach → g_object_class_list_properties
> > > 
> > > ...and so on.
> > 
> > That would require to define all the options at compile time. Any option
> > name change or type or removal would break the API.
> 
> If we provide a #define for each common option, those could also change
> in between releases. If an option is renamed, we can change the #define,
> but for options removed, added, or the option type changed, the API would
> be broken with your suggested approach as well.
> 
> So no matter which of both approaches is implemented, the API can break
> between releases. The only way of preventing that in both cases is avoiding
> to merge JSC patches in stable release branches that include changes to the
> JSC options — and allowing changes in between major releases.

But in the case of not autogenerated code, we would only provide macros for a few common options, that are more unlikely to change. We can deal with changes in any case, we have experience with that when the DOM bindings were autogenerated, and that's the main reason why I want to avoid generated code here :-) I don't know how we could deal with range options using properties, unless we consider them a string of the form begin:end and parse it. With explicit API we could simply add get/set_range (options, begin, end);

> > > > /**
> > > >  * JSC_OPTION_USE_JIT:
> > > >  *
> > > >  * Allows the executable pages to be allocated for JIT and thunks if %TRUE.
> > > >  * Type: %G_TYPE_BOOLEAN
> > > >  * Default: %TRUE
> > > >  */
> > > > #define JSC_OPTION_USE_JIT "useJIT"
> > > > 
> > > > It would be something like that.
> > > 
> > > It looks to me that we could even generate the code of such JSCOptions
> > > class making use of the JSC_OPTIONS() macro to autogenerate the parts of
> > > the jsc_options_class_init() which install the properties, and have a
> > > helper function to convert “someOptionName” internal camel-cased option
> > > names to GObject style snake-case “some-option-name” strings. Note that
> > > JSC_OPTIONS() even includes descriptions for the options, so those could
> > > be used when instantiating the GParamSpecs for the properties and then
> > > the option descriptions will automatically get picked by gtk-doc.
> > > 
> > > I think that even if the generated JSCOptions class does not have the
> > > jsc_options_set_some_option() and jsc_options_get_some_option() methods,
> > > the GObject properties functions will work well:
> > > 
> > >   JSCOptions *options = jsc_options_get ();
> > >   g_object_set (options, "use-jit", FALSE, NULL);
> > 
> > Yes, without the public getter/setter, it might work.
> > 
> > > If additional type safety there is desired, one can use g_object_setv()
> > > and g_object_getv(), too.
> > > 
> > > Or is there any reason I am missing to not reuse the existing GObject
> > > properties machinery?
> > 
> > My initial idea was to use GObject properties indeed, similar to
> > WebKitSettings. API stability was my main concern.
> 
> As I outlined above, the approach you suggested has exactly the same
> potential API stability issues as using GObject properties.

Not exactly the same, an option rename wouldn't be a problem without auto-generated properties. And again, the amount of options with a name in the API would be low.

> Therefore
> I would go with the solution that feels more natural in a GLib/GObject
> API and use object properties.

I'm not sure an object with no public API but a getter for the global instance is "natural" :-) I know GObject properties are also public API, but still.

> > > Also: Maybe I would call this “JSCSettings”, to keep the name consist
> > > with “WebKitSettings”.
> > 
> > Also thought about it, but I'm not sure. JSC options are not like WebKit
> > settings, so we don't really need to be consistent. In the case of JSC,
> > options are normally set at the very beginning of the program execution and
> > never changed again.
> 
> FWIW, I have nothing against the “JSCOptions” name. If you think using it
> helps to make it clearer that they are intended to be set once at start
> (in contrast to a “FooSettings” name), then let's use it :-)

I'm not sure either, but I'm sure we don't need to be consistent with the web view settings.
Comment 5 Adrian Perez 2018-08-24 07:31:15 PDT
(In reply to Carlos Garcia Campos from comment #4)
> (In reply to Adrian Perez from comment #3)
> > (In reply to Carlos Garcia Campos from comment #2)
> > > (In reply to Adrian Perez from comment #1)
> > > > (In reply to Carlos Garcia Campos from comment #0)
> >
> > [...]
> > 
> > So no matter which of both approaches is implemented, the API can break
> > between releases. The only way of preventing that in both cases is avoiding
> > to merge JSC patches in stable release branches that include changes to the
> > JSC options — and allowing changes in between major releases.
> 
> But in the case of not autogenerated code, we would only provide macros for
> a few common options, that are more unlikely to change. We can deal with
> changes in any case, we have experience with that when the DOM bindings were
> autogenerated, and that's the main reason why I want to avoid generated code
> here :-) I don't know how we could deal with range options using properties,
> unless we consider them a string of the form begin:end and parse it. With
> explicit API we could simply add get/set_range (options, begin, end);

Fair enough! Let's not autogenerate the code then.

> > > > > /**
> > > > >  * JSC_OPTION_USE_JIT:
> > > > >  *
> > > > >  * Allows the executable pages to be allocated for JIT and thunks if %TRUE.
> > > > >  * Type: %G_TYPE_BOOLEAN
> > > > >  * Default: %TRUE
> > > > >  */
> > > > > #define JSC_OPTION_USE_JIT "useJIT"
> > > > > 
> > > > > It would be something like that.
> > > > 
> > > > It looks to me that we could even generate the code of such JSCOptions
> > > > class making use of the JSC_OPTIONS() macro to autogenerate the parts of
> > > > the jsc_options_class_init() which install the properties, and have a
> > > > helper function to convert “someOptionName” internal camel-cased option
> > > > names to GObject style snake-case “some-option-name” strings. Note that
> > > > JSC_OPTIONS() even includes descriptions for the options, so those could
> > > > be used when instantiating the GParamSpecs for the properties and then
> > > > the option descriptions will automatically get picked by gtk-doc.
> > > > 
> > > > I think that even if the generated JSCOptions class does not have the
> > > > jsc_options_set_some_option() and jsc_options_get_some_option() methods,
> > > > the GObject properties functions will work well:
> > > > 
> > > >   JSCOptions *options = jsc_options_get ();
> > > >   g_object_set (options, "use-jit", FALSE, NULL);
> > > 
> > > Yes, without the public getter/setter, it might work.
> > > 
> > > > If additional type safety there is desired, one can use g_object_setv()
> > > > and g_object_getv(), too.
> > > > 
> > > > Or is there any reason I am missing to not reuse the existing GObject
> > > > properties machinery?
> > > 
> > > My initial idea was to use GObject properties indeed, similar to
> > > WebKitSettings. API stability was my main concern.
> > 
> > As I outlined above, the approach you suggested has exactly the same
> > potential API stability issues as using GObject properties.
> 
> Not exactly the same, an option rename wouldn't be a problem without
> auto-generated properties. And again, the amount of options with a name in
> the API would be low.

But the API would still allow setting the options for which we do not
add a “#define JSC_OPTION_FOO”, right? I would rather not allow that
behaviour.

> > Therefore
> > I would go with the solution that feels more natural in a GLib/GObject
> > API and use object properties.
> 
> I'm not sure an object with no public API but a getter for the global
> instance is "natural" :-) I know GObject properties are also public API, but
> still.

Now that you have convinced me about *not* autogenerating code,
I guess it's not much use exposing the properties either ;-)
Comment 6 Michael Catanzaro 2018-08-24 11:46:58 PDT
I agree with Carlos: if the available options can change and go away in the future, then the API should be string-based so that client applications understand that setting an option is best-effort only and not guaranteed to work in the future.
Comment 7 Carlos Garcia Campos 2018-09-06 00:21:23 PDT
Created attachment 349005 [details]
WIP

This patch is almost finished. It includes all the API required, but it doesn't expose the well known options yet. Yusuke, what jsc options do you think we should expose and document?
Comment 8 Adrian Perez 2018-09-06 04:37:12 PDT
Comment on attachment 349005 [details]
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 ;-)
Comment 9 Carlos Garcia Campos 2018-09-06 06:44:02 PDT
Created attachment 349021 [details]
WIP

Thanks for the review Adrian, I think I've addressed all the comments.
Comment 10 Adrian Perez 2018-09-07 02:49:41 PDT
Comment on attachment 349021 [details]
WIP

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

There's just one tiny typo, otherwise LGTM. Regarding the list of
options for which it may be interesting to provide documented symbols,
after taking a look at “Options.h”, I would start with the following
(all of them I *think* are unlikely to disappear any time soon):

  - dumpOptions: Useful to see what's enabled/disabled, because
    options can have implications, and (un)setting some of them
    can trigger switching others.

  - configFile: Well, handy to set many options at once *and* the
    location of the JSC log file, which AFAIK cannot be set in any
    other way.

  - useJIT: Because applications which don't do much JS may want
    to have a one-stop option to disable the whole JIT machinery
    and save some resources (e.g. embedding a Web view in an e-mail
    client, where there shouldn't be ever many scripts.)

  - useWebAssembly: I guess some applications may want to disable
    the WebAssembly machinery. Not super sure about this one, though.

It would be interesting to have some feedback from people who work
more often in JSC on the list above, maybe Caitlin and/or Yusuke have
some suggestions.

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:37
> + * Only a few of them are documented, you can use the undocummented options at

Typo, accidental double “m”: undocummented → undocumented

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

I see you used <emphasis> in the end, which is fine. Good
catch with the XML escaping using &entities; :D
Comment 11 Carlos Garcia Campos 2019-01-15 02:33:51 PST
Created attachment 359149 [details]
Patch
Comment 12 Michael Catanzaro 2019-01-23 13:34:21 PST
Comment on attachment 359149 [details]
Patch

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

Sorry it took me much longer than usual to get to this.

The options are global, not specific to a particular VM?

For the options JSC_OPTIONS_USE_JIT, JSC_OPTIONS_USE_DFG, JSC_OPTIONS_USE_FTL, and JSC_OPTIONS_USE_LLINT, it would be nice to have a little bit more documentation. The only one library users are likely to understand is the plain JIT, and even then we should probabyl explain it a little bit. DGF, FTL, and LLINT are probably just alphabet soup even to experienced developers, unless you're familiar with WebKit internals.

Lastly... use-case. You used to require that new APIs be used by a real application. Do you have plans to use it somewhere? Surely not adding API just for the sake of it?

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:33
> + * JavaScript options allow to change the behavior of the JavaScript engine.

allow changing

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:37
> + * Only a few of them are documented, you can use the undocumented options at

documented;

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

your own risk. (You can find the list of options in the WebKit source code.)

> Source/JavaScriptCore/API/glib/JSCOptions.cpp:52
> +typedef int32_t int32;
> +typedef size_t size;

typedefs are fine, but a more modern/natural way would be:

using int32 = int32_t;
using size = size_t;

I guess these are for use by the FOR_EACH options macros?

> Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3376
> +        "--jsc-configFile=/tmp/bar",

Does the config file actually get created by the tests? I assume not, since you're not actually running any JS. But if so, it should be cleaned.
Comment 13 Carlos Garcia Campos 2019-01-24 03:07:30 PST
(In reply to Michael Catanzaro from comment #12)
> Comment on attachment 359149 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359149&action=review
> 
> Sorry it took me much longer than usual to get to this.

No problem.

> The options are global, not specific to a particular VM?

Global.

> For the options JSC_OPTIONS_USE_JIT, JSC_OPTIONS_USE_DFG,
> JSC_OPTIONS_USE_FTL, and JSC_OPTIONS_USE_LLINT, it would be nice to have a
> little bit more documentation. The only one library users are likely to
> understand is the plain JIT, and even then we should probabyl explain it a
> little bit. DGF, FTL, and LLINT are probably just alphabet soup even to
> experienced developers, unless you're familiar with WebKit internals.

I expect users of those options to know what they are.

> Lastly... use-case. You used to require that new APIs be used by a real
> application. Do you have plans to use it somewhere? Surely not adding API
> just for the sake of it?
> 
> > Source/JavaScriptCore/API/glib/JSCOptions.cpp:33
> > + * JavaScript options allow to change the behavior of the JavaScript engine.
> 
> allow changing
> 
> > Source/JavaScriptCore/API/glib/JSCOptions.cpp:37
> > + * Only a few of them are documented, you can use the undocumented options at
> 
> documented;
> 
> > Source/JavaScriptCore/API/glib/JSCOptions.cpp:38
> > + * your own risk (you can find the list of options in the WebKit source code).
> 
> your own risk. (You can find the list of options in the WebKit source code.)
> 
> > Source/JavaScriptCore/API/glib/JSCOptions.cpp:52
> > +typedef int32_t int32;
> > +typedef size_t size;
> 
> typedefs are fine, but a more modern/natural way would be:
> 
> using int32 = int32_t;
> using size = size_t;
> 
> I guess these are for use by the FOR_EACH options macros?

Yes.

> > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3376
> > +        "--jsc-configFile=/tmp/bar",
> 
> Does the config file actually get created by the tests? I assume not, since
> you're not actually running any JS. But if so, it should be cleaned.

It's not created.
Comment 14 Carlos Garcia Campos 2019-01-24 03:10:01 PST
Committed r240431: <https://trac.webkit.org/changeset/240431>
Comment 15 Radar WebKit Bug Importer 2019-01-24 03:11:29 PST
<rdar://problem/47510693>