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.
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?
Created attachment 155557 [details] Patch
With this patch, all the webintents/* layout tests are now passing for WK2 EFL.
Comment on attachment 155557 [details] Patch Attachment 155557 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13410014
Created attachment 155572 [details] Patch Attempt to fix Windows build.
Created attachment 155573 [details] Patch
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 on attachment 155573 [details] Patch Attachment 155573 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13404144
Created attachment 155611 [details] Patch Attempt to fix Mac build.
Created attachment 155615 [details] Patch
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 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.
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.
Created attachment 156152 [details] Patch - Add empty line before return - Attempt to clarify Changelogs
(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
What about moving it in a preparation patch?
(In reply to comment #16) > What about moving it in a preparation patch? Sure, done in Bug 93175.
Created attachment 156531 [details] Patch Rebase on master now that the dependency landed.
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 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.
> 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.
I think Anders reviewed the original C api, it would be great if he could have a look again.
(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.
> 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 :-)
Created attachment 156537 [details] Patch Take Kenneth's feedback into consideration.
Created attachment 157902 [details] Patch Rebase on master and unskip related test cases for WebKit2 EFL.
Created attachment 157998 [details] Patch Rebase on master.
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 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).
Created attachment 158384 [details] Patch Take Kenneth's feedback into consideration. Note that I did not rename deliverCoreIntentToFrame() (see previous comment).
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*);
Created attachment 158500 [details] Patch Take Gyuyoung nits into account.
Comment on attachment 158500 [details] Patch Attachment 158500 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13504460
Created attachment 158502 [details] Patch
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.
Created attachment 158547 [details] Patch for landing Fix nit spotted by Kenneth.
Comment on attachment 158547 [details] Patch for landing Clearing flags on attachment: 158547 Committed r125670: <http://trac.webkit.org/changeset/125670>
All reviewed patches have been landed. Closing bug.