Bug 188695 – [GLIB] Complete the JSCException API

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Carlos Garcia Campos]

Add more API to JSCException:

 * New function to get the column number
 * New function get exception as string (toString())
 * Add the possibility to create exceptions with a custom error name.
 * New function to get the exception error name
 * Convenience function to report a exception by returning a formatted string with all the exception details, to be shown as a user error message.

[Carlos Garcia Campos]

Created attachment 347356 [details]
Patch

[Adrian Perez]

Comment on attachment 347356 [details]
Patch

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

Looks good to me overall, but please check the comment about using
“backtrace” instead of “back trace” before landing.

> Source/JavaScriptCore/API/glib/JSCException.cpp:262
> + * jsc_exception_get_back_trace_string:

Somehow this function name feels a bit clunky when reading it, because
usually “backtrace” is how the term is written. For the sake of double
checking, I did the following:

    % rg -i backtrace /usr/include/|wc -l
    408
    % rg -i back_trace /usr/include/ | wc -l
    0

I am *very* sure that we want this to be “jsc_exception_get_backtrace_string”.

> Source/JavaScriptCore/API/glib/JSCException.cpp:265
> + * Get a string with the exception back trace.

Let's also use “backtrace” in documentation comments, please.

[Adrian Perez]

(In reply to Adrian Perez from comment #2)
 
> > Source/JavaScriptCore/API/glib/JSCException.cpp:262
> > + * jsc_exception_get_back_trace_string:
> 
> Somehow this function name feels a bit clunky when reading it, because
> usually “backtrace” is how the term is written. For the sake of double
> checking, I did the following:
> 
>     % rg -i backtrace /usr/include/|wc -l
>     408
>     % rg -i back_trace /usr/include/ | wc -l
>     0

Also, “backtrace” is written as a single word in many online resources,
and used as such in other programming languages and library functions:

  - https://secure.php.net/manual/en/function.debug-backtrace.php
  - https://linux.die.net/man/3/backtrace
  - https://sourceware.org/gdb/onlinedocs/gdb/Backtrace.html
  - https://wiki.ubuntu.com/Backtrace
  - https://en.wikipedia.org/wiki/Stack_trace

Example from the Wikipedia page:

  “In computing, a stack trace (also called stack backtrace[1] or
   stack traceback[2]) is […]”

[Carlos Garcia Campos]

Ok, I'll update the patch to use backtrace instead. Thanks Adrian!

[Carlos Garcia Campos]

Created attachment 347358 [details]
Patch

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #4)
> Ok, I'll update the patch to use backtrace instead. Thanks Adrian!

The patch looks perfect now, thanks! r+ all the things! :-)

[Michael Catanzaro]

Comment on attachment 347358 [details]
Patch

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

> Source/JavaScriptCore/API/glib/JSCException.cpp:269
> +const char* jsc_exception_get_backtrace_string(JSCException* exception)

Hm, shouldn't it use const gchar*, for consistency with our other public APIs? I know gchar/gint/etc. are dumb types, but consistency is good. Maybe you should audit the entire JSC API for this. We definitely use the g types in the WebKit2 API, for example.

> Source/JavaScriptCore/API/glib/JSCException.cpp:286
> + * Get the string representation of @exception error.
> + *
> + * Returns: (transfer full): the string representation of @exception error.

It reads better without the word "error" at the end.

"Get the string representation of @exception.

Returns: (transfer full): the string representation of @exception."

[Carlos Garcia Campos]

Committed r235023: <https://trac.webkit.org/changeset/235023>

[Radar WebKit Bug Importer]

<rdar://problem/43534489>