IPC testing JS API should have the ability to intercept IPC messaging being sent and received in Web process.\ <rdar://problem/70350874>
Created attachment 411963 [details] Patch
Comment on attachment 411963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411963&action=review > Source/WebKit/Platform/IPC/Connection.cpp:1044 > + if (hasDeadObservers) > + m_messageObservers.removeAllMatching([](auto& observer) { return !observer; }); Seems like we could use removeAllMatching to remove while iterating, and call didReceiveMessage on the ones we don’t remove instead of having two loops and a boolean. > Source/WebKit/Platform/IPC/JSIPCBinding.h:55 > + object->putDirect(vm, JSC::Identifier::fromString(vm, "type"_s), JSC::jsNontrivialString(vm, type)); What guarantees non-trivial for this string? Maybe these are always string constants. If so, consider making the argument ASCIILiteral instead of const String&. Even type could be ASCIILiteral instead of const String&. > Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:909 > + JSObjectCallAsFunction(m_context, m_callback, m_callback, WTF_ARRAY_LENGTH(arguments), arguments, nullptr); Should be able to use std::size instead of WTF_ARRAY_LENGTH in new code. We can probably get rid of WTF_ARRAY_LENGTH. > Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:257 > + RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]); auto? > Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:302 > + RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 300, 300) configuration:configuration.get()]); auto?
(In reply to Darin Adler from comment #2) > Comment on attachment 411963 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411963&action=review > > > Source/WebKit/Platform/IPC/Connection.cpp:1044 > > + if (hasDeadObservers) > > + m_messageObservers.removeAllMatching([](auto& observer) { return !observer; }); > > Seems like we could use removeAllMatching to remove while iterating, and > call didReceiveMessage on the ones we don’t remove instead of having two > loops and a boolean. Hm... that sounds a bit fishy given removeAllMatching could resize the buffer. I'd rather separate the removal part after the loop where we don't have a pointer anymore. > > Source/WebKit/Platform/IPC/JSIPCBinding.h:55 > > + object->putDirect(vm, JSC::Identifier::fromString(vm, "type"_s), JSC::jsNontrivialString(vm, type)); > > What guarantees non-trivial for this string? Maybe these are always string > constants. If so, consider making the argument ASCIILiteral instead of const > String&. Yes. > Even type could be ASCIILiteral instead of const String&. Oh, I thought _s creates ASCIILiteral. I guess it directly creates String string instead. Good to know. > > Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:909 > > + JSObjectCallAsFunction(m_context, m_callback, m_callback, WTF_ARRAY_LENGTH(arguments), arguments, nullptr); > > Should be able to use std::size instead of WTF_ARRAY_LENGTH in new code. We > can probably get rid of WTF_ARRAY_LENGTH. Oh, that's a good point. Done. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:257 > > + RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]); > > auto? Oh, actually, I should be using createWebViewWithIPCTestingAPI that I had recently added. Done that instead.
Committed r268848: <https://trac.webkit.org/changeset/268848>
Reopening to attach new patch.
Created attachment 412118 [details] Patch
Created attachment 412119 [details] Patch
Comment on attachment 411963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411963&action=review >>> Source/WebKit/Platform/IPC/Connection.cpp:1044 >>> + m_messageObservers.removeAllMatching([](auto& observer) { return !observer; }); >> >> Seems like we could use removeAllMatching to remove while iterating, and call didReceiveMessage on the ones we don’t remove instead of having two loops and a boolean. > > Hm... that sounds a bit fishy given removeAllMatching could resize the buffer. I'd rather separate the removal part after the loop where we don't have a pointer anymore. No, I don’t agree. Are you worried about didReceiveMessage calling out while iterating m_messageObservers and then m_messageObservers or this being changed out form under us? If so, then that’s *already* a problem in the code above, which needs to change.
(In reply to Ryosuke Niwa from comment #3) > > Even type could be ASCIILiteral instead of const String&. > > Oh, I thought _s creates ASCIILiteral. I guess it directly creates String > string instead. Good to know. You were right, _s *does* create ASCIILiteral. I meant that the argument named type could be ASCIILiteral. Yes, "type"_s already is.
Committed r268883: <https://trac.webkit.org/changeset/268883> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412119 [details].