Bug 89072

Summary: [WK2] Add support for Web Intents MessagePorts
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cmarcelo, d-r, gbillock, gustavo, gyuyoung.kim, hausmann, kenneth, menard, philn, rakuco, sam, webkit.review.bot, xan.lopez, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.w3.org/TR/web-intents/
Bug Depends on: 93175, 93778    
Bug Blocks: 88303, 93353    
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Patch
none
Patch
buildbot: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
gyuyoung.kim: commit-queue-
Patch
kenneth: review+
Patch for landing none

Description Chris Dumez 2012-06-14 01:29:31 PDT
A Web Intent can carry MessagePorts. Those MessagePorts need to be passed carried over from the intent dispatch to its delivery.
Currently, the MessagePorts are ignored and they will be missing upon delivery to the Intent service.
Comment 1 Chris Dumez 2012-06-14 02:44:11 PDT
The MessagePorts technically don't need to be used on UI side. We - however - need to keep information about the MessagePorts in IntentData when dispatched to the UI so that the MessagePorts can be passed back to the intent service on WebProcess side upon delivery to a frame.

I'm thinking of adding a "HashMap<uint64_t, WebCore::MessagePortArray*> m_messagePorts;" to Webprocess.h in order to keep the MessagePorts on WebProcess side and assign them an identifier. Then, I would pass only the identifier to the UI side via IntentData. Finally, upon intent delivery (WebFrame::deliverIntent() in Bug 88989), I would get back the WebCore::MessagePortArray* from m_messagePorts using the identifier in IntentData.

Can anyone tell me if this is the right/best approach for this use case?
Comment 2 Chris Dumez 2012-07-31 09:04:16 PDT
Created attachment 155557 [details]
Patch
Comment 3 Chris Dumez 2012-07-31 09:06:45 PDT
With this patch, all the webintents/* layout tests are now passing for WK2 EFL.
Comment 4 Build Bot 2012-07-31 09:34:07 PDT
Comment on attachment 155557 [details]
Patch

Attachment 155557 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13410014
Comment 5 Chris Dumez 2012-07-31 09:50:56 PDT
Created attachment 155572 [details]
Patch

Attempt to fix Windows build.
Comment 6 Chris Dumez 2012-07-31 09:53:18 PDT
Created attachment 155573 [details]
Patch
Comment 7 Chris Dumez 2012-07-31 12:13:34 PDT
I don't know how headers forwarding works for Mac port. It does not seem to be working for Source/WebCore/dom/default/*.h

Can anyone tell me what I should do to get it working?
Comment 8 Build Bot 2012-07-31 12:15:54 PDT
Comment on attachment 155573 [details]
Patch

Attachment 155573 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13404144
Comment 9 Chris Dumez 2012-07-31 13:00:16 PDT
Created attachment 155611 [details]
Patch

Attempt to fix Mac build.
Comment 10 Chris Dumez 2012-07-31 13:11:29 PDT
Created attachment 155615 [details]
Patch
Comment 11 Kenneth Rohde Christiansen 2012-08-02 12:24:06 PDT
Comment on attachment 155615 [details]
Patch

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

You should probably refer to the draft spec, plus do you have any regression test for this? You mention that the tests in webintents will be changed by this change but you are not adding the new results as part of this bug?

> Source/WebKit2/Shared/IntentData.cpp:56
> +    MessagePortChannelArray* coreMessagePorts = coreIntent->messagePorts();
> +    if (coreMessagePorts) {
> +        size_t numMessagePorts = coreMessagePorts->size();
> +        for (size_t i = 0; i < numMessagePorts; ++i)
> +            messagePorts.append(WebProcess::shared().addMessagePortChannel((*coreMessagePorts)[i]->channel()));
> +    }

no iterators for iterating over this?

> Source/WebKit2/UIProcess/WebIntentData.cpp:40
> +WebIntentData::WebIntentData(const IntentData& store, WebProcessProxy* process)

You have to move this class around as part of this patch? It makes it harder to see what was changed

> Source/WebKit2/UIProcess/WebIntentData.cpp:82
> +        wkExtras.set(it->first, WebString::create(it->second));
> +    return ImmutableDictionary::adopt(wkExtras);

can we get a newline before the return
Comment 12 Chris Dumez 2012-08-02 13:22:53 PDT
Comment on attachment 155615 [details]
Patch

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

>> Source/WebKit2/Shared/IntentData.cpp:56
>> +    }
> 
> no iterators for iterating over this?

WebKit coding Style says to use indexes instead of iterators to improve code readability. This is why I did this.

>> Source/WebKit2/UIProcess/WebIntentData.cpp:40
>> +WebIntentData::WebIntentData(const IntentData& store, WebProcessProxy* process)
> 
> You have to move this class around as part of this patch? It makes it harder to see what was changed

I believe I need to move it because it depends on a UiProcess type now: WebProcessProxy.
Comment 13 Chris Dumez 2012-08-02 13:27:23 PDT
Also, note that there is no need to update test results. The tests are currently failing for WebKit2 EFL and they start passing with this patch. The tests are not skipped or anything and the global test expectations are fine. EFL is the only port to enable Web Intents in WK2. Basically, this patch brings WK2 support for Web intents to the same level as WK1.
Comment 14 Chris Dumez 2012-08-02 13:39:04 PDT
Created attachment 156152 [details]
Patch

- Add empty line before return
- Attempt to clarify Changelogs
Comment 15 Chris Dumez 2012-08-02 13:41:32 PDT
(In reply to comment #12)
> (From update of attachment 155615 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155615&action=review
> 
> >> Source/WebKit2/Shared/IntentData.cpp:56
> >> +    }
> > 
> > no iterators for iterating over this?
> 
> WebKit coding Style says to use indexes instead of iterators to improve code readability. This is why I did this.

For the coding style, I'm referring to:

Prefer index over iterator in Vector iterations for a terse, easier-to-read code.
Right:

size_t frameViewsCount = frameViews.size();
for (size_t i = i; i < frameViewsCount; ++i)
    frameViews[i]->updateLayoutAndStyleIfNeededRecursive();

Wrong:

const Vector<RefPtr<FrameView> >::iterator end = frameViews.end();
for (Vector<RefPtr<FrameView> >::iterator it = frameViews.begin(); it != end; ++it)
    (*it)->updateLayoutAndStyleIfNeededRecursive();


From http://www.webkit.org/coding/coding-style.html
Comment 16 Kenneth Rohde Christiansen 2012-08-03 08:07:25 PDT
What about moving it in a preparation patch?
Comment 17 Chris Dumez 2012-08-03 22:45:16 PDT
(In reply to comment #16)
> What about moving it in a preparation patch?

Sure, done in Bug 93175.
Comment 18 Chris Dumez 2012-08-04 06:58:16 PDT
Created attachment 156531 [details]
Patch

Rebase on master now that the dependency landed.
Comment 19 Kenneth Rohde Christiansen 2012-08-04 07:40:21 PDT
Comment on attachment 156531 [details]
Patch

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

> Source/WebKit2/Shared/IntentData.h:32
> +#include <WebCore/KURL.h>

Where are you using this one?

> Source/WebKit2/UIProcess/API/C/WKIntentData.cpp:34
> +#include "WebSerializedScriptValue.h"

You add this to a cpp file you are only removing code from

> Source/WebKit2/UIProcess/API/C/WKIntentData.h:-37
> -WK_EXPORT WKIntentDataRef WKIntentDataCreate(WKDictionaryRef initDictionary);

Shouldn't the changelog explain why this was removed and what replaces it. So you can only use WKBundleIntentCreate() now? Can you make a C test showing how to use  this new api, maybe even api documentation?

> Source/WebKit2/UIProcess/WebIntentData.cpp:50
> +WebIntentData::~WebIntentData()
> +{
> +    // Remove MessagePortChannels from WebProcess.
> +    if (m_process && !m_store.messagePorts.isEmpty()) {
> +        size_t numMessagePorts = m_store.messagePorts.size();

How is all of this clean up handled when the web process crashes? Would be great if it could be tested

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntent.cpp:65
> +    RefPtr<Intent> coreIntent = Intent::create(toImpl(action)->string(), toImpl(type)->string(), data ? static_cast<SerializedScriptValue*>(toImpl(data)->internalRepresentation()) : 0, dummyPorts, ec);

Wouldn't it be nicer to do the ? * : * on a separate line

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntent.h:43
> +WK_EXPORT WKStringRef WKBundleIntentCopyExtra(WKBundleIntentRef intentRef, WKStringRef key);

CopyExtraValue? to make it more different from CopyExtras

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1977
> +    WebFrame* frame = WebProcess::shared().webFrame(frameID);
> +    if (!frame)
> +        return;
> +
> +    frame->deliverIntent(coreIntent);

Why not just

if (WebFrame* frame = WebProcess::shared().webFrame(frameID))
    frame->deliverIntent(coreIntent);
Comment 20 Chris Dumez 2012-08-04 10:18:41 PDT
Comment on attachment 156531 [details]
Patch

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

>> Source/WebKit2/Shared/IntentData.h:32
>> +#include <WebCore/KURL.h>
> 
> Where are you using this one?

Because of the removal of "GenericCallback.h" (which should not have been there in the first place), I need to add a few includes in this file and in other Web Intents files.

> Source/WebKit2/Shared/IntentData.h:59
>      Vector<WebCore::KURL> suggestions;

See, KURL is used here.

>> Source/WebKit2/UIProcess/API/C/WKIntentData.cpp:34
>> +#include "WebSerializedScriptValue.h"
> 
> You add this to a cpp file you are only removing code from

well, it is really needed because WebSerializedScriptValue is used in this file and we no longer included "WebSerializedScriptValue.h" in WebIntentData.h. See WebIntentData.h cleanup in this present patch.

>> Source/WebKit2/UIProcess/API/C/WKIntentData.h:-37
>> -WK_EXPORT WKIntentDataRef WKIntentDataCreate(WKDictionaryRef initDictionary);
> 
> Shouldn't the changelog explain why this was removed and what replaces it. So you can only use WKBundleIntentCreate() now? Can you make a C test showing how to use  this new api, maybe even api documentation?

Yes, I could keep this here but this was originally added to support WTR use case. Now that WTR uses WKBundleIntent, we don't technically need it anymore. I could add a mention in the Changelog for this, yes. Regarding, the doc, I have not seen any C API with documentation. I am however planning to add C API tests for Web Intents in a separate patch. The API has not been very stable so far so I was waiting for feature-completion. Really, we are not supposed to create Web Intents on native side except for testing purposes AFAIK.

>> Source/WebKit2/UIProcess/WebIntentData.cpp:50
>> +        size_t numMessagePorts = m_store.messagePorts.size();
> 
> How is all of this clean up handled when the web process crashes? Would be great if it could be tested

Well, the MessagePorts are stored on WebProcess side, what could I even do if it crashes?

>> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntent.cpp:65
>> +    RefPtr<Intent> coreIntent = Intent::create(toImpl(action)->string(), toImpl(type)->string(), data ? static_cast<SerializedScriptValue*>(toImpl(data)->internalRepresentation()) : 0, dummyPorts, ec);
> 
> Wouldn't it be nicer to do the ? * : * on a separate line

Sure, no problem.

>> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntent.h:43
>> +WK_EXPORT WKStringRef WKBundleIntentCopyExtra(WKBundleIntentRef intentRef, WKStringRef key);
> 
> CopyExtraValue? to make it more different from CopyExtras

Hmm, I don't mind making this change but then I could need to edit the UIProcess equivalent as well. This is made to be consistent with WKIntentDataCopyExtra(). Do you think I should make both changes in this patch?

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1977
>> +    frame->deliverIntent(coreIntent);
> 
> Why not just
> 
> if (WebFrame* frame = WebProcess::shared().webFrame(frameID))
>     frame->deliverIntent(coreIntent);

Ok.
Comment 21 Kenneth Rohde Christiansen 2012-08-04 10:23:24 PDT
> Yes, I could keep this here but this was originally added to support WTR use case. Now that WTR uses WKBundleIntent, we don't technically need it anymore. I could add a mention in the Changelog for this, yes. Regarding, the doc, I have not seen any C API with documentation. I am however planning to add C API tests for Web Intents in a separate patch. The API has not been very stable so far so I was waiting for feature-completion. Really, we are not supposed to create Web Intents on native side except for testing purposes AFAIK.

OK, just mention it in the changelog then.
 
> >> Source/WebKit2/UIProcess/WebIntentData.cpp:50
> >> +        size_t numMessagePorts = m_store.messagePorts.size();
> > 
> > How is all of this clean up handled when the web process crashes? Would be great if it could be tested
> 
> Well, the MessagePorts are stored on WebProcess side, what could I even do if it crashes?

You are storing the messageports on the UI side as well right, they should be cleared if it crashes. Are you doing that?

> > CopyExtraValue? to make it more different from CopyExtras
> 
> Hmm, I don't mind making this change but then I could need to edit the UIProcess equivalent as well. This is made to be consistent with WKIntentDataCopyExtra(). Do you think I should make both changes in this patch?

I think that is fine, or in another patch, whatever you feel like.
Comment 22 Kenneth Rohde Christiansen 2012-08-04 10:23:54 PDT
I think Anders reviewed the original C api, it would be great if he could have a look again.
Comment 23 Chris Dumez 2012-08-04 10:30:15 PDT
(In reply to comment #21)
> > Yes, I could keep this here but this was originally added to support WTR use case. Now that WTR uses WKBundleIntent, we don't technically need it anymore. I could add a mention in the Changelog for this, yes. Regarding, the doc, I have not seen any C API with documentation. I am however planning to add C API tests for Web Intents in a separate patch. The API has not been very stable so far so I was waiting for feature-completion. Really, we are not supposed to create Web Intents on native side except for testing purposes AFAIK.
> 
> OK, just mention it in the changelog then.
> 
> > >> Source/WebKit2/UIProcess/WebIntentData.cpp:50
> > >> +        size_t numMessagePorts = m_store.messagePorts.size();
> > > 
> > > How is all of this clean up handled when the web process crashes? Would be great if it could be tested
> > 
> > Well, the MessagePorts are stored on WebProcess side, what could I even do if it crashes?
> 
> You are storing the messageports on the UI side as well right, they should be cleared if it crashes. Are you doing that?

I'm not storing MessagePorts on UI side. I'm merely storing their identifiers in WebIntentData. The MessagePorts are not meant to be used on UI side so I only use identifiers. MessagePorts are stored only in the WebProcess.
Comment 24 Kenneth Rohde Christiansen 2012-08-04 11:15:13 PDT
> I'm not storing MessagePorts on UI side. I'm merely storing their identifiers in WebIntentData. The MessagePorts are not meant to be used on UI side so I only use identifiers. MessagePorts are stored only in the WebProcess.

Ok, just checking :-)
Comment 25 Chris Dumez 2012-08-04 12:02:38 PDT
Created attachment 156537 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 26 Chris Dumez 2012-08-12 08:52:29 PDT
Created attachment 157902 [details]
Patch

Rebase on master and unskip related test cases for WebKit2 EFL.
Comment 27 Chris Dumez 2012-08-13 07:45:38 PDT
Created attachment 157998 [details]
Patch

Rebase on master.
Comment 28 Kenneth Rohde Christiansen 2012-08-14 05:27:31 PDT
Comment on attachment 157998 [details]
Patch

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

> Source/WebKit2/ChangeLog:12
> +        between the Web application and the intent service.
> +        This functionality is already supported by WebKit 1.

I would add a newline before that last line

> Source/WebKit2/ChangeLog:25
> +        before delivering to the DOMWindow.

delivering the intent to the ...

> Source/WebKit2/ChangeLog:27
> +        This functionality is already tested by:

newline after

> Source/WebKit2/UIProcess/WebIntentData.cpp:51
> +    if (m_process && !m_store.messagePorts.isEmpty()) {
> +        size_t numMessagePorts = m_store.messagePorts.size();
> +        for (size_t i = 0; i < numMessagePorts; ++i)

why look at empty if you are looking at the size() anyway?

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:198
> +    send(Messages::WebProcess::RemoveMessagePortChannel(channelID), 0);

Add comment for what the 0 represents like /* ... */ 0

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntent.h:37
> +WK_EXPORT WKBundleIntentRef WKBundleIntentCreate(WKDictionaryRef initDictionary);

im not sure the init adds much value

> Source/WebKit2/WebProcess/WebPage/WebPage.h:482
> -    void deliverIntentToFrame(uint64_t frameID, const IntentData&);
> +    void deliverCoreIntentToFrame(uint64_t frameID, WebCore::Intent*);

Why not keep the name and rename the one with IntentData? or get rid of the one with IntentData
Comment 29 Chris Dumez 2012-08-14 10:33:26 PDT
Comment on attachment 157998 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:482
>> +    void deliverCoreIntentToFrame(uint64_t frameID, WebCore::Intent*);
> 
> Why not keep the name and rename the one with IntentData? or get rid of the one with IntentData

I cannot get rid of the one with IntentData because it maps to an IPC message in WebPage.messages.in. This is also why I did not rename it (less intrusive change).
Comment 30 Chris Dumez 2012-08-14 11:42:46 PDT
Created attachment 158384 [details]
Patch

Take Kenneth's feedback into consideration. Note that I did not rename deliverCoreIntentToFrame() (see previous comment).
Comment 31 Gyuyoung Kim 2012-08-14 18:22:19 PDT
Comment on attachment 158384 [details]
Patch

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

In Bug 93942, LayoutTestController.cpp is being changed to TestRunner.cpp. I think you need to check the bug for this patch.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntent.cpp:40
> +#if ENABLE(WEB_INTENTS)

It seems to me this is a little excessive #if ~ #endif. usage. Can't you wrap from *#include XXX* to *using namespace* at a time ?

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleIntent.cpp:82
> +        wkExtras.set(it->first, WebString::create(it->second));

You have mentioned "return xxxx" is added after a newline. But, I think this is trivial stuff.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleIntent.cpp:93
> +    return ImmutableArray::adopt(wkSuggestions);

ditto.

> Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:267
> +void WebFrame::deliverIntent(Intent* intent)

Parameter definition is different from webFrame.h

WebFrame.h
void deliverIntent(WebCore::Intent*);
Comment 32 Chris Dumez 2012-08-14 21:56:04 PDT
Created attachment 158500 [details]
Patch

Take Gyuyoung nits into account.
Comment 33 Gyuyoung Kim 2012-08-14 22:09:05 PDT
Comment on attachment 158500 [details]
Patch

Attachment 158500 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13504460
Comment 34 Chris Dumez 2012-08-14 22:15:43 PDT
Created attachment 158502 [details]
Patch
Comment 35 Kenneth Rohde Christiansen 2012-08-15 03:21:51 PDT
Comment on attachment 158502 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleIntent.cpp:67
> +    RefPtr<Intent> coreIntent = Intent::create(toImpl(action)->string(),
> +                                               toImpl(type)->string(),
> +                                               data ? static_cast<SerializedScriptValue*>(toImpl(data)->internalRepresentation()) : 0,
> +                                               dummyPorts,
> +                                               ec);

We would normally just try to keep this on one line instead of adding weird identation. Using helper variables can make it look slightly nicer.
Comment 36 Chris Dumez 2012-08-15 05:07:48 PDT
Created attachment 158547 [details]
Patch for landing

Fix nit spotted by Kenneth.
Comment 37 WebKit Review Bot 2012-08-15 06:03:35 PDT
Comment on attachment 158547 [details]
Patch for landing

Clearing flags on attachment: 158547

Committed r125670: <http://trac.webkit.org/changeset/125670>
Comment 38 WebKit Review Bot 2012-08-15 06:03:45 PDT
All reviewed patches have been landed.  Closing bug.