Bug 166681 - WebInspector: refactor RemoteInspector to move cocoa specific code to their own files
Summary: WebInspector: refactor RemoteInspector to move cocoa specific code to their o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks: 166680
  Show dependency treegraph
 
Reported: 2017-01-04 04:17 PST by Carlos Garcia Campos
Modified: 2017-02-22 10:02 PST (History)
7 users (show)

See Also:


Attachments
WIP patch (44.35 KB, patch)
2017-01-04 04:55 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP patch (27.16 KB, patch)
2017-01-04 05:25 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP: trying xcode changes (49.27 KB, patch)
2017-01-10 06:55 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP: try to fix the mac builds and some style issues (48.25 KB, patch)
2017-01-10 08:47 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP: Try to fix mac builds (49.01 KB, patch)
2017-01-10 08:58 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP: Try to fix mac builds (49.04 KB, patch)
2017-01-10 09:27 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP: Trying to fix mac builds (49.05 KB, patch)
2017-01-10 09:33 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (53.96 KB, patch)
2017-01-10 23:53 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (84.84 KB, patch)
2017-02-15 23:41 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-01-04 04:17:57 PST
Most of the RemoteInspector implementation is actually cross-platform, but RemoteInspector.mm and RemoteConnectionToTarget.mm need to be factored out.
Comment 1 Carlos Garcia Campos 2017-01-04 04:55:11 PST
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.
Comment 2 Carlos Garcia Campos 2017-01-04 05:25:32 PST
Created attachment 298008 [details]
WIP patch
Comment 3 Carlos Garcia Campos 2017-01-10 06:55:19 PST
Created attachment 298462 [details]
WIP: trying xcode changes
Comment 4 WebKit Commit Bot 2017-01-10 06:57:49 PST
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.
Comment 5 Carlos Garcia Campos 2017-01-10 08:47:14 PST
Created attachment 298469 [details]
WIP: try to fix the mac builds and some style issues
Comment 6 WebKit Commit Bot 2017-01-10 08:48:41 PST
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.
Comment 7 Carlos Garcia Campos 2017-01-10 08:58:54 PST
Created attachment 298470 [details]
WIP: Try to fix mac builds
Comment 8 WebKit Commit Bot 2017-01-10 09:01:44 PST
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.
Comment 9 Carlos Garcia Campos 2017-01-10 09:27:39 PST
Created attachment 298472 [details]
WIP: Try to fix mac builds
Comment 10 WebKit Commit Bot 2017-01-10 09:29:01 PST
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.
Comment 11 Carlos Garcia Campos 2017-01-10 09:33:41 PST
Created attachment 298474 [details]
WIP: Trying to fix mac builds
Comment 12 WebKit Commit Bot 2017-01-10 09:38:43 PST
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.
Comment 13 Carlos Garcia Campos 2017-01-10 23:53:08 PST
Created attachment 298557 [details]
Patch
Comment 14 WebKit Commit Bot 2017-01-10 23:54:47 PST
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.
Comment 15 Carlos Garcia Campos 2017-01-16 08:03:24 PST
Ping reviewers. This is just a refactoring.
Comment 16 Michael Catanzaro 2017-02-03 11:35:18 PST
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
Comment 17 Brian Burg 2017-02-10 15:36:15 PST
I'm going to update this patch and land it.
Comment 18 Brian Burg 2017-02-10 17:29:01 PST
Committed r212169: <http://trac.webkit.org/changeset/212169>
Comment 19 Carlos Garcia Campos 2017-02-10 23:25:52 PST
Thanks Brian!
Comment 20 Csaba Osztrogonác 2017-02-15 09:34:00 PST
(In reply to comment #18)
> Committed r212169: <http://trac.webkit.org/changeset/212169>

Mac CMake buildfix landed in https://trac.webkit.org/changeset/212363
Comment 21 Alex Christensen 2017-02-15 13:36:26 PST
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
Comment 22 Alex Christensen 2017-02-15 14:35:51 PST
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.
Comment 23 Brian Burg 2017-02-15 17:07:15 PST
(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.
Comment 24 Carlos Garcia Campos 2017-02-15 22:27:05 PST
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.
Comment 25 Alex Christensen 2017-02-15 22:49:50 PST
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.
Comment 26 Carlos Garcia Campos 2017-02-15 23:10:33 PST
(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.
Comment 27 Brian Burg 2017-02-15 23:12:27 PST
(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 28 Alex Christensen 2017-02-15 23:39:26 PST
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.
Comment 29 Alex Christensen 2017-02-15 23:41:44 PST
Created attachment 301717 [details]
Patch
Comment 30 Alex Christensen 2017-02-15 23:42:39 PST
Re-committed latest patch with fixed API in http://trac.webkit.org/r212424
Comment 31 Carlos Garcia Campos 2017-02-15 23:43:53 PST
(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.
Comment 32 Alex Christensen 2017-02-15 23:45:24 PST
I put ifdefs.  We could maybe do something like PlatformString, but that definitely needs to stay an NSString for some reason.
Comment 33 Joseph Pecoraro 2017-02-16 15:47:39 PST
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).
Comment 34 Brian Burg 2017-02-22 10:02:11 PST
<rdar://problem/30610701>