Linux: plumb fontconfig call out to the sandbox host.

Linux: plumb fontconfig call out to the sandbox host.

This is hopefully the last step before our renderers can run cleanly
in a chroot.

WebKit needs to be able to ask for the correct font to use in the case
that the current font doesn't include glyphs for certain code points.
Currently we make a fontconfig call in our WebKit port to handle this.

This patch changes this so that the call is sent our via
ChromiumBridge. Since we are at ChromiumBridge, we could make a sync
IPC to the browser. However, fontconfig is a single threaded library
and we are already using it on the UI thread in the browser, so the
sync IPC would have to terminate on the UI thread. Even if this
doesn't deadlock, it causes huge spikes in latency.

So, instead, we send the IPC to the sandbox host process which is
already setup to handle fontconfig requests from Skia. See:
  http://code.google.com/p/chromium/wiki/LinuxSandboxIPC

[agl, 2009-06-18 02:32:52]

There's a WebKit side to this change which is yet to land, but it's simply
replacing a function with a call to ChromiumBridge.

[darin (slow to review), 2009-06-18 06:28:56]

http://codereview.chromium.org/132007/diff/21/1014
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/132007/diff/21/1014#newcode137
Line 137: #if defined(__linux__)
i think you should define WebSandboxSupport for Linux instead of #ifdef'ing
here.  each platform gets to define their own WebSandboxSupport interface.

[agl, 2009-06-18 17:31:17]

On 2009/06/18 06:28:56, darin wrote:
> i think you should define WebSandboxSupport for Linux instead of #ifdef'ing
> here.  each platform gets to define their own WebSandboxSupport interface.

Done.

(p.s. I note that in ChromiumBridge.cpp we are using both PLATFORM(...) and
defined(OS_...) macros. I wasn't sure which was best so I just replicated what
was already there.

Cheers

[darin (slow to review), 2009-06-18 18:56:22]

only reviewed part of the CL...

http://codereview.chromium.org/132007/diff/25/30
File webkit/api/src/ChromiumBridge.cpp (right):

http://codereview.chromium.org/132007/diff/25/30#newcode152
Line 152: #if defined(OS_WIN)
ugh.. this is a bug.  it is carryover from when this code lived in
webkit/glue/chromium_bridge_impl.cc.  we need to replace it with #if
PLATFORM(WIN_OS).  i wonder how build_config.h is getting included.  maybe via
googleurl...

[darin (slow to review), 2009-06-18 19:22:53]

Looks like WebSandboxSupport.h is missing from this CL.

http://codereview.chromium.org/132007/diff/25/26
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/132007/diff/25/26#newcode180
Line 180: return;
note, in Chrome IPC, when we get an unexpected error parsing an incoming IPC, we
terminate the child process.  it seems like it would be good to arrange the
infrastructure here so that the same is easy to do here.

http://codereview.chromium.org/132007/diff/25/28
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/132007/diff/25/28#newcode22
Line 22: #include "webkit/api/public/WebCommon.h"
WebCommon.h is already included by WebString.h and WebURL.h

http://codereview.chromium.org/132007/diff/25/28#newcode153
Line 153: const std::string key = std::string(reinterpret_cast<const
char*>(characters),
no need to copy the string again... you can just do this:

  const std::string key(
      reinterpret_cast<const char*>(characters),
      num_characters * sizeof(characters[0]));

http://codereview.chromium.org/132007/diff/25/28#newcode164
Line 164: request.WriteUInt32(characters[i]);
nit: 2 space indent

http://codereview.chromium.org/132007/diff/25/28#newcode167
Line 167: static const int kMagicSandboxFD = 4;
there's a lot of details here.... it seems like it would be nice to move this
code for doing linux sandbox IPC into a helper function or class.  that way it
can be documented there, and this code can just do something much simpler
looking.  you can probably reuse some of the same Tuple stuff we use for normal
Chrome IPC to hide the details.  or if this is the only case of sandbox IPC,
then just creating a helper function for this IPC in a separate file seems fine.

http://codereview.chromium.org/132007/diff/25/29
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/132007/diff/25/29#newcode58
Line 58: const WebKit::WebUChar* characters, int numCharacters);
num_characters

http://codereview.chromium.org/132007/diff/25/29#newcode66
Line 66: std::map<std::string, std::string> unicode_font_families_;
nit: typedef std::map<...> FontFamiliesMap ?  this way you don't have to spell
out the template type when you want to declare an iterator.

http://codereview.chromium.org/132007/diff/25/31
File webkit/tools/test_shell/test_shell_gtk.cc (right):

http://codereview.chromium.org/132007/diff/25/31#newcode666
Line 666: FcCharSet* cset = FcCharSetCreate();
this looks a lot like the code in render_sandbox_host_linux.cc.  shouldn't it be
factored out into a helper function that both can use?

http://codereview.chromium.org/132007/diff/25/31#newcode713
Line 713: WebKit::WebSandboxSupport* TestShellWebKitInit::sandboxSupport() {
kind of odd to be implementing TestShellWebKitInit in this file.  how about
test_shell_webkit_init_linux?

also, it seems a bit unfortunate that we need sandbox support in TestShell when
there is no sandbox.  maybe it would make sense for this chunk of code to be
part of the WebKit API.  if WebKitClient has no WebSandboxSupport, then
ChromiumBridge.cpp could just call this code directly.  otherwise, it would call
into the WebSandboxSupport, which in Chrome would IPC over to the browser, where
we would then call the WebKit API to do the work.

[agl, 2009-06-18 21:22:10]

http://codereview.chromium.org/132007/diff/25/26
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/132007/diff/25/26#newcode180
Line 180: return;
On 2009/06/18 19:22:54, darin wrote:
> note, in Chrome IPC, when we get an unexpected error parsing an incoming IPC,
we
> terminate the child process.  it seems like it would be good to arrange the
> infrastructure here so that the same is easy to do here.

It's a useful desiderata, but it's tough to do. We need a new channel from the
sandbox helper process to the browser and then the browser needs to send a
message to the zygote, who can actually kill the process.

http://codereview.chromium.org/132007/diff/25/28
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/132007/diff/25/28#newcode22
Line 22: #include "webkit/api/public/WebCommon.h"
On 2009/06/18 19:22:54, darin wrote:
> WebCommon.h is already included by WebString.h and WebURL.h

Done.

http://codereview.chromium.org/132007/diff/25/28#newcode153
Line 153: const std::string key = std::string(reinterpret_cast<const
char*>(characters),
On 2009/06/18 19:22:54, darin wrote:
> no need to copy the string again... you can just do this:
> 
>   const std::string key(
>       reinterpret_cast<const char*>(characters),
>       num_characters * sizeof(characters[0]));

I'm pretty sure that C++ compilers can optimise that away, but I've changed it
anyway since it's less code.

http://codereview.chromium.org/132007/diff/25/28#newcode167
Line 167: static const int kMagicSandboxFD = 4;
done. created renderer_sandbox_support_linux.(cc|h)

http://codereview.chromium.org/132007/diff/25/29
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/132007/diff/25/29#newcode58
Line 58: const WebKit::WebUChar* characters, int numCharacters);
On 2009/06/18 19:22:54, darin wrote:
> num_characters

Code deleted.

http://codereview.chromium.org/132007/diff/25/29#newcode66
Line 66: std::map<std::string, std::string> unicode_font_families_;
On 2009/06/18 19:22:54, darin wrote:
> nit: typedef std::map<...> FontFamiliesMap ?  this way you don't have to spell
> out the template type when you want to declare an iterator.

Unless you feel strongly, I'd like to leave it. I don't like such typedefs, they
just obfuscate the types and mean that I have to go check a header file to
figure out what code is doing.

http://codereview.chromium.org/132007/diff/25/30
File webkit/api/src/ChromiumBridge.cpp (right):

http://codereview.chromium.org/132007/diff/25/30#newcode152
Line 152: #if defined(OS_WIN)
On 2009/06/18 18:56:22, darin wrote:
> ugh.. this is a bug.  it is carryover from when this code lived in
> webkit/glue/chromium_bridge_impl.cc.  we need to replace it with #if
> PLATFORM(WIN_OS).  i wonder how build_config.h is getting included.  maybe via
> googleurl...

Done.

http://codereview.chromium.org/132007/diff/25/31
File webkit/tools/test_shell/test_shell_gtk.cc (right):

http://codereview.chromium.org/132007/diff/25/31#newcode666
Line 666: FcCharSet* cset = FcCharSetCreate();
On 2009/06/18 19:22:54, darin wrote:
> this looks a lot like the code in render_sandbox_host_linux.cc.  shouldn't it
be
> factored out into a helper function that both can use?

Code ended up in the WebKit API.

http://codereview.chromium.org/132007/diff/25/31#newcode713
Line 713: WebKit::WebSandboxSupport* TestShellWebKitInit::sandboxSupport() {
>, then
> ChromiumBridge.cpp could just call this code directly.  otherwise, it would
call
> into the WebSandboxSupport, which in Chrome would IPC over to the browser,
where
> we would then call the WebKit API to do the work.

Refactored as such.

[darin (slow to review), 2009-06-19 17:36:12]

All sounds good to me.  Unfortunately, it looks like this CL is missing some
files, so I can't complete the review.

[agl, 2009-06-19 18:58:18]

Blast, sorry. Missing files now included I hope.

[darin (slow to review), 2009-06-22 16:55:35]

looks good.  just nits:

http://codereview.chromium.org/132007/diff/3001/4001
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/132007/diff/3001/4001#newcode190
Line 190: WebKit::WebUChar* chars = new WebKit::WebUChar[num_chars];
scoped_array<WebUChar>

please add using directives up top for WebUChar, WebString, and WebFontInfo.  we
are doing that consistently elsewhere, and i'd really like to stick to it.

http://codereview.chromium.org/132007/diff/3001/4008
File webkit/api/public/gtk/WebFontInfo.h (right):

http://codereview.chromium.org/132007/diff/3001/4008#newcode40
Line 40: WEBKIT_API static WebString familyForChars(const WebUChar* characters,
int numCharacters);
Might want to add a comment that explains what this function does?  Does it
return a null WebString if it could not find a matching family, or does it
return a default font family, etc.?

http://codereview.chromium.org/132007/diff/3001/4009
File webkit/api/public/linux/WebSandboxSupport.h (right):

http://codereview.chromium.org/132007/diff/3001/4009#newcode51
Line 51: virtual WebString getFontFamilyForCharacters(const WebUChar*
characters, int numCharacters) = 0;
nit: in the webkit api, we're using size_t for character counts instead of int.

http://codereview.chromium.org/132007/diff/3001/4010
File webkit/api/src/ChromiumBridge.cpp (right):

http://codereview.chromium.org/132007/diff/3001/4010#newcode164
Line 164: return
webKitClient()->sandboxSupport()->getFontFamilyForCharacters(reinterpret_cast<const
WebUChar*>(characters), numCharacters);
nit: 4 white space indentation

nit: i think the reinterpret_cast should be unnecessary.

http://codereview.chromium.org/132007/diff/3001/4011
File webkit/api/src/gtk/WebFontInfo.cpp (right):

http://codereview.chromium.org/132007/diff/3001/4011#newcode31
Line 31: #include <string.h>
nit: this header should move down.  the first header needs to be config.h
followed by WebFontInfo.h to match webkit style.

http://codereview.chromium.org/132007/diff/3001/4011#newcode47
Line 47: && U16_IS_SURROGATE_LEAD(characters[i])
nit: indentation is 4 white spaces in webkit style

http://codereview.chromium.org/132007/diff/3001/4011#newcode74
Line 74: const char* char_family = reinterpret_cast<char*>(family);
nit: charFamily

http://codereview.chromium.org/132007/diff/3001/4011#newcode84
Line 84: }  // namespace WebKit
nit: webkit style is to only have one space before //

http://codereview.chromium.org/132007/diff/3001/4012
File webkit/webkit.gyp (right):

http://codereview.chromium.org/132007/diff/3001/4012#newcode4231
Line 4231: 'api/src/gtk/WebFontInfo.cpp',
need to also add the header file