Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inject InspectorFrontendHost in the Inspector frontend and communicate over raw sockets #317

Merged
merged 4 commits into from
Sep 18, 2015

Conversation

KristinaKoeva
Copy link
Contributor

@KristinaKoeva KristinaKoeva added this to the 1.4.0 milestone Sep 16, 2015
@ns-bot
Copy link

ns-bot commented Sep 16, 2015

Test PASSed.

struct communication_channel {
BOOL connected;
dispatch_fd_t socket;
__unsafe_unretained dispatch_io_t io_channel;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use a class instead of a struct so that ARC can manage the class instance and the dispatch_io_t object within.

@fealebenpae
Copy link
Contributor

For C functions prefer prefixed PascalCased names, like in CoreFoundation, or TNSDebugging. Apply the t suffix to C types and typedefs, such as communication_channel_t and dispatch_io_t.

@ns-bot
Copy link

ns-bot commented Sep 17, 2015

Test PASSed.


CommunicationChannel* TNSSetupCommunicationChannel(char* socket_path, InspectorReadHandler read_handler, InspectorErrorHandler error_handler) {
__block dispatch_fd_t _socket;
__block dispatch_io_t _channel;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming these to something more descriptive.

@fealebenpae fealebenpae changed the title Inject InspectorFrontendHost so that we could stop using web sockets to communicate with the inspector Inject InspectorFrontendHost in the Inspector frontend and communicate over raw sockets Sep 17, 2015
@ns-bot
Copy link

ns-bot commented Sep 17, 2015

Test PASSed.

@fealebenpae
Copy link
Contributor

I think the functions in Communication.m would work better as methods of the CommunicationChannel class since they all need it as a parameter.

@ns-bot
Copy link

ns-bot commented Sep 17, 2015

Test PASSed.

@property(nonatomic) dispatch_fd_t socket;
@property(nonatomic, strong) dispatch_io_t ioChannel;

- (id)initWithSocketPath:(NSString*)socketPath readHandler:(InspectorReadHandler)readHandler errorHandler:(InspectorErrorHandler)errorHandler;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be instancetype instead of id.

@ns-bot
Copy link

ns-bot commented Sep 17, 2015

Test PASSed.

@fealebenpae
Copy link
Contributor

Looks good to me. Please squash this and be sure to update the Inspector binary blob in the repo with the new build so we can merge it.

@ns-bot
Copy link

ns-bot commented Sep 17, 2015

Test PASSed.

@KristinaKoeva KristinaKoeva force-pushed the KristinaKoeva/InspectorCommunication branch from bd60742 to c180115 Compare September 18, 2015 08:19
@ns-bot
Copy link

ns-bot commented Sep 18, 2015

Test PASSed.

fealebenpae added a commit that referenced this pull request Sep 18, 2015
…unication

Inject InspectorFrontendHost in the Inspector frontend and communicate over raw sockets
@fealebenpae fealebenpae merged commit ba6281c into master Sep 18, 2015
@fealebenpae fealebenpae removed the bug label Sep 18, 2015
@fealebenpae fealebenpae deleted the KristinaKoeva/InspectorCommunication branch September 30, 2015 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants