Bug 92227

Summary: [WK2][WTR] LayoutTestController.sendWebIntentResponse() needs to be implemented
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cmarcelo, gyuyoung.kim, kenneth, menard, rakuco, sam, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 88303    
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2012-07-25 01:17:01 PDT
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.
Comment 1 Chris Dumez 2012-07-25 01:31:58 PDT
Created attachment 154287 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-07-25 02:40:19 PDT
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
Comment 3 Kenneth Rohde Christiansen 2012-07-25 03:11:55 PDT
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?
Comment 4 Chris Dumez 2012-07-25 03:21:54 PDT
(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.
Comment 5 Chris Dumez 2012-07-25 03:30:29 PDT
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).
Comment 6 Gyuyoung Kim 2012-07-25 04:17:23 PDT
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.
Comment 7 Chris Dumez 2012-07-25 04:30:59 PDT
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());
}
Comment 8 Chris Dumez 2012-07-25 04:34:37 PDT
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)
Comment 9 WebKit Review Bot 2012-07-25 13:34:50 PDT
Comment on attachment 154325 [details]
Patch

Clearing flags on attachment: 154325

Committed r123652: <http://trac.webkit.org/changeset/123652>
Comment 10 WebKit Review Bot 2012-07-25 13:34:56 PDT
All reviewed patches have been landed.  Closing bug.