[webkit-reviews] review granted: [Bug 47239] Generate the messages sent to the WebPageProxy : [Attachment 69877] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Sam Weinig
<sam at webkit.org>'s request for review:
Bug 47239: Generate the messages sent to the WebPageProxy
https://bugs.webkit.org/show_bug.cgi?id=47239

Attachment 69877: Patch
https://bugs.webkit.org/attachment.cgi?id=69877&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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.