Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 132007: Linux: plumb fontconfig call out to the sandbox host. (Closed)

Created:
10 years, 4 months ago by agl
Modified:
8 years, 5 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : ... #

Patch Set 3 : ... #

Total comments: 1

Patch Set 4 : Change to use SandboxSupport #

Total comments: 19

Patch Set 5 : refactor #

Patch Set 6 : Forgot to change a define to enable sandboxSupport in the renderer #

Patch Set 7 : Probably screwed up that last upload #

Patch Set 8 : forgot files #

Total comments: 9

Patch Set 9 : ... #

Unified diffs Side-by-side diffs Delta from patch set Stats (+383 lines, -3 lines) Patch
M base/message_pump_glib.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_sandbox_host_linux.cc View 1 2 3 4 5 6 7 8 3 chunks +46 lines, -0 lines 0 comments Download
M chrome/chrome.gyp View 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/common/sandbox_methods_linux.h View 1 1 chunk +19 lines, -0 lines 0 comments Download
A chrome/renderer/renderer_sandbox_support_linux.h View 5 6 8 1 chunk +25 lines, -0 lines 0 comments Download
A chrome/renderer/renderer_sandbox_support_linux.cc View 5 6 8 1 chunk +35 lines, -0 lines 0 comments Download
M chrome/renderer/renderer_webkitclient_impl.h View 1 2 3 4 5 6 7 8 2 chunks +20 lines, -1 line 0 comments Download
M chrome/renderer/renderer_webkitclient_impl.cc View 1 2 3 4 5 6 7 8 3 chunks +24 lines, -1 line 0 comments Download
A webkit/api/public/gtk/WebFontInfo.h View 5 6 8 1 chunk +52 lines, -0 lines 0 comments Download
A webkit/api/public/linux/WebSandboxSupport.h View 5 6 8 1 chunk +56 lines, -0 lines 0 comments Download
M webkit/api/src/ChromiumBridge.cpp View 1 2 3 4 5 6 7 8 3 chunks +16 lines, -1 line 0 comments Download
A webkit/api/src/gtk/WebFontInfo.cpp View 5 6 8 1 chunk +83 lines, -0 lines 0 comments Download
M webkit/webkit.gyp View 4 5 6 7 8 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
agl
There's a WebKit side to this change which is yet to land, but it's simply ...
10 years, 4 months ago (2009-06-18 02:32:52 UTC) #1
darin (slow to review)
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 ...
10 years, 4 months ago (2009-06-18 06:28:56 UTC) #2
agl
On 2009/06/18 06:28:56, darin wrote: > i think you should define WebSandboxSupport for Linux instead ...
10 years, 4 months ago (2009-06-18 17:31:17 UTC) #3
darin (slow to review)
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) ...
10 years, 4 months ago (2009-06-18 18:56:22 UTC) #4
darin (slow to review)
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: ...
10 years, 4 months ago (2009-06-18 19:22:53 UTC) #5
agl
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, ...
10 years, 4 months ago (2009-06-18 21:22:10 UTC) #6
darin (slow to review)
All sounds good to me. Unfortunately, it looks like this CL is missing some files, ...
10 years, 4 months ago (2009-06-19 17:36:12 UTC) #7
agl
Blast, sorry. Missing files now included I hope.
10 years, 4 months ago (2009-06-19 18:58:18 UTC) #8
darin (slow to review)
10 years, 4 months ago (2009-06-22 16:55:35 UTC) #9
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

Powered by Google App Engine
This is Rietveld 408576698