Summary: Provide a way to open an inspector frontend for a remote target. The Remote Web Inspector in Safari currently uses its own separate path for opening a frontend for a remote debuggable (JSContext, WebView). Implementing this in WebKit will have a number of advantages by sharing the implementation with the normal local Web Inspector. Unifying the implementations where possible will address a few subtle differences in their behavior, and making it much easier to modify this configuration.
<rdar://problem/13182127>
Created attachment 287696 [details] [PATCH] Proposed Fix
Attachment 287696 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/WebInspectorProxy.h:102: The parameter name "page" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:88: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:90: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:129: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:130: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 5 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 287696 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=287696&action=review > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:-116 > - pageLevelMap().remove(m_inspectedPage); Oh, I didn't mention in my changelog but this was a fix to the local inspector. This was wrong. It was using the inspected page instead of the inspector page so that is just plain wrong. Using new names trackInspectorPage and untrackInspectorPage this became more clear, and the new location is down below (didClose) before clearing m_inspectorPage.
Seems I need a dummy RemoteWebInspectorProxy implementation for non-Mac ports used by generated code. /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/DerivedSources/WebKit2/RemoteWebInspectorProxyMessageReceiver.cpp:64:126: error: no member named 'sendMessageToBackend' in namespace 'Messages::RemoteWebInspectorProxy' IPC::handleMessage<Messages::RemoteWebInspectorProxy::SendMessageToBackend>(decoder, this, &RemoteWebInspectorProxy::sendMessageToBackend); I'll be cleaning this up.
Created attachment 287706 [details] [PATCH] Proposed Fix
Created attachment 287715 [details] [PATCH] Proposed Fix
Created attachment 287722 [details] [PATCH] Proposed Fix
Created attachment 287729 [details] [PATCH] Proposed Fix
Created attachment 287734 [details] [PATCH] Proposed Fix Still trying to get EFL building.
Created attachment 287777 [details] [PATCH] Proposed Fix
Comment on attachment 287777 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=287777&action=review (Taking a break for lunch) > Source/WebCore/ChangeLog:11 > + Now it may create a frontend for a javascript or web 'javascript' and 'web' > Source/WebInspectorUI/UserInterface/Protocol/LoadInspectorBackendCommands.js:28 > + if (!backendCommandsURL) let backendCommandsURL = a || b > Source/WebKit2/ChangeLog:12 > + support and older version of the protocol. The Inspector frontend 'an' > Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.h:61 > +- (void)inspectorViewControllerInspectorDidClose:(_WKRemoteWebInspectorViewController *)controller; FrontendDidClose? > Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:141 > +#endif // PLATFORM(MAC) Nit: && WK_API_ENABLED in the comment > Source/WebKit2/UIProcess/RemoteWebInspectorProxy.cpp:147 > +{ > + if (m_inspectorPage) > + return; > + > + m_inspectorPage = platformCreateFrontendPageAndWindow(); > + > + trackInspectorPage(m_inspectorPage); > + > + m_inspectorPage->process().addMessageReceiver(Messages::RemoteWebInspectorProxy::messageReceiverName(), m_inspectorPage->pageID(), *this); > +} > + > +void RemoteWebInspectorProxy::closeFrontendPageAndWindow() > +{ > + if (!m_inspectorPage) > + return; > + > + m_inspectorPage->process().removeMessageReceiver(Messages::RemoteWebInspectorProxy::messageReceiverName(), m_inspectorPage->pageID()); > + > + untrackInspectorPage(m_inspectorPage); > + > + m_inspectorPage = nullptr; > + > + platformCloseFrontendPageAndWindow(); > +} > + XX > Source/WebKit2/UIProcess/WebInspectorUtilities.cpp:80 > + WebProcessPool*& pool = inspectionLevel == 1 ? s_mainInspectorProcessPool : s_nestedInspectorProcessPool; please put the conditional in parens to make easier to parse.
Comment on attachment 287777 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=287777&action=review r=me, this is very clean Joe! Great work. > Source/WebKit2/ChangeLog:61 > + Previously this was part of WebInspectorProxy and subclasses but can This could be a separate patch but I don't care at this point =) >> Source/WebKit2/UIProcess/RemoteWebInspectorProxy.cpp:147 >> + > > XX disregard my placeholder. > Source/WebKit2/UIProcess/RemoteWebInspectorProxy.h:35 > +#if PLATFORM(MAC) I am not sure there is any point to including the RemoteInspectorProxy stuff on non-Mac/iOS ports. But this is a guard for iOS right? > Source/WebKit2/UIProcess/RemoteWebInspectorProxy.h:106 > + RemoteWebInspectorProxyClient *m_client { nullptr }; Nit: this is a C++ class, right? > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:522 > + Nice catch. > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:219 > + [configuration setProcessPool: ::WebKit::wrapper(inspectorProcessPool(inspectorLevel))]; Maybe this should be inspectorProcessPoolForLevel ?
Comment on attachment 287777 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=287777&action=review >> Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.h:61 >> +- (void)inspectorViewControllerInspectorDidClose:(_WKRemoteWebInspectorViewController *)controller; > > FrontendDidClose? I figured this is the InspectorViewController so if the Inspector closed that would be enough. I'll leave this as is but we can always change it later. >> Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:141 >> +#endif // PLATFORM(MAC) > > Nit: && WK_API_ENABLED in the comment This seems to be a common pattern (all other COCOA headers with both in the #if just have this comment at the end), so I left it as is. >> Source/WebKit2/UIProcess/RemoteWebInspectorProxy.h:35 >> +#if PLATFORM(MAC) > > I am not sure there is any point to including the RemoteInspectorProxy stuff on non-Mac/iOS ports. But this is a guard for iOS right? I ended up including it to get things to compile. There may be a way to do this that I overlooked, but it is a very small unused object on ports that don't use it. >> Source/WebKit2/UIProcess/RemoteWebInspectorProxy.h:106 >> + RemoteWebInspectorProxyClient *m_client { nullptr }; > > Nit: this is a C++ class, right? Yep! Moved the *.
<https://trac.webkit.org/changeset/205369>
This patch caused a regression on the El Capitan CMake build: https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/8067
Never mind, fixed in https://trac.webkit.org/changeset/205375.
(In reply to comment #15) > <https://trac.webkit.org/changeset/205369> and the Apple Mac cmake build fixed by https://trac.webkit.org/changeset/205545
(In reply to comment #18) > (In reply to comment #15) > > <https://trac.webkit.org/changeset/205369> > > and the Apple Mac cmake build fixed by > https://trac.webkit.org/changeset/205545 Doh, thanks!