We need to do some work directly on the WebProcess side, so set up a WebAutomationSessionProxy object and messages.
<rdar://problem/25054868>
Created attachment 273402 [details] WIP Sketch This builds but I didn't test it in any way.
Comment on attachment 273402 [details] WIP Sketch View in context: https://bugs.webkit.org/attachment.cgi?id=273402&action=review > Source/WebKit2/UIProcess/Automation/WebAutomationSession.h:53 > +class WebAutomationSession final : public API::ObjectImpl<API::Object::Type::AutomationSession>, public IPC::MessageReceiver This needs to register as a message receiver with the process pool when WebAutomationSession::setProcessPool is called. processPool->addMessageReceiver(Messages::WebAutomationSession::messageReceiverName(), *this); And unregister is the destructor.
Comment on attachment 273402 [details] WIP Sketch View in context: https://bugs.webkit.org/attachment.cgi?id=273402&action=review > Source/WebKit2/DerivedSources.make:113 > + WebAutomationSessionProxy \ Need the analogue changes for CMakeLists.txt, too. > Source/WebKit2/WebProcess/WebProcess.h:355 > + RefPtr<WebAutomationSessionProxy> m_automationSessionProxy; Shouldn't this be owned by the WebProcess class? Not seeing the need for RefPtr in my imagination yet..
Comment on attachment 273402 [details] WIP Sketch View in context: https://bugs.webkit.org/attachment.cgi?id=273402&action=review >> Source/WebKit2/WebProcess/WebProcess.h:355 >> + RefPtr<WebAutomationSessionProxy> m_automationSessionProxy; > > Shouldn't this be owned by the WebProcess class? Not seeing the need for RefPtr in my imagination yet.. You are right. It could be an OwnedPtr that we create and destroy as needed.
Created attachment 273551 [details] WIP Sketch v2 This adds an injected script to the WebProcess side so it is easy to implement the JS heavy features like element mapping and evaluating scripts. This patch replaces the refresh end point and calls a test function on the injected script. I verified this worked before passing out.
Comment on attachment 273551 [details] WIP Sketch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=273551&action=review > Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:258 > + page->process().send(Messages::WebAutomationSessionProxy::EvaluateJavaScript(page->mainFrame()->frameID(), WTF::emptyString()), 0); Note: mainFrame can be null. It is null when the WebPageProxy is first created. I am not sure on when it is available yet, be we should null check it. > Source/WebKit2/UIProcess/Automation/WebAutomationSession.messages.in:25 > +messages -> WebAutomationSession { > + Test() > +} We will likely need these messages to the UIProcess to implement execute_async or any other events. > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:101 > + JSRetainPtr<JSStringRef> testString(Adopt, JSStringCreateWithUTF8CString("test")); > + JSObjectRef testFunction = (JSObjectRef)JSObjectGetProperty(context, scriptObject, testString.get(), &exception); > + RELEASE_ASSERT(!exception); > + RELEASE_ASSERT(JSObjectIsFunction(context, testFunction)); > + > + JSObjectCallAsFunction(context, testFunction, nullptr, 0, nullptr, &exception); > + RELEASE_ASSERT(!exception); Just a test. This would call JSEvaluateScript and pass the result to the injected script for processing (turning elements into handles, etc.) > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:43 > + test() > + { > + console.log("Hello World!"); > + } This gets called in the end by requesting the refresh endpoint right now. > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1584 > + if (auto automationSessionProxy = WebProcess::singleton().automationSessionProxy()) > + automationSessionProxy->didClearWindowObjectForFrame(*m_frame); Needed so we unprotect our script object ref.
Created attachment 274056 [details] Patch
Comment on attachment 274056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274056&action=review Looks good, I'll let Brian review the Automation bits. > Source/WebKit2/CMakeLists.txt:799 > + COMMAND ${CMAKE_COMMAND} -E echo "//# sourceURL=__WebInspectorInjectedScript__" > ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxy.min.js Should we be reusing the sourceURL "__WebInspectorInjectedScript__" here? Perhaps "__WebAutomationInjectedScript__"? If we want this to be hidden from WebInspector we can update: Utilities.js function isWebInspectorDebugScript(url) { return url && url.startsWith("__WebInspector"); } To just check "__Web" or something. > Source/WebKit2/DerivedSources.make:236 > + echo "//# sourceURL=__WebInspectorInjectedScript__" > $(basename $(notdir $<)).min.js Same comment about the name "WebInspector". > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:92 > + JSObjectRef scriptObjectFunction = const_cast<JSObjectRef>(JSEvaluateScript(context, toJSString(script).get(), nullptr, nullptr, 0, &exception)); I think JSValueToObject be safer then these casts, is there precedent for just casting? > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:26 > +//# sourceURL=__WebInspectorInjectedScript__ Same comment about the use of "WebInspector" here. > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:32 > +var Object = {}.constructor; > + > +var AutomationSessionProxy = class AutomationSessionProxy Use `let` instead of `var? `Object` and evaluate/createUUID aren't used yet, but I expect they will be soon.
Comment on attachment 274056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274056&action=review > Source/WebKit2/CMakeLists.txt:795 > +add_custom_command( This command should go after the list(APPEND WebKit2_HEADERS for the inspector generated files. > Source/WebKit2/CMakeLists.txt:802 > + VERBATIM) The CMake build failed because you forgot the following: list(APPEND WebKit2_HEADERS ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h)
Comment on attachment 274056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274056&action=review >> Source/WebKit2/CMakeLists.txt:795 >> +add_custom_command( > > This command should go after the list(APPEND WebKit2_HEADERS for the inspector generated files. Thanks. I was just throwing crap at the wall here. >> Source/WebKit2/CMakeLists.txt:799 >> + COMMAND ${CMAKE_COMMAND} -E echo "//# sourceURL=__WebInspectorInjectedScript__" > ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxy.min.js > > Should we be reusing the sourceURL "__WebInspectorInjectedScript__" here? Perhaps "__WebAutomationInjectedScript__"? > > If we want this to be hidden from WebInspector we can update: Utilities.js > > function isWebInspectorDebugScript(url) > { > return url && url.startsWith("__WebInspector"); > } > > To just check "__Web" or something. Yeah, I will make that change. I will post a separate patch for Utilities.js. >> Source/WebKit2/CMakeLists.txt:802 >> + VERBATIM) > > The CMake build failed because you forgot the following: > > list(APPEND WebKit2_HEADERS ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h) Thanks. >> Source/WebKit2/DerivedSources.make:236 >> + echo "//# sourceURL=__WebInspectorInjectedScript__" > $(basename $(notdir $<)).min.js > > Same comment about the name "WebInspector". Will do. >> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:32 >> +var AutomationSessionProxy = class AutomationSessionProxy > > Use `let` instead of `var? `Object` and evaluate/createUUID aren't used yet, but I expect they will be soon. Will do. Yes a follow up uses them.
Comment on attachment 274056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274056&action=review >> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:92 >> + JSObjectRef scriptObjectFunction = const_cast<JSObjectRef>(JSEvaluateScript(context, toJSString(script).get(), nullptr, nullptr, 0, &exception)); > > I think JSValueToObject be safer then these casts, is there precedent for just casting? I can change it. Some Safari extensions code just casts for values expected to be objects. In this case we control both sides of the code.
Created attachment 274075 [details] Patch
Comment on attachment 274075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274075&action=review > Source/WebKit2/UIProcess/Automation/WebAutomationSession.h:71 > + // Called by WebAutomationSession messages > + // FIXME: Add message functions here. > + void test() { }; Messages handlers should be private.
Comment on attachment 274075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274075&action=review >> Source/WebKit2/UIProcess/Automation/WebAutomationSession.h:71 >> + void test() { }; > > Messages handlers should be private. Okay, will change.
Created attachment 274136 [details] Patch
https://trac.webkit.org/r198736
So... Looks like the reason why the Gtk and Efl bots are red is a typo. Which I can fix with this: diff --git a/Source/WebKit2/CMakeLists.txt b/Source/WebKit2/CMakeLists.txt index 85dd3d7..71278d4 100644 --- a/Source/WebKit2/CMakeLists.txt +++ b/Source/WebKit2/CMakeLists.txt @@ -815,7 +815,7 @@ add_custom_command( VERBATIM) list(APPEND WebKit2_HEADERS - ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h + ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxyScriptSource.h ) But now the build fails with: ninja: error: '/xxd.pl', needed by 'DerivedSources/WebKit2/WebAutomationSessionProxyScriptSource.h', missing and no known rule to make it Suggestions?
(In reply to comment #18) > So... Looks like the reason why the Gtk and Efl bots are red is a typo. > Which I can fix with this: > > diff --git a/Source/WebKit2/CMakeLists.txt b/Source/WebKit2/CMakeLists.txt > index 85dd3d7..71278d4 100644 > --- a/Source/WebKit2/CMakeLists.txt > +++ b/Source/WebKit2/CMakeLists.txt > @@ -815,7 +815,7 @@ add_custom_command( > VERBATIM) > > list(APPEND WebKit2_HEADERS > - ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h > + ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxyScriptSource.h > ) > > But now the build fails with: > > ninja: error: '/xxd.pl', needed by > 'DerivedSources/WebKit2/WebAutomationSessionProxyScriptSource.h', missing > and no known rule to make it > > Suggestions? xxd.pl should be in the JavaScriptCore_SCRIPTS_DIR directory. $(PERL) $(JavaScriptCore_SCRIPTS_DIR)/xxd.pl
(In reply to comment #19) > xxd.pl should be in the JavaScriptCore_SCRIPTS_DIR directory. > > $(PERL) $(JavaScriptCore_SCRIPTS_DIR)/xxd.pl Looks like JavaScriptCore_SCRIPTS_DIR is an empty string :)
This fixes the CMake part --- a/Source/WebKit2/CMakeLists.txt +++ b/Source/WebKit2/CMakeLists.txt @@ -750,6 +750,12 @@ set(PluginProcess_LIBRARIES WebKit2 ) +if (WIN32 AND INTERNAL_BUILD) + set(JavaScriptCore_SCRIPTS_DIR "${CMAKE_BINARY_DIR}/../include/private/JavaScriptCore/Scripts") +else () + set(JavaScriptCore_SCRIPTS_DIR "${DERIVED_SOURCES_DIR}/ForwardingHeaders/JavaScriptCore/Scripts") +endif () + # librt is needed for shm_open on Linux. find_library(LIBRT_LIBRARIES NAMES rt) mark_as_advanced(LIBRT_LIBRARIES) @@ -815,7 +821,7 @@ add_custom_command( VERBATIM) list(APPEND WebKit2_HEADERS - ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h + ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxyScriptSource.h ) But then the build fails here: In file included from ../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:27: ../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.h:68:12: error: field has incomplete type 'WTF::String' String m_sessionIdentifier; ^ ../../Source/WTF/wtf/Forward.h:49:7: note: forward declaration of 'WTF::String' class String; ^ In file included from ../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:27: ../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.h:46:12: error: incomplete result type 'WTF::String' in function definition String sessionIdentifier() const { return m_sessionIdentifier; } ^ ../../Source/WTF/wtf/Forward.h:49:7: note: forward declaration of 'WTF::String' class String; ^ ../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:101:29: error: no matching member function for call to 'addMessageReceiver' WebProcess::singleton().addMessageReceiver(Messages::WebAutomationSessionProxy::messageReceiverName(), *this); ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~ ../../Source/WebKit2/Shared/ChildProcess.h:61:10: note: candidate function not viable: no known conversion from 'WebKit::WebAutomationSessionProxy' to 'IPC::MessageReceiver &' for 2nd argument void addMessageReceiver(IPC::StringReference messageReceiverName, IPC::MessageReceiver&); ^ ../../Source/WebKit2/Shared/ChildProcess.h:62:10: note: candidate function not viable: requires 3 arguments, but 2 were provided void addMessageReceiver(IPC::StringReference messageReceiverName, uint64_t destinationID, IPC::MessageReceiver&); ^ 3 errors generated. [107/604] Building CXX object Source/WebKit2/CMakeFiles/WebKit2.dir/WebProcess/WebPage/WebPage.cpp.o
Committed r198757: <http://trac.webkit.org/changeset/198757>
I'm a bit confused why we had to redefine JavaScriptCore_SCRIPTS_DIR in WebKit2/CMakeLists.txt. I wonder if CMake is processing WebKit2/CMakeLists.txt before it processes JavaScriptCore/CMakeLists.txt.
(In reply to comment #23) > I'm a bit confused why we had to redefine JavaScriptCore_SCRIPTS_DIR in > WebKit2/CMakeLists.txt. I wonder if CMake is processing > WebKit2/CMakeLists.txt before it processes JavaScriptCore/CMakeLists.txt. Is also redefined in Source/WebCore/CMakeLists.txt Basically I copied the definition from there.
(In reply to comment #23) > I'm a bit confused why we had to redefine JavaScriptCore_SCRIPTS_DIR in > WebKit2/CMakeLists.txt. I wonder if CMake is processing > WebKit2/CMakeLists.txt before it processes JavaScriptCore/CMakeLists.txt. Sorry about the breakage folks, Mac CMake just started working again recently or I would have caught it. I am not sure why variable needs to be redefined, but I have run into this before and just redefined it without figuring why. :|