Bug 47239

Summary: Generate the messages sent to the WebPageProxy
Product: WebKit Reporter: Sam Weinig <sam>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, andersca, darin, kbalazs, kenneth, ossy, simon.fraser, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 47297    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Updated Patch
none
Updated.
none
Updated sam: review+

Description Sam Weinig 2010-10-05 18:18:56 PDT
Generate the messages sent to the WebPageProxy
Comment 1 Sam Weinig 2010-10-05 18:28:19 PDT
Created attachment 69877 [details]
Patch
Comment 2 Simon Fraser (smfr) 2010-10-05 21:33:01 PDT
I don't know what "Generate the messages sent to the WebPageProxy" means.
Comment 3 Sam Weinig 2010-10-05 22:15:14 PDT
(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.
Comment 4 Adam Roben (:aroben) 2010-10-06 04:46:09 PDT
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.
Comment 5 Sam Weinig 2010-10-06 10:31:55 PDT
Landed in r69210.
Comment 6 Kenneth Rohde Christiansen 2010-10-06 13:11:52 PDT
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
Comment 7 Kenneth Rohde Christiansen 2010-10-06 13:14:23 PDT
During qmake I get the following:

WARNING: Failure to find: generated/WebPageProxyMessageReceiver.cpp
WARNING: Failure to find: generated/WebPageProxyMessages.h
Comment 8 Kenneth Rohde Christiansen 2010-10-06 13:22:00 PDT
(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?
Comment 9 Kenneth Rohde Christiansen 2010-10-06 13:31:09 PDT
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
Comment 10 Andras Becsi 2010-10-06 13:34:59 PDT
(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.
Comment 11 Csaba Osztrogonác 2010-10-06 13:37:23 PDT
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 ...
Comment 12 Csaba Osztrogonác 2010-10-06 13:42:43 PDT
Comment on attachment 69877 [details]
Patch

Rolled out by http://trac.webkit.org/changeset/69229 , because it broke Windows and Qt
Comment 13 Sam Weinig 2010-10-06 15:47:04 PDT
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.
Comment 14 Sam Weinig 2010-10-06 15:48:49 PDT
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.
Comment 15 Balazs Kelemen 2010-10-06 16:06:54 PDT
(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+.
Comment 16 Csaba Osztrogonác 2010-10-06 16:11:09 PDT
(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)
Comment 17 Kenneth Rohde Christiansen 2010-10-07 07:53:25 PDT
(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.
Comment 18 Sam Weinig 2010-10-07 10:35:21 PDT
Created attachment 70114 [details]
Updated Patch
Comment 19 Sam Weinig 2010-10-07 10:36:26 PDT
(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.
Comment 20 Kenneth Rohde Christiansen 2010-10-07 10:37:27 PDT
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.
Comment 21 WebKit Review Bot 2010-10-07 10:38:19 PDT
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.
Comment 22 Sam Weinig 2010-10-07 10:51:37 PDT
Created attachment 70117 [details]
Updated.
Comment 23 WebKit Review Bot 2010-10-07 10:54:18 PDT
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.
Comment 24 Early Warning System Bot 2010-10-07 10:54:50 PDT
Attachment 70114 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4176136
Comment 25 Sam Weinig 2010-10-07 10:55:01 PDT
(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.
Comment 26 Csaba Osztrogonác 2010-10-07 11:02:03 PDT
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
Comment 27 Sam Weinig 2010-10-07 11:03:02 PDT
Created attachment 70118 [details]
Updated
Comment 28 Kenneth Rohde Christiansen 2010-10-07 11:09:21 PDT
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
Comment 29 Csaba Osztrogonác 2010-10-07 11:12:12 PDT
(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.
Comment 30 Anders Carlsson 2010-10-07 11:20:29 PDT
I'll land this now and make sure that any Qt build errors are resolved quickly.
Comment 31 Andras Becsi 2010-10-07 11:32:19 PDT
(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.
Comment 32 Csaba Osztrogonác 2010-10-07 11:42:13 PDT
(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.
Comment 33 Anders Carlsson 2010-10-07 12:33:01 PDT
Committed r69329: <http://trac.webkit.org/changeset/69329>