Bug 88399 – [WK2] Add implementation for registerIntentService in WebFrameLoaderClient

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

In order to bring support for Web Intents in WebKit2, we need to implement registerIntentService() in WebFrameLoaderClient.

[Chris Dumez]

Created attachment 145961 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 145961 [details]
Patch

Attachment 145961 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12908131

New failing tests:
canvas/philip/tests/2d.gradient.radial.cone.top.html
canvas/philip/tests/2d.gradient.radial.cone.shape2.html

[WebKit Review Bot]

Created attachment 146162 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Chris Dumez]

Created attachment 146217 [details]
Patch

Remove bundle API and bump the WKPageLoaderClient version.

[Kenneth Rohde Christiansen]

Comment on attachment 146217 [details]
Patch

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

> Source/WebKit2/Shared/APIClientTraits.h:57
>  template<> struct APIClientTraits<WKPageLoaderClient> {
> -    static const size_t interfaceSizesByVersion[2];
> +    static const size_t interfaceSizesByVersion[3];
>  };

I don't know about this versioning, maybe someone else can have a look

> Source/WebKit2/Shared/IntentServiceInfo.h:14
> + *   * Neither the name of Intel Corporation nor the names of its

Is the Intel name the only thing that differs here from what is commonly used in WebKit?

> Source/WebKit2/Shared/IntentServiceInfo.h:53
> +    WebCore::KURL href;

Any reason for using href instead of url ?

> Source/WebKit2/UIProcess/WebIntentServiceInfo.cpp:45
> +namespace WebKit {
> +
> +WebIntentServiceInfo::WebIntentServiceInfo(const IntentServiceInfo& store)
> +    : m_store(store)
> +{
> +}
> +
> +WebIntentServiceInfo::~WebIntentServiceInfo()
> +{
> +}

Any reason why not just doing this in the header file?

> Source/WebKit2/UIProcess/WebLoaderClient.cpp:167
> +#if ENABLE(WEB_INTENTS_TAG)

Given the current discussion around WebIntents and WebActivities, I am not sure this will remain a tag, so why not just use WEB_INTENTS ?

> Source/WebKit2/UIProcess/WebLoaderClient.cpp:168
> +void WebLoaderClient::registerIntentServiceForFrame(WebPageProxy* page, WebFrameProxy* frame, const IntentServiceInfo& serviceInfo)

IntentsService? or just registerIntentForFrame ?

> Source/WebKit2/UIProcess/WebLoaderClient.h:52
>  class WebPageProxy;
>  class WebProtectionSpace;
>  
> +#if ENABLE(WEB_INTENTS_TAG)
> +struct IntentServiceInfo;
> +#endif

WebIntentsServiceInfo for consistency?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1563
> +    notImplemented();

This seems wrong

[Chris Dumez]

Comment on attachment 146217 [details]
Patch

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

>> Source/WebKit2/Shared/APIClientTraits.h:57
>>  };
> 
> I don't know about this versioning, maybe someone else can have a look

The version bump was requested by andersca.

>> Source/WebKit2/Shared/IntentServiceInfo.h:14
>> + *   * Neither the name of Intel Corporation nor the names of its
> 
> Is the Intel name the only thing that differs here from what is commonly used in WebKit?

This is the standard BSD header we are asked to use at Intel. Is this a problem?
I guess I could use the same one as in the other files if this is a problem.

>> Source/WebKit2/Shared/IntentServiceInfo.h:53
>> +    WebCore::KURL href;
> 
> Any reason for using href instead of url ?

Just trying to match the spec for consistency:
http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html#registration-markup

>> Source/WebKit2/UIProcess/WebIntentServiceInfo.cpp:45
>> +}
> 
> Any reason why not just doing this in the header file?

I could. I followed what was done for WebNavigationData class in same folder. WebNavigationData.cpp was written and contains pretty much the same thing.

>> Source/WebKit2/UIProcess/WebLoaderClient.cpp:167
>> +#if ENABLE(WEB_INTENTS_TAG)
> 
> Given the current discussion around WebIntents and WebActivities, I am not sure this will remain a tag, so why not just use WEB_INTENTS ?

I don't really understand why we would use WEB_INTENTS here. WEB_INTENTS can be enabled while WEB_INTENTS_TAG is disabled at the moment. I'm matching what is in WebCore. If the 2 merge in the future, then we can replace it then.

>> Source/WebKit2/UIProcess/WebLoaderClient.cpp:168
>> +void WebLoaderClient::registerIntentServiceForFrame(WebPageProxy* page, WebFrameProxy* frame, const IntentServiceInfo& serviceInfo)
> 
> IntentsService? or just registerIntentForFrame ?

This is really for registering an intent service, not an intent. "registerIntentService" is what is used in FrameLoaderClient.

>> Source/WebKit2/UIProcess/WebLoaderClient.h:52
>> +#endif
> 
> WebIntentsServiceInfo for consistency?

You mean that you want registerIntentServiceForFrame() in this file to take a WebIntentServiceInfo in argument instead of an IntentServiceInfo? If so, Ok, I can fix this.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1563
>> +    notImplemented();
> 
> This seems wrong

Yes it does :) I'll remove it.

[Chris Dumez]

Created attachment 147271 [details]
Patch

Take Kenneth's feedback into consideration:
- Change license headers
- Remove the extra notImplemented() in WebFrameLoaderClient.cpp
- Move empty destructor from implementation to header (WebIntentServiceInfo)
- Use WebIntentServiceInfo instead of IntentServiceInfo in WebLoaderClient

I kept the rest as is for now (please see my previous comments).

[Early Warning System Bot]

Comment on attachment 147271 [details]
Patch

Attachment 147271 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12939975

[Chris Dumez]

Created attachment 147274 [details]
Patch

Tiny Changelog fix.

[Early Warning System Bot]

Comment on attachment 147274 [details]
Patch

Attachment 147274 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12954016

[Chris Dumez]

Created attachment 147323 [details]
Patch

Rebase on master and fix the wk2-qt build.

[Chris Dumez]

Created attachment 147330 [details]
Patch

[Chris Dumez]

Created attachment 147482 [details]
Patch for landing

Rebase on master.

[WebKit Review Bot]

Comment on attachment 147482 [details]
Patch for landing

Clearing flags on attachment: 147482

Committed r120301: <http://trac.webkit.org/changeset/120301>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.