[webkit-reviews] review granted: [Bug 108015] Refactor XPCService initialization to make it easier to add more services : [Attachment 184887] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 108015: Refactor XPCService initialization to make it easier to add more
services
https://bugs.webkit.org/show_bug.cgi?id=108015

Attachment 184887: Patch
https://bugs.webkit.org/attachment.cgi?id=184887&action=review

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
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.