[WK2] Implement WebChromeClient::addMessageToConsole()

[Carlos Garcia Campos]

It's currently unimplemented. It will be useful to get information about exceptions and any other messages sent to the javascript console.

[Alexey Proskuryakov]

This seems to work on Mac. Are only some platform specific bits unimplemented?

[Carlos Garcia Campos]

What do you mean by "work"? I guess mac uses injected bundle API then. Web process doesn't notify the ui process about messages sent to the javascript console, so there's no callback for this in the UIClient. 
See http://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp#L263

So, what I'm requesting is a DidAddMessageToConsole message sent to the UI process instead of the notImplemented(); And a callback in the UIClient to be able to handle messages from the UI process. I'll write the patch if someone confirms this is something that we want.

[Carlos Garcia Campos]

Created attachment 129454 [details]
Patch

I haven't updated the project.pbxproj because I have no idea how to generate the fileRef of new files.

[Carlos Garcia Campos]

Could anybody tell me how to add new files to xcode project files, so that I can fix this patch to work on mac too?

[Adam Barth]

(In reply to comment #4)
> Could anybody tell me how to add new files to xcode project files, so that I can fix this patch to work on mac too?

Unfortunately, you need to use Xcode itself.  If you don't have a Mac, there are some Python scripts you can find that might be able to edit the file.  I think there's one called xcode-bodge that used to be used by the Chromium project.

[Carlos Garcia Campos]

Created attachment 133259 [details]
Updated patch to add new files to xcode too

Rebased to current git master and added new files to xcode too

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > Could anybody tell me how to add new files to xcode project files, so that I can fix this patch to work on mac too?
> 
> Unfortunately, you need to use Xcode itself.  If you don't have a Mac, there are some Python scripts you can find that might be able to edit the file.  I think there's one called xcode-bodge that used to be used by the Chromium project.

Thanks!, I used xcodebodge in the end, I hope it works.

[Build Bot]

Comment on attachment 133259 [details]
Updated patch to add new files to xcode too

Attachment 133259 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12114043

[Build Bot]

Comment on attachment 133259 [details]
Updated patch to add new files to xcode too

Attachment 133259 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12117042

[Carlos Garcia Campos]

It seems it didn't work :-(, any idea what's wrong?

[Carlos Garcia Campos]

Created attachment 134553 [details]
Another attempt to fix mac build

This time I added the files to WebKit2 and WebProcess targets, I don't know if it's enough or even correct.

[Build Bot]

Comment on attachment 134553 [details]
Another attempt to fix mac build

Attachment 134553 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12193281

[Build Bot]

Comment on attachment 134553 [details]
Another attempt to fix mac build

Attachment 134553 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12192259

[Gustavo Noronha (kov)]

Looking at the xcode project file and comparing to WKBase, it looks like the header is missing from the 'in Headers' lists:

BCDDB317124EBD130048D13C /* WKBase.h in Headers */,

BCDDB317124EBD130048D13C /* WKBase.h in Headers */ = {isa = PBXBuildFile; fileRef = BCDDB316124EBD130048D13C /* WKBase.h */; settings = {ATTRIBUTES = (Public, ); }; };

Seems like this is what's missing for the new header.

[Sam Weinig]

Is this actually used by people?  I would rather not add additional client APIs if no one wants this.

[Carlos Garcia Campos]

(In reply to comment #15)
> Is this actually used by people?  I would rather not add additional client APIs if no one wants this.

Yes, at least people who use the GTK+ port, I don't know other ports users though.

[Sam Weinig]

Comment on attachment 134553 [details]
Another attempt to fix mac build

If you are going to add new API, please add API tests to TestWebKitAPI as well.

[Carlos Garcia Campos]

(In reply to comment #17)
> (From update of attachment 134553 [details])
> If you are going to add new API, please add API tests to TestWebKitAPI as well.

Fair enough. Patch attached to bug #79918 includes unit tests for the GTK+ port API that wraps this one, fwiw. In any case, I'll add a test to TestWebKitAPI too, but we don't use/build it, so it won't be trivial thing.

[Carlos Garcia Campos]

Created attachment 138366 [details]
Patch including unit tests

I still haven't figured out how to add new files to xcodeproj files, and I don't have a mac, so it will still fail to build for mac and win.

[Build Bot]

Comment on attachment 138366 [details]
Patch including unit tests

Attachment 138366 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12494022

[Build Bot]

Comment on attachment 138366 [details]
Patch including unit tests

Attachment 138366 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12511241

[Philippe Normand]

The Qt port might be interested by that feature as well. I heard Joone recently rebased this patch, any news about it?

[Joone Hur]

Created attachment 173804 [details]
Rebased patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:267:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 1 in 22 files


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

[Build Bot]

Comment on attachment 173804 [details]
Rebased patch

Attachment 173804 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14818425

[Carlos Garcia Campos]

Comment on attachment 173804 [details]
Rebased patch

The patch adds a new callback to the UI client, so we need to add a new version an update the API traits. I'll submit a new patch.

[Build Bot]

Comment on attachment 173804 [details]
Rebased patch

Attachment 173804 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14821521

[Carlos Garcia Campos]

Created attachment 174119 [details]
Updated patch

Added new interface version to UI client and update API traits. Also added constructors to ConsoleMessage to simplify the code. I haven't included the xcode changes because they don't work anyway.

[Build Bot]

Comment on attachment 174119 [details]
Updated patch

Attachment 174119 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14844023

[Joone Hur]

There is a build break in the Mac port as follows:

Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2/WebPageProxyMessages.h:85:5: error: class 'ConsoleMessage' was previously declared as a struct [-Werror,-Wmismatched-tags]
    class ConsoleMessage;
    ^
/Volumes/Data/EWS/WebKit/Source/WebKit2/Shared/ConsoleMessage.h:39:8: note: previous use is here
struct ConsoleMessage {
       ^
/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2/WebPageProxyMessages.h:85:5: note: did you mean struct here?
    class ConsoleMessage;
    ^~~~~
    struct
1 error generated.

Why is ConsoleMessage added to WebPageProxyMessages.h as forward declaration of a class?

[Carlos Garcia Campos]

We need to add WebKit::ConsoleMessage to the list of structs in struct_or_class function (Source/WebKit2/Scripts/webkit2/messages.py)

[Carlos Garcia Campos]

Created attachment 175402 [details]
Updated patch

Added WebKit::ConsoleMessage to the list of structs

[Build Bot]

Comment on attachment 175402 [details]
Updated patch

Attachment 175402 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14911982

[Sam Weinig]

We purposely left this out, the WebInspector has superseded the need for this callback.

[Joone Hur]

(In reply to comment #34)
> We purposely left this out, the WebInspector has superseded the need for this callback.

However, there are webview clients that don't use the WebInspector. In this case, this callback is very useful.

[Sam Weinig]

(In reply to comment #35)
> (In reply to comment #34)
> > We purposely left this out, the WebInspector has superseded the need for this callback.
> 
> However, there are webview clients that don't use the WebInspector. In this case, this callback is very useful.

For those rare cases, the bundle provides a WKBundlePageWillAddMessageToConsole callback.

[Eric Seidel (no email)]

Comment on attachment 175402 [details]
Updated patch

Cleared review? from attachment 175402 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).