Bug 188695 - [GLIB] Complete the JSCException API
Summary: [GLIB] Complete the JSCException 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: 188698
  Show dependency treegraph
 
Reported: 2018-08-17 03:34 PDT by Carlos Garcia Campos
Modified: 2018-08-20 16:53 PDT (History)
10 users (show)

See Also:


Attachments
Patch (25.98 KB, patch)
2018-08-17 03:38 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (26.02 KB, patch)
2018-08-17 05:32 PDT, 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-17 03:34:54 PDT
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.
Comment 1 Carlos Garcia Campos 2018-08-17 03:38:13 PDT
Created attachment 347356 [details]
Patch
Comment 2 Adrian Perez 2018-08-17 04:48:08 PDT
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.
Comment 3 Adrian Perez 2018-08-17 04:52:59 PDT
(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 […]”
Comment 4 Carlos Garcia Campos 2018-08-17 05:02:42 PDT
Ok, I'll update the patch to use backtrace instead. Thanks Adrian!
Comment 5 Carlos Garcia Campos 2018-08-17 05:32:31 PDT
Created attachment 347358 [details]
Patch
Comment 6 Adrian Perez 2018-08-17 06:32:39 PDT
(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! :-)
Comment 7 Michael Catanzaro 2018-08-17 07:26:53 PDT
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."
Comment 8 Carlos Garcia Campos 2018-08-19 23:57:23 PDT
Committed r235023: <https://trac.webkit.org/changeset/235023>
Comment 9 Radar WebKit Bug Importer 2018-08-20 16:53:13 PDT
<rdar://problem/43534489>