Bug 108015 - Refactor XPCService initialization to make it easier to add more services
Summary: Refactor XPCService initialization to make it easier to add more services
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-26 14:58 PST by Sam Weinig
Modified: 2013-01-26 16:41 PST (History)
1 user (show)

See Also:


Attachments
Patch (43.88 KB, patch)
2013-01-26 15:02 PST, Sam Weinig
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2013-01-26 14:58:55 PST
Refactor XPCService initialization to make it easier to add more services
Comment 1 Sam Weinig 2013-01-26 15:02:58 PST
Created attachment 184887 [details]
Patch
Comment 2 mitz 2013-01-26 16:20:25 PST
Comment on attachment 184887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184887&action=review

r=me if you fix the macro name, but feel free to address other comments.

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceBootstrapper.Development.h:30
> +#error XPC_SERVICE_INITIALIZER must be defined.

The XPC_ prefix for use by XPC. Please prefix this with something WebKit-specific.

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceBootstrapper.Development.h:63
> +                // Setup the posix_spawn attributes.

s/Setup/Set up/. What is this comment for anyway?

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceBootstrapper.Development.h:83
> +                // Set the architecture.
> +                cpu_type_t cpuTypes[] = { (cpu_type_t)xpc_dictionary_get_uint64(event, "architecture") };
> +                size_t outCount = 0;
> +                posix_spawnattr_setbinpref_np(&attr, 1, cpuTypes, &outCount);

Why is this stuck in the middle of the section that sets the flags?

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceBootstrapper.Development.h:95
> +                // Setup the command line.

s/Setup/Set up/. What is this comment for anyway?

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceBootstrapper.Development.h:100
> +                // Setup the environment.

Ditto.

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceBootstrapper.Development.h:113
> +                    size_t stringLength = strlen(string);
> +
> +                    char* environmentVariable = (char*)malloc(stringLength + 1);
> +                    memcpy(environmentVariable, string, stringLength);
> +                    environmentVariable[stringLength] = '\0';
> +
> +                    environment[i] = environmentVariable;

Isn’t this equivalent to:
    environment[i] = strdup(environmentVariable)
? Why is this required and a simple environment[i] = environmentVariable won’t do? Are the pointers in the environment parameter required to stay valid after the call to posix_spawn?

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceBootstrapper.Development.h:122
> +                NSLog(@"Unable to re-exec for path: %s\n", path);

No need for \n in NSLog.

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceBootstrapper.Development.h:129
> +                    NSLog(@"Unable to load WebKit2.framework: %s\n", dlerror());

Might be useful to log the path here.

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceBootstrapper.Development.h:136
> +                    NSLog(@"Unable to find entry point in WebKit2.framework: %s\n", dlerror());

Might be useful to log the name here.
Comment 3 Sam Weinig 2013-01-26 16:41:37 PST
Committed r140919: <http://trac.webkit.org/changeset/140919>