Bug 131050 – Web Inspector: Provide a way for JSContext console to log to system console

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

We already have a way for a Page's console to log to the system console. We should provide a way for this to work with JSContext's console. An application user default seems reasonable. We could also make API/SPI to toggle at runtime.

[Radar WebKit Bug Importer]

<rdar://problem/16488820>

[Joseph Pecoraro]

Created attachment 228304 [details]
[PATCH] Proposed Fix

cq- because I haven't tested with iOS yet and I'm not sure if printf will work. It works on OS X though, so getting the general idea reviewed.

[WebKit Commit Bot]

Attachment 228304 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/JSConsoleClient.cpp:73:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Comment on attachment 228304 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/JSConsoleClient.cpp:62
> +    Boolean preference = CFPreferencesGetAppBooleanValue(CFSTR("JavaScriptCoreConsoleLogToSystemConsole"), kCFPreferencesCurrentApplication, &keyExistsAndHasValidFormat);

JavaScriptCoreConsoleLogToSystemConsole implies only console.log is printed. A verb like "output" would help too. Maybe: OutputJavaScriptCoreConsoleToSystemConsole. Does this work for WebKit too? Maybe OutputJavaScriptConsoleToSystemConsole if so.

> Source/JavaScriptCore/inspector/JSConsoleClient.cpp:82
> +    if (JSConsoleClient::logToSystemConsole())

No need for JSConsoleClient:: right?

[Joseph Pecoraro]

(In reply to comment #4)
> (From update of attachment 228304 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228304&action=review
> 
> > Source/JavaScriptCore/inspector/JSConsoleClient.cpp:62
> > +    Boolean preference = CFPreferencesGetAppBooleanValue(CFSTR("JavaScriptCoreConsoleLogToSystemConsole"), kCFPreferencesCurrentApplication, &keyExistsAndHasValidFormat);
> 
> JavaScriptCoreConsoleLogToSystemConsole implies only console.log is printed. A verb like "output" would help too. Maybe: OutputJavaScriptCoreConsoleToSystemConsole. Does this work for WebKit too? Maybe OutputJavaScriptConsoleToSystemConsole if so.

I want the preference to start with "JavaScriptCore" as a namespacing thing. There is one other CFPreference in JavaScriptCore and it does the same.

Any preference among these?

    - "JavaScriptCoreOutputConsoleMessagesToSystemConsole"
    - "JavaScriptCoreLogConsoleMessagesToSystemConsole"
    - "JavaScriptCoreConsoleLogToSystemConsoleEnabled"


> > Source/JavaScriptCore/inspector/JSConsoleClient.cpp:82
> > +    if (JSConsoleClient::logToSystemConsole())
> 
> No need for JSConsoleClient:: right?

Correct, but I prefer the explicit prefix because it makes it obvious that this is a static/class function and not a member function. If we have a style one way or the other I can match.

[Timothy Hatcher]

Comment on attachment 228304 [details]
[PATCH] Proposed Fix

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

>>> Source/JavaScriptCore/inspector/JSConsoleClient.cpp:62
>>> +    Boolean preference = CFPreferencesGetAppBooleanValue(CFSTR("JavaScriptCoreConsoleLogToSystemConsole"), kCFPreferencesCurrentApplication, &keyExistsAndHasValidFormat);
>> 
>> JavaScriptCoreConsoleLogToSystemConsole implies only console.log is printed. A verb like "output" would help too. Maybe: OutputJavaScriptCoreConsoleToSystemConsole. Does this work for WebKit too? Maybe OutputJavaScriptConsoleToSystemConsole if so.
> 
> I want the preference to start with "JavaScriptCore" as a namespacing thing. There is one other CFPreference in JavaScriptCore and it does the same.
> 
> Any preference among these?
> 
>     - "JavaScriptCoreOutputConsoleMessagesToSystemConsole"
>     - "JavaScriptCoreLogConsoleMessagesToSystemConsole"
>     - "JavaScriptCoreConsoleLogToSystemConsoleEnabled"

Prefixing with JSC makes sense. I like JavaScriptCoreOutputConsoleMessagesToSystemConsole.

>>> Source/JavaScriptCore/inspector/JSConsoleClient.cpp:82
>>> +    if (JSConsoleClient::logToSystemConsole())
>> 
>> No need for JSConsoleClient:: right?
> 
> Correct, but I prefer the explicit prefix because it makes it obvious that this is a static/class function and not a member function. If we have a style one way or the other I can match.

It looks like you are calling a base class method with this syntax. I know I have seen other static methods called without a prefix in WebCore. Not sure there is a formal style guide.

[Joseph Pecoraro]

Created attachment 228546 [details]
[PATCH] Revised

This refactors all the printfs instead StringBuilder + WTFLogAlways. It also includes the updated default name. I tested on iOS this works (the printf would have too, but I think this is nicer).

[WebKit Commit Bot]

Attachment 228546 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/JSConsoleClient.cpp:73:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Comment on attachment 228546 [details]
[PATCH] Revised

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

> Source/JavaScriptCore/inspector/JSConsoleClient.cpp:75
> +    static std::once_flag initializeLogging;
> +    std::call_once(initializeLogging, []{
> +        JSConsoleClient::initializeLogToSystemConsole();
> +    });

This makes the default behave differently than the WebKit2 equivalent setting. It can only be changed between executions, not while the app is running. That seems fine, but it might be limiting if an app needed to change it at runtime (like with a command line flag or checkbox.) The solution would require always calling CFPreferencesGetAppBooleanValue, or exposing a JSC API to change it at runtime. We can cross that bridge when needed.

[WebKit Commit Bot]

Comment on attachment 228546 [details]
[PATCH] Revised

Clearing flags on attachment: 228546

Committed r166799: <http://trac.webkit.org/changeset/166799>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.