Bug 89072 – [WK2] Add support for Web Intents MessagePorts

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

[Chris Dumez]

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.

[Chris Dumez]

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?

[Chris Dumez]

Created attachment 155557 [details]
Patch

[Chris Dumez]

With this patch, all the webintents/* layout tests are now passing for WK2 EFL.

[Build Bot]

Comment on attachment 155557 [details]
Patch

Attachment 155557 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13410014

[Chris Dumez]

Created attachment 155572 [details]
Patch

Attempt to fix Windows build.

[Chris Dumez]

Created attachment 155573 [details]
Patch

[Chris Dumez]

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?

[Build Bot]

Comment on attachment 155573 [details]
Patch

Attachment 155573 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13404144

[Chris Dumez]

Created attachment 155611 [details]
Patch

Attempt to fix Mac build.

[Chris Dumez]

Created attachment 155615 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 156152 [details]
Patch

- Add empty line before return
- Attempt to clarify Changelogs

[Chris Dumez]

(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

[Kenneth Rohde Christiansen]

What about moving it in a preparation patch?

[Chris Dumez]

(In reply to comment #16)
> What about moving it in a preparation patch?

Sure, done in Bug 93175.

[Chris Dumez]

Created attachment 156531 [details]
Patch

Rebase on master now that the dependency landed.

[Kenneth Rohde Christiansen]

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);

[Chris Dumez]

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.

[Kenneth Rohde Christiansen]

> 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.

[Kenneth Rohde Christiansen]

I think Anders reviewed the original C api, it would be great if he could have a look again.

[Chris Dumez]

(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.

[Kenneth Rohde Christiansen]

> 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 :-)

[Chris Dumez]

Created attachment 156537 [details]
Patch

Take Kenneth's feedback into consideration.

[Chris Dumez]

Created attachment 157902 [details]
Patch

Rebase on master and unskip related test cases for WebKit2 EFL.

[Chris Dumez]

Created attachment 157998 [details]
Patch

Rebase on master.

[Kenneth Rohde Christiansen]

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

[Chris Dumez]

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).

[Chris Dumez]

Created attachment 158384 [details]
Patch

Take Kenneth's feedback into consideration. Note that I did not rename deliverCoreIntentToFrame() (see previous comment).

[Gyuyoung Kim]

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*);

[Chris Dumez]

Created attachment 158500 [details]
Patch

Take Gyuyoung nits into account.

[Gyuyoung Kim]

Comment on attachment 158500 [details]
Patch

Attachment 158500 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13504460

[Chris Dumez]

Created attachment 158502 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[Chris Dumez]

Created attachment 158547 [details]
Patch for landing

Fix nit spotted by Kenneth.

[WebKit Review Bot]

Comment on attachment 158547 [details]
Patch for landing

Clearing flags on attachment: 158547

Committed r125670: <http://trac.webkit.org/changeset/125670>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.