<rdar://problem/17748414>

Internal dupe of <rdar://problem/11749409>.

I am working on this.

Created attachment 237676 [details] WIP

Created attachment 237677 [details] WIP (correct)

Created attachment 237727 [details] WIP (latest)

Comment on attachment 237727 [details] WIP (latest) View in context: https://bugs.webkit.org/attachment.cgi?id=237727&action=review Looks pretty slick overall. Seeking some clarification on the code path for GTK/EFL: does it change at all? Some of the messages looked deleted but I didn't see any updates to the other platform-specific files. > Source/WebCore/inspector/InspectorController.cpp:198 > +void InspectorController::setInspectorFrontendClient(InspectorFrontendClient* inspectorFrontendClient) Before, the Controller owned the client. Who owns it now? Can this pass a reference? > Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:-137 > - , m_frontendClient(0) This should still be initialized to nullptr, right? > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:110 > pageGroup->preferences().setLogsPageMessagesToSystemConsoleEnabled(true); Can you update the comment above this to mean "developers can inspector the inspector in debug mode _without changing any settings_" ? AFAICT, that's what it does. > Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:1 > /* Per the naming conventions, files that implement the InspectorProcess probably should be in Source/WebKit2/InspectorProcess/* not in WebProcess. > Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:30 > +#include "WebPage.h" newline after ENABLE(...)

Comment on attachment 237727 [details] WIP (latest) View in context: https://bugs.webkit.org/attachment.cgi?id=237727&action=review >> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:-137 >> - , m_frontendClient(0) > > This should still be initialized to nullptr, right? It was removed. They aren't in the same process anymore. >> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:110 >> pageGroup->preferences().setLogsPageMessagesToSystemConsoleEnabled(true); > > Can you update the comment above this to mean "developers can inspector the inspector in debug mode _without changing any settings_" ? AFAICT, that's what it does. Sure. >> Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:1 >> /* > > Per the naming conventions, files that implement the InspectorProcess probably should be in Source/WebKit2/InspectorProcess/* not in WebProcess. They are in WebProcess, just two different web processes now.

Comment on attachment 237727 [details] WIP (latest) View in context: https://bugs.webkit.org/attachment.cgi?id=237727&action=review >>> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:-137 >>> - , m_frontendClient(0) >> >> This should still be initialized to nullptr, right? > > It was removed. They aren't in the same process anymore. Oh, this is WK1. m_frontendClient is a unique_ptr now which defaults to nullptr.

Comment on attachment 237727 [details] WIP (latest) View in context: https://bugs.webkit.org/attachment.cgi?id=237727&action=review >> Source/WebCore/inspector/InspectorController.cpp:198 >> +void InspectorController::setInspectorFrontendClient(InspectorFrontendClient* inspectorFrontendClient) > > Before, the Controller owned the client. Who owns it now? Can this pass a reference? The higher level objects in WK2 and WK1 own it now with a lifetime of the Page. It might be possible to pass as a reference, but it needs to be nullable.

Created attachment 237798 [details] WIP (almost final) Posting to let EWS try things while I look into a layout test hang.

Attachment 237798 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:32: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:39: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:42: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:145: Declaration has space between type name and * in const char *sideString [whitespace/declaration] [3] ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.h:39: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebCore/WebCore.exp.in:0: Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script [list/order] [5] Total errors found: 6 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 237803 [details] WIP (almost final x2)

Created attachment 237806 [details] WIP (almost final x3)

Created attachment 237818 [details] WIP (almost final x4)

Comment on attachment 237818 [details] WIP (almost final x4) Attachment 237818 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5085997704413184 New failing tests: http/tests/xmlhttprequest/access-control-repeated-failed-preflight-crash.html

Created attachment 237830 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Created attachment 238037 [details] WIP (latest final)

Comment on attachment 238037 [details] WIP (latest final) View in context: https://bugs.webkit.org/attachment.cgi?id=238037&action=review Will look at this more later, but some quick comments: * Missing ChangeLogs. * Many tests in LayoutTests/inspector/ will crash under WK1 if you force them to run. This is a regression. * Saving resources seems to work, but it names the downloaded files as "Unknown". Is this a regression? > Source/WebKit2/UIProcess/gtk/WebInspectorProxyGtk.cpp:185 > + // Set a default sizes based on InspectorFrontendClientLocal. grammar.

It would also be nice to name the inspector process something different from com.apple.WebKit.WebContent[.Development] so that different web processes can be easily distinguished from Xcode.

Looks like Safari expects some symbols that were removed in this patch. Maybe these should be stubbed out and deprecated until Safari no longer uses them. dyld: lazy symbol binding failed: Symbol not found: _WKInspectorIsProfilingJavaScript Referenced from: /System/Library/StagedFrameworks/Safari/Safari.framework/Versions/A/Safari Expected in: /Users/burg/repos/replay-staging/OpenSource/WebKitBuild/Debug/WebKit2.framework/Versions/A/WebKit2

(In reply to comment #21) > Looks like Safari expects some symbols that were removed in this patch. Maybe these should be stubbed out and deprecated until Safari no longer uses them. > > dyld: lazy symbol binding failed: Symbol not found: _WKInspectorIsProfilingJavaScript > Referenced from: /System/Library/StagedFrameworks/Safari/Safari.framework/Versions/A/Safari > Expected in: /Users/burg/repos/replay-staging/OpenSource/WebKitBuild/Debug/WebKit2.framework/Versions/A/WebKit2 To clarify, this happens when you open the Develop menu in Safari launched from `run-safari`. Another bug (which I'll have to unapply the patch to get around, for now): the `pagehide` event is not getting fired when the inspector closes, so breakpoints and the current view state are not being persisted across reopening the inspector.

(In reply to comment #20) > It would also be nice to name the inspector process something different from com.apple.WebKit.WebContent[.Development] so that different web processes can be easily distinguished from Xcode. Joe and I talked about this too. One issue is we are not guaranteed to be in our own process. Safari sets the process limit to 20. We should force our own process that all inspectors share, then we can name it properly.

(In reply to comment #21) > Looks like Safari expects some symbols that were removed in this patch. Maybe these should be stubbed out and deprecated until Safari no longer uses them. > > dyld: lazy symbol binding failed: Symbol not found: _WKInspectorIsProfilingJavaScript > Referenced from: /System/Library/StagedFrameworks/Safari/Safari.framework/Versions/A/Safari > Expected in: /Users/burg/repos/replay-staging/OpenSource/WebKitBuild/Debug/WebKit2.framework/Versions/A/WebKit2 Oops. Yeah, we need to stub those out.

(In reply to comment #22) > Another bug (which I'll have to unapply the patch to get around, for now): the `pagehide` event is not getting fired when the inspector closes, so breakpoints and the current view state are not being persisted across reopening the inspector. I thought Joe was having issues with page hide not firing already?

(In reply to comment #25) > (In reply to comment #22) > > Another bug (which I'll have to unapply the patch to get around, for now): the `pagehide` event is not getting fired when the inspector closes, so breakpoints and the current view state are not being persisted across reopening the inspector. > > I thought Joe was having issues with page hide not firing already? I recall some discussion about this, but can't find a bug. And, it is definitely a regression with this patch applied vs not.

(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #22) > > > Another bug (which I'll have to unapply the patch to get around, for now): the `pagehide` event is not getting fired when the inspector closes, so breakpoints and the current view state are not being persisted across reopening the inspector. > > > > I thought Joe was having issues with page hide not firing already? > > I recall some discussion about this, but can't find a bug. And, it is definitely a regression with this patch applied vs not. In my last tip of tree build page hide events were working as expected. Were you still seeing issues? That will certainly affect state restoration as well.

Created attachment 238427 [details] WIP (For Bots) This version passes inspector and inspector-protocol tests. Still need ChangeLogs, then I will post it for review. Checking EWS first.

Comment on attachment 238427 [details] WIP (For Bots) View in context: https://bugs.webkit.org/attachment.cgi?id=238427&action=review > Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:127 > + console.assert(InspectorFrontendAPI[methodName], "Unexpected InspectorFrontendAPI method name: " + InspectorFrontendAPI[methodName]); I wish we could do: if (!console.assert(pred, ...)) return; But this would require some more intelligent rewriting to strip console assertions.

Created attachment 238430 [details] WIP (Rebased For Bots)

Comment on attachment 238430 [details] WIP (Rebased For Bots) "Binary files a/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js and b/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js differ" ^-- this is causing the patch to not apply. What diff command are you using? I've never had this problem using `Tools/Scripts/webkit-patch upload`

Created attachment 238437 [details] WIP (Binary Patch)

Attachment 238437 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/WebInspectorProxy.cpp:351: Missing space before { [whitespace/braces] [5] Total errors found: 1 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 238446 [details] Patch

This will need a WebKIt2 owner review.

Comment on attachment 238446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238446&action=review > Source/WebKit2/ChangeLog:108 > + Stop taking an optional InspectorFrontendChannel and be it directly. Create a connection that can 'be it'

Created attachment 238454 [details] Patch

Created attachment 238483 [details] Patch

Created attachment 238499 [details] Patch

Created attachment 238503 [details] Patch

Comment on attachment 238503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238503&action=review > Source/WebCore/testing/Internals.cpp:1495 > + m_frontendWindow.clear(); This should use = nullptr. > Source/WebKit2/UIProcess/API/C/WKInspector.cpp:177 > -bool WKInspectorIsDebuggingJavaScript(WKInspectorRef inspectorRef) > +bool WKInspectorIsDebuggingJavaScript(WKInspectorRef) As discussed, these should move to WKDeprecatedFunctions. > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:359 > + static std::once_flag onceFlag; > + static LazyNeverDestroyed<RefPtr<WebContext>> context; > + > + std::call_once(onceFlag, [] { > + WebContextConfiguration configuration; > + WebContext::applyPlatformSpecificConfigurationDefaults(configuration); > + context.construct(WebContext::create(configuration)); > + context.get()->setProcessModel(ProcessModelMultipleSecondaryProcesses); > + }); There's no need to put a smart pointer inside a LazyNeverDestroyed. This can just be something like: static WebContext* context; if (!context) { /// initialize context } return *context; > Source/WebKit2/WebProcess/WebPage/WebInspector.messages.in:25 > -messages -> WebInspector LegacyReceiver { > +messages -> WebInspector { Very nice!

32-bit mac build fix: <http://trac.webkit.org/changeset/173933>

WK1 test crash fix: http://trac.webkit.org/changeset/173936

64-bit Windows build fix: http://trac.webkit.org/changeset/173986.