Bug 213379

Created attachment 402313 [details]
Patch

Comment on attachment 402313 [details]
Patch

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

> Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:96
> +    StringBuilder path;
> +    path.append([webKit2Bundle resourcePath]);
> +    path.append("/");
> +    path.append("com.apple.WebKit.NetworkProcess.sb");
> +    sandboxParameters.setOverrideSandboxProfilePath(path.toString());

This is a job for makeString, not StringBuilder.

    sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle resourcePath], "/com.apple.WebKit.NetworkProcess.sb"));

Or use a local String variable if you like.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:229
> +    // We don't need to talk to the dock.

Capitalize Dock.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> +            [nsApplicationClass _preventDockConnections];
> +    }

I know this code is pre-existing and only being moved, but this does seem risky — if this method is renamed, moved, or removed this code will just silently stop having any effect.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:589
> +    StringBuilder path;
> +    path.append([webKit2Bundle resourcePath]);
> +    path.append("/");
> +    path.append("com.apple.WebProcess.sb");
> +    sandboxParameters.setOverrideSandboxProfilePath(path.toString());

This is a job for makeString, not StringBuilder.

    sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle resourcePath], "/com.apple.WebKit.WebProcess.sb"));

Or use a local String variable if you like.

Created attachment 402329 [details]
Patch

(In reply to Darin Adler from comment #2)
> Comment on attachment 402313 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402313&action=review
> 
> > Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:96
> > +    StringBuilder path;
> > +    path.append([webKit2Bundle resourcePath]);
> > +    path.append("/");
> > +    path.append("com.apple.WebKit.NetworkProcess.sb");
> > +    sandboxParameters.setOverrideSandboxProfilePath(path.toString());
> 
> This is a job for makeString, not StringBuilder.
> 
>    
> sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle
> resourcePath], "/com.apple.WebKit.NetworkProcess.sb"));
> 
> Or use a local String variable if you like.
> 

Fixed.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:229
> > +    // We don't need to talk to the dock.
> 
> Capitalize Dock.
> 

Done.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> > +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> > +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> > +            [nsApplicationClass _preventDockConnections];
> > +    }
> 
> I know this code is pre-existing and only being moved, but this does seem
> risky — if this method is renamed, moved, or removed this code will just
> silently stop having any effect.
> 

I could be wrong, but it seems this method is supported.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:589
> > +    StringBuilder path;
> > +    path.append([webKit2Bundle resourcePath]);
> > +    path.append("/");
> > +    path.append("com.apple.WebProcess.sb");
> > +    sandboxParameters.setOverrideSandboxProfilePath(path.toString());
> 
> This is a job for makeString, not StringBuilder.
> 
>    
> sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle
> resourcePath], "/com.apple.WebKit.WebProcess.sb"));
> 
> Or use a local String variable if you like.

Fixed.

Thanks for reviewing!

Comment on attachment 402329 [details]
Patch

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

> Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:92
> +    sandboxParameters.setOverrideSandboxProfilePath(makeString(String([webKitBundle resourcePath]), "/com.apple.WebKit.NetworkProcess.sb"));

You should not need String() around [webKitBundle resourcePath]. If you do, that’s a bug in StringConcatenation.h implementation that we should fix.

(In reply to Per Arne Vollan from comment #4)
> (In reply to Darin Adler from comment #2)
> > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> > > +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> > > +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> > > +            [nsApplicationClass _preventDockConnections];
> > > +    }
> > 
> > I know this code is pre-existing and only being moved, but this does seem
> > risky — if this method is renamed, moved, or removed this code will just
> > silently stop having any effect.
> 
> I could be wrong, but it seems this method is supported.

Why then are we calling it in such an unusual way?

(In reply to Darin Adler from comment #6)
> (In reply to Per Arne Vollan from comment #4)
> > (In reply to Darin Adler from comment #2)
> > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> > > > +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> > > > +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> > > > +            [nsApplicationClass _preventDockConnections];
> > > > +    }
> > > 
> > > I know this code is pre-existing and only being moved, but this does seem
> > > risky — if this method is renamed, moved, or removed this code will just
> > > silently stop having any effect.
> > 
> > I could be wrong, but it seems this method is supported.
> 
> Why then are we calling it in such an unusual way?

That is a good point! I wonder if it is done this way for it to compile on OS's where this method is not supported?

Created attachment 402341 [details]
Patch

(In reply to Per Arne Vollan from comment #7)
> (In reply to Darin Adler from comment #6)
> > (In reply to Per Arne Vollan from comment #4)
> > > (In reply to Darin Adler from comment #2)
> > > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> > > > > +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> > > > > +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> > > > > +            [nsApplicationClass _preventDockConnections];
> > > > > +    }
> > > > 
> > > > I know this code is pre-existing and only being moved, but this does seem
> > > > risky — if this method is renamed, moved, or removed this code will just
> > > > silently stop having any effect.
> > > 
> > > I could be wrong, but it seems this method is supported.
> > 
> > Why then are we calling it in such an unusual way?
> 
> That is a good point! I wonder if it is done this way for it to compile on
> OS's where this method is not supported?

Trying out a patch without using respondsToSelector.

Thanks for reviewing!

Committed r263295: <https://trac.webkit.org/changeset/263295>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402341 [details].

<rdar://problem/64549175>

This broke setting log channels via -WebCoreLogging

Re-opened since this is blocked by bug 213577

Created attachment 403234 [details]
Patch

Committed r263773: <https://trac.webkit.org/changeset/263773>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403234 [details].