WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 188742
[GLIB] Expose JavaScriptCore options in GLib public API
https://bugs.webkit.org/show_bug.cgi?id=188742
Summary
[GLIB] Expose JavaScriptCore options in GLib public API
Carlos Garcia Campos
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Adrian Perez
Comment 1
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”.
Carlos Garcia Campos
Comment 2
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.
Adrian Perez
Comment 3
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 :-)
Carlos Garcia Campos
Comment 4
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.
Adrian Perez
Comment 5
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 ;-)
Michael Catanzaro
Comment 6
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.
Carlos Garcia Campos
Comment 7
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?
Adrian Perez
Comment 8
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 ;-)
Carlos Garcia Campos
Comment 9
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.
Adrian Perez
Comment 10
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>[!]<low>[:<high>]</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
Carlos Garcia Campos
Comment 11
2019-01-15 02:33:51 PST
Created
attachment 359149
[details]
Patch
Michael Catanzaro
Comment 12
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.
Carlos Garcia Campos
Comment 13
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.
Carlos Garcia Campos
Comment 14
2019-01-24 03:10:01 PST
Committed
r240431
: <
https://trac.webkit.org/changeset/240431
>
Radar WebKit Bug Importer
Comment 15
2019-01-24 03:11:29 PST
<
rdar://problem/47510693
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug