Most of the RemoteInspector implementation is actually cross-platform, but RemoteInspector.mm and RemoteConnectionToTarget.mm need to be factored out.
Created attachment 298005 [details] WIP patch I need someone with a mac who can add make the XCode changes after the refactoring, please. I'll deal with builds breaks using EWS.
Created attachment 298008 [details] WIP patch
Created attachment 298462 [details] WIP: trying xcode changes
Attachment 298462 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:111: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:82: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:163: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:198: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:216: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:50: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:69: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:71: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:147: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:209: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:452: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:562: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:588: No space between ^ and block definition. [whitespace/brackets] [4] Total errors found: 20 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 298469 [details] WIP: try to fix the mac builds and some style issues
Attachment 298469 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 298470 [details] WIP: Try to fix mac builds
Attachment 298470 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 298472 [details] WIP: Try to fix mac builds
Attachment 298472 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 298474 [details] WIP: Trying to fix mac builds
Attachment 298474 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 298557 [details] Patch
Attachment 298557 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ping reviewers. This is just a refactoring.
Comment on attachment 298557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298557&action=review Looks straightforward, and inspector devs have had plenty of time to complain by now if something's undesired. > Source/JavaScriptCore/ChangeLog:10 > + cross-platform parts of RemoteInspector have been moced to a new RemoteInspector.cpp file. Also moved to cocoa moved
I'm going to update this patch and land it.
Committed r212169: <http://trac.webkit.org/changeset/212169>
Thanks Brian!
(In reply to comment #18) > Committed r212169: <http://trac.webkit.org/changeset/212169> Mac CMake buildfix landed in https://trac.webkit.org/changeset/212363
I'm seeing suspicious crashes on my phone when starting the Web Inspector on iOS: Thread 0 name: Dispatch queue: com.apple.main-thread Thread 0 Crashed: 0 JavaScriptCore 0x00000001016b86a8 Inspector::InspectorValue::parseJSON(WTF::String const&, WTF::RefPtr<Inspector::InspectorValue>&) + 40 1 JavaScriptCore 0x000000010166c414 Inspector::BackendDispatcher::dispatch(WTF::String const&) + 104 2 JavaScriptCore 0x000000010166c414 Inspector::BackendDispatcher::dispatch(WTF::String const&) + 104 3 JavaScriptCore 0x0000000101956278 ___ZN9Inspector24RemoteConnectionToTarget19sendMessageToTargetERKN3WTF6StringE_block_invoke + 128 4 JavaScriptCore 0x0000000101956634 Inspector::RemoteTargetHandleRunSourceGlobal(void*) + 220 5 CoreFoundation 0x000000018fe4e3c8 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24 6 CoreFoundation 0x000000018fe4dd38 __CFRunLoopDoSources0 + 540 7 CoreFoundation 0x000000018fe4b944 __CFRunLoopRun + 744 8 CoreFoundation 0x000000018fd7b9bc CFRunLoopRunSpecific + 424 9 Foundation 0x0000000190897884 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 304 10 Foundation 0x00000001908ec1d8 -[NSRunLoop(NSRunLoop) run] + 88 11 libxpc.dylib 0x000000018ef9fc54 _xpc_objc_main + 664 12 libxpc.dylib 0x000000018efa1984 xpc_main + 200 13 com.apple.WebKit.WebContent 0x00000001000e75b8 main + 376 14 libdyld.dylib 0x000000018ed8959c start + 4
I verified that the WebInspector crashed immediately when starting to inspect a page 100% of the time and I reverted this in http://trac.webkit.org/r212394 and the WebInspector works fine on iOS. Brian, please check that the WebInspector works on iOS before re-landing this.
(In reply to comment #22) > I verified that the WebInspector crashed immediately when starting to > inspect a page 100% of the time and I reverted this in > http://trac.webkit.org/r212394 and the WebInspector works fine on iOS. > Brian, please check that the WebInspector works on iOS before re-landing > this. I would have appreciated a ping at the least. It is probably something trivial, and now I'll need to waste some build time on trunk to fix someone else's code.
I'll re-check the patch, it shouldn't change anything actually, it's just a refactoring. Let me know if I can help somehow, I can't test on iOS, though.
Carlos, I'm 100% sure if you re-land the patch as-is it will make the iOS web inspector crash on launch 100% of the time.
(In reply to comment #25) > Carlos, I'm 100% sure if you re-land the patch as-is it will make the iOS > web inspector crash on launch 100% of the time. Sure, that's why we need to investigate :-) I must have messed anything in the refactoring.
(In reply to comment #26) > (In reply to comment #25) > > Carlos, I'm 100% sure if you re-land the patch as-is it will make the iOS > > web inspector crash on launch 100% of the time. > > Sure, that's why we need to investigate :-) I must have messed anything in > the refactoring. There was at least one change to RemoteInspector committed to trunk after this patch was last rebased. I had to manually merge in one of them, and must have missed another one. Please compare the pre- and post- of my commit, not the one up on bugzilla.
Comment on attachment 298557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298557&action=review > Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mmSource/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mm:213 > -void RemoteConnectionToTarget::sendMessageToTarget(NSString *message) > +void RemoteConnectionToTarget::sendMessageToTarget(const String& message) This change broke it. I'm going to re-commit it with an NSString.
Created attachment 301717 [details] Patch
Re-committed latest patch with fixed API in http://trac.webkit.org/r212424
(In reply to comment #28) > Comment on attachment 298557 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298557&action=review > > > Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mmSource/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mm:213 > > -void RemoteConnectionToTarget::sendMessageToTarget(NSString *message) > > +void RemoteConnectionToTarget::sendMessageToTarget(const String& message) > > This change broke it. I'm going to re-commit it with an NSString. Thanks Alex! Then we need ifdefs, no?. I assumed there was an implicit conversion, because it built.
I put ifdefs. We could maybe do something like PlatformString, but that definitely needs to stay an NSString for some reason.
Comment on attachment 298557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298557&action=review >>> Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mmSource/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mm:213 >>> +void RemoteConnectionToTarget::sendMessageToTarget(const String& message) >> >> This change broke it. I'm going to re-commit it with an NSString. > > Thanks Alex! Then we need ifdefs, no?. I assumed there was an implicit conversion, because it built. Yes, this would cause issues with WTFString because the String is being transported across multiple threads here. Here it is on RemoteInspector's XPC Connection Queue and dispatchAsyncOnTarget below will use the string on the Target's Debugger Queue (typically the Main Queue / Web Thread for WebViews). Using WTFString I think it would destroy the data by the time the other thread uses it. In the NSString case it gets retained for use in the block and released afterwards. The typical pattern for using WTFString across threads would be an isolatedCopy and capturing in a lambda. It seems that would be rather wasteful here though (the data is not going to change).
<rdar://problem/30610701>