Bug 115578

Summary: [GTK] [WebKit2] enable displaying console.log messages to system console
Product: WebKit Reporter: arno. <a.renevier>
Component: WebKitGTKAssignee: arno. <a.renevier>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, gns, gyuyoung.kim, mrobinson, philn, rakuco, rego+ews, sam, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch proposal
none
updated patch against tot
none
updated patch
none
updated patch: fixes gtkdoc errors
none
updated patch: fixes gtkdoc errors
none
updated patch: same patch; documentation updated to adress martin's comment none

Description arno. 2013-05-03 16:11:13 PDT
Hi,
WebCore preferences have a logsPageMessagesToSystemConsoleEnabled method which allows to display console (page) message to system console (shell console).
Maybe we could expose that to WebKit2 Gtk API.
Comment 1 arno. 2013-05-03 16:30:59 PDT
Created attachment 200506 [details]
patch proposal
Comment 2 arno. 2013-05-04 09:17:47 PDT
Created attachment 200530 [details]
updated patch against tot
Comment 3 WebKit Commit Bot 2013-05-04 09:19:40 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 4 WebKit Commit Bot 2013-05-04 09:19:47 PDT
Attachment 200530 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp']" exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1113:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1115:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1116:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Total errors found: 5 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Martin Robinson 2013-05-04 11:15:34 PDT
Comment on attachment 200530 [details]
updated patch against tot

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

Looks good to me, with just a few nits about naming and style and such. This needs to be approved by another GTK+ reviewer as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1108
> +     * WebKitSettings:enable-messages-to-system-console:

I'm not sure if the "system console" is a Gnomey concept. Perhaps this would be better named something like enable-page-messages-sent-to-stdout, though that seems a bit clunky too.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1110
> +     * Enable or disable displaying of page messages to system console.

You should probably explain a bit that this applies to messages that normally go to the web inspector console.

>> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1113
>> +                                    PROP_ENABLE_MESSAGES_TO_SYSTEM_CONSOLE,
> 
> When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]

Please fix these style issues.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h:395
> +                                                                gboolean        enabled);

Looks like the indentation is off here.
Comment 6 arno. 2013-05-04 15:18:24 PDT
Created attachment 200547 [details]
updated patch

I checked and those messages are only written in case of a message console. So I think it would make sense to specify console in the name of the setting. In the new patch, the setting is called enable-write-console-messages-to-stdout; Other nits are fixed
Comment 7 Sam Weinig 2013-05-05 14:24:00 PDT
Looks fine to me.
Comment 8 arno. 2013-05-05 15:31:16 PDT
Created attachment 200596 [details]
updated patch: fixes gtkdoc errors
Comment 9 arno. 2013-05-06 10:30:44 PDT
Created attachment 200719 [details]
updated patch: fixes gtkdoc errors

oups, I uploaded the wrong patch
Comment 10 Martin Robinson 2013-05-14 11:01:42 PDT
Comment on attachment 200719 [details]
updated patch: fixes gtkdoc errors

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

Looks good to me. This needs another GTK+ reviewer to approve the new API, but I like the name now. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1110
> +     * Enable or disable writing console messages to stdout.

You probably want to expand this documentation slightly to say that this includes console.log and friends.
Comment 11 Martin Robinson 2013-05-14 11:02:29 PDT
Gustavo, Xan, or Carlos, could you take a look at this new WK2 API and r+ the patch if it appears reasonable to you as well?
Comment 12 Gustavo Noronha (kov) 2013-05-14 12:28:13 PDT
Comment on attachment 200719 [details]
updated patch: fixes gtkdoc errors

Sure!
Comment 13 arno. 2013-05-15 12:34:26 PDT
Created attachment 201863 [details]
updated patch: same patch; documentation updated to adress martin's comment
Comment 14 WebKit Commit Bot 2013-05-15 17:12:12 PDT
Comment on attachment 201863 [details]
updated patch: same patch; documentation updated to adress martin's comment

Clearing flags on attachment: 201863

Committed r150158: <http://trac.webkit.org/changeset/150158>
Comment 15 WebKit Commit Bot 2013-05-15 17:12:16 PDT
All reviewed patches have been landed.  Closing bug.