Bug 47239 – Generate the messages sent to the WebPageProxy

• Component: WebKit2
• Status: RESOLVED FIXED

[Sam Weinig]

Generate the messages sent to the WebPageProxy

[Sam Weinig]

Created attachment 69877 [details]
Patch

[Simon Fraser (smfr)]

I don't know what "Generate the messages sent to the WebPageProxy" means.

[Sam Weinig]

(In reply to comment #2)
> I don't know what "Generate the messages sent to the WebPageProxy" means.

Fair enough. We currently use an adhoc mechanism for passing messages from one process to the other in WebKit2, where we use generic In() and Out() classes to encode/decode data over the wire.  We are moving a system where we generate specialized typed structs from .messages.in files to make some of the work a little more type safe.

This patch converts messages being sent from the WebProcess to the WebPageProxy to use this new generated technique.

[Adam Roben (:aroben)]

Comment on attachment 69877 [details]
Patch

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

> WebKit2/ChangeLog:48
> +        * UIProcess/WebPageProxy.cpp:
> +        (WebKit::WebPageProxy::didReceiveMessage):
> +        (WebKit::WebPageProxy::didReceiveSyncMessage):
> +        (WebKit::WebPageProxy::didStartProvisionalLoadForFrame):
> +        (WebKit::WebPageProxy::didReceiveServerRedirectForProvisionalLoadForFrame):
> +        (WebKit::WebPageProxy::didFailProvisionalLoadForFrame):
> +        (WebKit::WebPageProxy::didCommitLoadForFrame):
> +        (WebKit::WebPageProxy::didFinishDocumentLoadForFrame):
> +        (WebKit::WebPageProxy::didFinishLoadForFrame):
> +        (WebKit::WebPageProxy::didFailLoadForFrame):
> +        (WebKit::WebPageProxy::didReceiveTitleForFrame):
> +        (WebKit::WebPageProxy::didFirstLayoutForFrame):
> +        (WebKit::WebPageProxy::didFirstVisuallyNonEmptyLayoutForFrame):
> +        (WebKit::WebPageProxy::didRemoveFrameFromHierarchy):
> +        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
> +        (WebKit::WebPageProxy::decidePolicyForNewWindowAction):

All this boilerplate isn't really helpful. Summarizing the kinds of changes made ("Updated to pass a message struct", "Added new files to the project", etc.) would be nice. You could remove the individual function names if they all have the same description.

> WebKit2/Scripts/webkit2/messages.py:160
>          'uint16_t',
>          'uint32_t',
>          'uint64_t',
> +        'int8_t',
> +        'int16_t',
> +        'int32_t',
> +        'int64_t',
>      ])

Maybe we should sort this list.

> WebKit2/Scripts/webkit2/messages.py:333
>      special_cases = {
>          'WTF::String': '"ArgumentCoders.h"',
> +        'WebKit::InjectedBundleUserMessageEncoder': '"InjectedBundleUserMessageCoders.h"',
>      }

This one, too.

> WebKit2/Scripts/webkit2/messages.py:403
> +    for message in receiver.messages:
> +        if message.reply_parameters is not None:
> +            for reply_parameter in message.reply_parameters:
> +                type = reply_parameter.type
> +                argument_encoder_headers = argument_coder_headers_for_type(type)
> +                if argument_encoder_headers:
> +                    headers.update(argument_encoder_headers)
> +                    continue
> +
> +                type_headers = headers_for_type(type)
> +                headers.update(type_headers)
> +

Looks like you could have used your iterreplyparameters function here. My guess is that you didn't because you needed the "is not None" check. But iterreplyparameters can do that! (reply_parameter for message in self.messages if message.reply_parameters is not None for reply_parameter in message.reply_parameters)

> WebKit2/Shared/StringPairVector.h:38
> +// This class is a hack to work around the fact that the IPC message generator
> +// cannot deal with class templates with more than one paramter.
> +class StringPairVector {
> +public:

:-(

Obviously we should make the generator smarter!

> WebKit2/UIProcess/WebPageProxy.cpp:624
> -void WebPageProxy::didFailProvisionalLoadForFrame(WebFrameProxy* frame, APIObject* userData)
> +void WebPageProxy::didFailProvisionalLoadForFrame(uint64_t frameID, CoreIPC::ArgumentDecoder* arguments)
>  {
> -    m_loaderClient.didFailProvisionalLoadWithErrorForFrame(this, frame, userData);
> +    RefPtr<APIObject> userData;
> +    WebContextUserMessageDecoder messageDecoder(userData, pageNamespace()->context());
> +    if (!arguments->decode(messageDecoder))
> +        return;

It would be nice to be able to get rid of all this repeated WebContextUserMessageDecoder code.

> WebKit2/UIProcess/WebPageProxy.cpp:744
> +void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, uint32_t opaqueNavigationType, uint32_t opaqueModifiers, int32_t opaqueMouseButton, const String& url, uint64_t listenerID)
>  {
> +    WebFrameProxy* frame = process()->webFrame(frameID);
> +    NavigationType navigationType = static_cast<NavigationType>(opaqueNavigationType);
> +    WebEvent::Modifiers modifiers = static_cast<WebEvent::Modifiers>(opaqueModifiers);
> +    WebMouseEvent::Button mouseButton = static_cast<WebMouseEvent::Button>(opaqueMouseButton);

Maybe someday we can teach the generator how to convert the types for us.

[Sam Weinig]

Landed in r69210.

[Kenneth Rohde Christiansen]

This seems to somehow have broken Qt:

../../../webkit2/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:33:34: error: WebPageProxyMessages.h: No such file or directory
../../../webkit2/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp: In member function ‘virtual void WebKit::WebEditorClient::registerCommandForUndo(WTF::PassRefPtr<WebCore::EditCommand>)’:
../../../webkit2/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:214: error: ‘Messages’ has not been declared
../../../webkit2/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp: In member function ‘virtual void WebKit::WebEditorClient::clearUndoRedoOperations()’:
../../../webkit2/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:223: error: ‘Messages’ has not been declared
...

Maybe WebPageProxyMessages.h is just missing in the build system

[Kenneth Rohde Christiansen]

During qmake I get the following:

WARNING: Failure to find: generated/WebPageProxyMessageReceiver.cpp
WARNING: Failure to find: generated/WebPageProxyMessages.h

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> During qmake I get the following:
> 
> WARNING: Failure to find: generated/WebPageProxyMessageReceiver.cpp
> WARNING: Failure to find: generated/WebPageProxyMessages.h

From DerivedSources.pro:

UIProcess/WebPageProxy/WebProcess.messages.in

^ this seems wrong

Shouldn't this be WebKit2/UIProcess/WebPageProxy.messages.in?

[Kenneth Rohde Christiansen]

Comment on attachment 69877 [details]
Patch

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

> WebKit2/DerivedSources.pro:55
> +processmessageheader_generator.commands = python $${SRC_ROOT_DIR}/WebKit2/Scripts/generate-messages-header.py  $${SRC_ROOT_DIR}/WebKit2/UIProcess/WebPageProxy.messages.in > $$OUTPUT_DIR/WebKit2/generated/WebPageProxyMessages.h

processmessageheader_generator is already used elsewhere, so you need to choose another name or it will get overridden.

> WebKit2/DerivedSources.pro:56
> +processmessageheader_generator.depends  = $${SRC_ROOT_DIR}/WebKit2/Scripts/generate-messages-header.py $${SRC_ROOT_DIR}/UIProcess/WebPageProxy/WebProcess.messages.in

$${SRC_ROOT_DIR}/UIProcess/WebPageProxy/WebProcess.messages.in should be $${SRC_ROOT_DIR}/WebKit2/UIProcess/WebPageProxy.messages.in

[Andras Becsi]

(In reply to comment #9)
> (From update of attachment 69877 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69877&action=review
> 
> > WebKit2/DerivedSources.pro:55
> > +processmessageheader_generator.commands = python $${SRC_ROOT_DIR}/WebKit2/Scripts/generate-messages-header.py  $${SRC_ROOT_DIR}/WebKit2/UIProcess/WebPageProxy.messages.in > $$OUTPUT_DIR/WebKit2/generated/WebPageProxyMessages.h
> 
> processmessageheader_generator is already used elsewhere, so you need to choose another name or it will get overridden.
> 
> > WebKit2/DerivedSources.pro:56
> > +processmessageheader_generator.depends  = $${SRC_ROOT_DIR}/WebKit2/Scripts/generate-messages-header.py $${SRC_ROOT_DIR}/UIProcess/WebPageProxy/WebProcess.messages.in
> 
> $${SRC_ROOT_DIR}/UIProcess/WebPageProxy/WebProcess.messages.in should be $${SRC_ROOT_DIR}/WebKit2/UIProcess/WebPageProxy.messages.in

And also:

./WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:31:#include "WebPageProxyMessageKinds.h"

should be changed to include the new WebPageProxyMessages.h to fix the Qt build.

[Csaba Osztrogonác]

Guys, the culprit patch landed 3 hours ago, and the Qt bot is still red.

I _strongly_ ask you to use EWS not to break the build!

If you break the build accidentally, you should fix it as soon 
as possible or roll out the patch. All WebKit developers have to 
respect this policy, you shouldn't ignore the redness of the Qt bot.

I don't know why do you break the Qt build regularly and leave the bot red ...

[Csaba Osztrogonác]

Comment on attachment 69877 [details]
Patch

Rolled out by http://trac.webkit.org/changeset/69229 , because it broke Windows and Qt

[Sam Weinig]

I was actually hoping the EWS bots would run, but, after many hours of being up for review, they still had not.  Please note, WebKit2 is still experimental and in state where big changes are going to happen from time to time, so please expect build breakages.

[Sam Weinig]

It would also be helpful it the Qt build moved to using DerivedSources.make instead of rolling its own. This would cause less headaches in the future.

[Balazs Kelemen]

(In reply to comment #14)
> It would also be helpful it the Qt build moved to using DerivedSources.make instead of rolling its own. This would cause less headaches in the future.

We can't use that because we are using a totally different toolchain where qmake is the meta-generator. Please note that EWS does not run for patches that got r+.

[Csaba Osztrogonác]

(In reply to comment #13)
> I was actually hoping the EWS bots would run, but, after many hours of being up for review, they still had not.  
It isn't true. Qt EWS ran at 2010-10-05 18:32 PST.
(You submitted the patch at 2010-10-05 18:28 PST)
But unfortunately the patch didn't applied to ToT,
it caused the purple EWS bubbles ...

>Please note, WebKit2 is still experimental and in state where big changes are going to happen from time to time, so please expect build breakages.
I think it is a lame excuse. Build breakages in the trunk for core
builders are absolutely unacceptable. The main goal of the EWS's is 
not to break the build and not to block others work. 

The correct way should be followed by all developers:
1.) Upload patch with r?
2.) Wait for green EWS bubbles (It takes only 5-10 minutes on Qt.)
3.) If all bubbles are green and the patch is reviewed, commit it.
4.) If one of the bubbles are red, try to fix the build, and goto 1)

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> I was actually hoping the EWS bots would run, but, after many hours of being up for review, they still had not.  Please note, WebKit2 is still experimental and in state where big changes are going to happen from time to time, so please expect build breakages.

I agree that WebKit2 is still experimental and big refactoring will happen. I don't want to hinter this in any way, and am OK with a breakage from time to time, if everyone are interested in working together to fix the issues as soon as possible - just like when Adam Roben pinged me on IRC some time ago about an upcoming patch.

As we expect the build breakage, maybe we should not build webkit2 by default on the core builders and have separate builders for that until WebKit2 has become a more stable code base.

[Sam Weinig]

Created attachment 70114 [details]
Updated Patch

[Sam Weinig]

(In reply to comment #16)
> (In reply to comment #13)
> > I was actually hoping the EWS bots would run, but, after many hours of being up for review, they still had not.  
> It isn't true. Qt EWS ran at 2010-10-05 18:32 PST.
> (You submitted the patch at 2010-10-05 18:28 PST)
> But unfortunately the patch didn't applied to ToT,
> it caused the purple EWS bubbles ...

Interesting, I didn't realize that is what purple meant.  I will keep that in mind next time and upload a more up-to-date patch.

[Kenneth Rohde Christiansen]

Comment on attachment 70114 [details]
Updated Patch

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

> WebKit2/DerivedSources.pro:56
> +pageproxymessageheader_generator.depends  = $${SRC_ROOT_DIR}/WebKit2/Scripts/generate-messages-header.py $${SRC_ROOT_DIR}/UIProcess/WebPageProxy/WebPageProxy.messages.in

IS this right? Isn't it just $${SRC_ROOT_DIR}/UIProcess/WebPageProxy.messages.in, thus no WebPageProxy sub dir.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Scripts/webkit2/messages.py:196:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKit2/Scripts/webkit2/messages.py:325:  multiple statements on one line (semicolon)  [pep8/E702] [5]
WebKit2/UIProcess/WebPageProxy.cpp:881:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 26 files


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

[Sam Weinig]

Created attachment 70117 [details]
Updated.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Scripts/webkit2/messages.py:196:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKit2/Scripts/webkit2/messages.py:325:  multiple statements on one line (semicolon)  [pep8/E702] [5]
WebKit2/UIProcess/WebPageProxy.cpp:881:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 27 files


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

[Early Warning System Bot]

Attachment 70114 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4176136

[Sam Weinig]

(In reply to comment #21)
> Attachment 70114 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> WebKit2/Scripts/webkit2/messages.py:196:  expected 2 blank lines, found 1  [pep8/E302] [5]
> WebKit2/Scripts/webkit2/messages.py:325:  multiple statements on one line (semicolon)  [pep8/E702] [5]
> WebKit2/UIProcess/WebPageProxy.cpp:881:  More than one command on the same line  [whitespace/newline] [4]
> Total errors found: 3 in 26 files
> 
> 

Fixed all these locally.

[Csaba Osztrogonác]

Comment on attachment 70117 [details]
Updated.

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

> WebKit2/DerivedSources.pro:56
> +pageproxymessageheader_generator.depends  = $${SRC_ROOT_DIR}/WebKit2/Scripts/generate-messages-header.py $${SRC_ROOT_DIR}/UIProcess/WebPageProxy/WebPageProxy.messages.in

You forgot to add WebKit2 to the path:

pageproxymessageheader_generator.depends = $${SRC_ROOT_DIR}/WebKit2/Scripts/generate-messages-header.py $${SRC_ROOT_DIR}/WebKit2/UIProcess/WebPageProxy/WebPageProxy.messages.in

[Sam Weinig]

Created attachment 70118 [details]
Updated

[Kenneth Rohde Christiansen]

You can click on the purple bobble to find out what when wrong. It seems to not be able to apply the patch due to the xcode file:

Hunk #11 FAILED at 1947.
Hunk #12 FAILED at 2246.
2 out of 12 hunks FAILED -- saving rejects to file WebKit2/WebKit2.xcodeproj/project.pbxproj.rej

[Csaba Osztrogonác]

(In reply to comment #28)
> You can click on the purple bobble to find out what when wrong. It seems to not be able to apply the patch due to the xcode file:
> 
> Hunk #11 FAILED at 1947.
> Hunk #12 FAILED at 2246.
> 2 out of 12 hunks FAILED -- saving rejects to file WebKit2/WebKit2.xcodeproj/project.pbxproj.rej

It seems http://trac.webkit.org/changeset/69323 broke the appliable of the patch.

[Anders Carlsson]

I'll land this now and make sure that any Qt build errors are resolved quickly.

[Andras Becsi]

(In reply to comment #30)
> I'll land this now and make sure that any Qt build errors are resolved quickly.

Thank you very much for doing that.

[Csaba Osztrogonác]

(In reply to comment #30)
> I'll land this now and make sure that any Qt build errors are resolved quickly.

I tried it locally, and I got the following build error:

In file included from ../../../WebKit2/UIProcess/API/qt/ClientImpl.cpp:27:
../../../WebKit2/UIProcess/API/qt/qwkpage_p.h:56: error: ‘WebPageProxy’ has not been declared

You can fix it easily with adding a "using namespace WebKit;" to qwkpage_p.h,
because WebPageProxy class is defined in WebKit namespace. It works for me locally.

[Anders Carlsson]

Committed r69329: <http://trac.webkit.org/changeset/69329>