Bug 92227 – [WK2][WTR] LayoutTestController.sendWebIntentResponse() needs to be implemented

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

The following web intents tests rely on LayoutTestController.sendWebIntentResponse():
  webintents/web-intents-failure.html
  webintents/web-intents-reply.html

These 2 tests are now failing on WebKit2 WTR because LayoutTestController.sendWebIntentResponse() is undefined.

[Chris Dumez]

Created attachment 154287 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 154287 [details]
Patch

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

> Source/WebKit2/Shared/IntentData.cpp:40
> +IntentData::IntentData(Intent* coreIntent)

Why not just WebIntentData::WebIntentData(Intent* intent) ? WebKit2 normally prefixes everything in WebKit namespace with Web

[Kenneth Rohde Christiansen]

Comment on attachment 154287 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntentRequest.cpp:47
> +WKIntentDataRef WKBundleIntentRequestCopyIntent(WKBundleIntentRequestRef requestRef)

CopyIntentData?

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntentRequest.cpp:53
> +{
> +#if ENABLE(WEB_INTENTS)
> +    RefPtr<WebIntentData> webIntentData = toImpl(requestRef)->intent();
> +    return toAPI(webIntentData.release().leakRef());
> +#else
> +    return 0;

How is this being copied? comment?

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleIntentRequest.cpp:50
> +{
> +    m_intentRequest->postResult(static_cast<SerializedScriptValue*>(data->internalRepresentation()));
> +}

What does internalRepresentation return?

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 154287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154287&action=review
> 
> > Source/WebKit2/Shared/IntentData.cpp:40
> > +IntentData::IntentData(Intent* coreIntent)
> 
> Why not just WebIntentData::WebIntentData(Intent* intent) ? WebKit2 normally prefixes everything in WebKit namespace with Web

I already have a WebIntentData class. IntentData is the storage class used by WebIntentData. I have seen this being done for some other WK2 types. We could of course decide to merge the 2 but this is not exactly related to this change.

[Chris Dumez]

Comment on attachment 154287 [details]
Patch

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

>> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntentRequest.cpp:47
>> +WKIntentDataRef WKBundleIntentRequestCopyIntent(WKBundleIntentRequestRef requestRef)
> 
> CopyIntentData?

I was trying to keep the name short but sure.

>> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntentRequest.cpp:53
>> +    return 0;
> 
> How is this being copied? comment?

It is copied because I returned a new instance and I call .release().leakRef() on it. Using "Copy" in the name indicates to the client that it needs to adopt it. If I called it "Get" then the client would not adopt it. Also, I think "Create" is not suitable here. WKPageCopySessionState() or WKNavigationDataCopyOriginalRequest() is acting very similarly.

>> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleIntentRequest.cpp:50
>> +}
> 
> What does internalRepresentation return?

It returns a generic void* but the member it returns is of type WebCore::SerializedScriptValue* (always).

[Gyuyoung Kim]

Comment on attachment 154287 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleIntentRequest.h:57
> +    InjectedBundleIntentRequest(WebCore::IntentRequest*);

Missing *explicit* keyword.

[Chris Dumez]

Comment on attachment 154287 [details]
Patch

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

>>> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleIntentRequest.cpp:50
>>> +}
>> 
>> What does internalRepresentation return?
> 
> It returns a generic void* but the member it returns is of type WebCore::SerializedScriptValue* (always).

See for example:

WKSerializedScriptValueRef WKSerializedScriptValueCreateWithInternalRepresentation(void* internalRepresentation)
{
    RefPtr<WebSerializedScriptValue> serializedValue = WebSerializedScriptValue::create(static_cast<WebCore::SerializedScriptValue*>(internalRepresentation));
    return toAPI(serializedValue.release().leakRef());
}

[Chris Dumez]

Created attachment 154325 [details]
Patch

1. Rename WKBundleIntentRequestCopyIntent() to WKBundleIntentRequestCopyIntentData() as suggested by Kenneth
2. Add explicit keyword for InjectedBundleIntentRequest constructor as spotted by Gyuyoung
3. Rebase on master (the ewk_view showing trick is not longer needed as it was fixed in a separate patch that landed)

[WebKit Review Bot]

Comment on attachment 154325 [details]
Patch

Clearing flags on attachment: 154325

Committed r123652: <http://trac.webkit.org/changeset/123652>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.