Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8566026:
Implement skia sandbox callback (Closed)
|
Created:
9 years, 1 month ago by arthurhsu Modified:
9 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, brettw-cc_chromium.org, bungeman-skia Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement skia sandbox callback
BUG=103032
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111400
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use bungeman's change on Skia #
Total comments: 1
Patch Set 3 : Rebase trunk #Patch Set 4 : Rebase and Update per code review #
Total comments: 2
Patch Set 5 : Update per code review #Patch Set 6 : Fix errors in checkin #
Total comments: 28
Patch Set 7 : Update per code review #
Total comments: 10
Patch Set 8 : Update per code review #
Total comments: 18
Patch Set 9 : Update per code review #
Total comments: 6
Patch Set 10 : Update per code review #
Total comments: 2
Messages
Total messages: 22 (0 generated)
http://codereview.chromium.org/8566026/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/8566026/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.cc:263: class PrintWebViewHelperSkSandbox : public skia::SkSandbox { This is the wrong place for this. It should go in skia/ext/
try job sent for win,linux,mac,win-layout,linux-layout
I'm building a CL to just add the no-op callback at first to make the roll happen, then we can figure out where this stuff should all go. I happen to notice chrome_render_process_observer.cc:163 (calls ReleaseCahcedFonts). I wonder if this is part of the problem. http://codereview.chromium.org/8566026/diff/4001/chrome/renderer/print_web_vi... File chrome/renderer/print_web_view_helper.h (right): http://codereview.chromium.org/8566026/diff/4001/chrome/renderer/print_web_vi... chrome/renderer/print_web_view_helper.h:91: public skia::ISandboxSupport, This seems like the wrong place for this, it's not printing specific... maybe ChromeRenderProcessObserver::ChromeRenderProcessObserver... then again, maybe it needs to be inside the "content" abstraction too.
On 2011/11/16 05:42:55, vandebo wrote: > I'm building a CL to just add the no-op callback at first to make the roll > happen, then we can figure out where this stuff should all go. > > I happen to notice chrome_render_process_observer.cc:163 (calls > ReleaseCahcedFonts). I wonder if this is part of the problem. > > http://codereview.chromium.org/8566026/diff/4001/chrome/renderer/print_web_vi... > File chrome/renderer/print_web_view_helper.h (right): > > http://codereview.chromium.org/8566026/diff/4001/chrome/renderer/print_web_vi... > chrome/renderer/print_web_view_helper.h:91: public skia::ISandboxSupport, > This seems like the wrong place for this, it's not printing specific... maybe > ChromeRenderProcessObserver::ChromeRenderProcessObserver... then again, maybe it > needs to be inside the "content" abstraction too. It turns out that I can just acquire the active render thread and ask for PreCacheFont. I put them in separate files (ext/skia and content/renderer) to avoid circular dependencies in gyp. Two try bot failures are not related to my changes.
http://codereview.chromium.org/8566026/diff/8001/content/renderer/skia_sandbo... File content/renderer/skia_sandbox_support_impl.cc (right): http://codereview.chromium.org/8566026/diff/8001/content/renderer/skia_sandbo... content/renderer/skia_sandbox_support_impl.cc:17: void SandboxSupport::EnsureFontLoad(LOGFONT logfont) { This is adding a circular dependency between skia and content; content calls into skia, and adding this would make skia call into content (link modules, not namespaces).
http://codereview.chromium.org/8566026/diff/8001/content/renderer/skia_sandbo... File content/renderer/skia_sandbox_support_impl.cc (right): http://codereview.chromium.org/8566026/diff/8001/content/renderer/skia_sandbo... content/renderer/skia_sandbox_support_impl.cc:17: void SandboxSupport::EnsureFontLoad(LOGFONT logfont) { On 2011/11/17 17:11:04, vandebo wrote: > This is adding a circular dependency between skia and content; content calls > into skia, and adding this would make skia call into content (link modules, not > namespaces). Done.
You should also add a content/ OWNER as reviewer. http://codereview.chromium.org/8566026/diff/15001/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/8566026/diff/15001/content/renderer/render_thr... content/renderer/render_thread_impl.cc:530: // Make sure skia has its sandbox support We should figure out if this is guaranteed to run exactly once and before anything tries to use a font. http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... File content/renderer/skia_sandbox_support_impl.cc (right): http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... content/renderer/skia_sandbox_support_impl.cc:5: //#include "content/renderer/skia_sandbox_support_impl.h" Remove commented lines. http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... File content/renderer/skia_sandbox_support_impl.h (right): http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... content/renderer/skia_sandbox_support_impl.h:10: #if defined(OS_WIN) && defined(USE_SKIA) This is win only, so give the file a _win suffix. http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... content/renderer/skia_sandbox_support_impl.h:21: friend struct DefaultSingletonTraits<SkiaSandboxSupport>; Should this be a LeakySingleton? http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... File skia/ext/SkFontHost_sandbox_win.cpp (right): http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.cpp:8: #ifdef WIN32 You should include build/build_config.h and use the OS_WIN macro, but this will go away anyway. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.cpp:24: void SkFontHost::EnsureTypefaceAccessible(const SkTypeface& typeface) { Lets use Skia's version in the non-windows case. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... File skia/ext/SkFontHost_sandbox_win.h (right): http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. Use Chrome filename naming conventions. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:4: Add generic include guards and pragma once: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#The__define_Guard http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:5: #ifdef WIN32 I don't think you need this guard. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:10: class ISandboxSupport { If you want to note this as an interface, the Chrome convention is an "Interface" suffix: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Interfaces http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:10: class ISandboxSupport { *** I don't think we need this to be a class. Could we have just a function prototype and a global function pointer? http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:14: static ISandboxSupport* g_SkiaSandboxSupport; If we keep this as a class, should we make this private and add a static protected setter and static public getter? http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:14: static ISandboxSupport* g_SkiaSandboxSupport; skia/ext is considered Chrome code, so use Chrome naming conventions: g_skia_sandbox_support_; http://codereview.chromium.org/8566026/diff/15001/skia/skia.gyp File skia/skia.gyp (right): http://codereview.chromium.org/8566026/diff/15001/skia/skia.gyp#newcode481 skia/skia.gyp:481: #'../third_party/skia/src/ports/SkFontHost_sandbox_none.cpp', We can compile this for the non-Windows case.
Added jam@ as reviewer since he reviewed the PreCacheFont part.
http://codereview.chromium.org/8566026/diff/15001/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/8566026/diff/15001/content/renderer/render_thr... content/renderer/render_thread_impl.cc:530: // Make sure skia has its sandbox support On 2011/11/18 21:35:18, vandebo wrote: > We should figure out if this is guaranteed to run exactly once and before > anything tries to use a font. Updated to do the init once in platform init section of render process. http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... File content/renderer/skia_sandbox_support_impl.cc (right): http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... content/renderer/skia_sandbox_support_impl.cc:5: //#include "content/renderer/skia_sandbox_support_impl.h" On 2011/11/18 21:35:18, vandebo wrote: > Remove commented lines. File removed due to design change. http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... File content/renderer/skia_sandbox_support_impl.h (right): http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... content/renderer/skia_sandbox_support_impl.h:10: #if defined(OS_WIN) && defined(USE_SKIA) On 2011/11/18 21:35:18, vandebo wrote: > This is win only, so give the file a _win suffix. Done. http://codereview.chromium.org/8566026/diff/15001/content/renderer/skia_sandb... content/renderer/skia_sandbox_support_impl.h:21: friend struct DefaultSingletonTraits<SkiaSandboxSupport>; On 2011/11/18 21:35:18, vandebo wrote: > Should this be a LeakySingleton? Use global static function pointer instead. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... File skia/ext/SkFontHost_sandbox_win.cpp (right): http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.cpp:8: #ifdef WIN32 On 2011/11/18 21:35:18, vandebo wrote: > You should include build/build_config.h and use the OS_WIN macro, but this will > go away anyway. Done. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.cpp:24: void SkFontHost::EnsureTypefaceAccessible(const SkTypeface& typeface) { On 2011/11/18 21:35:18, vandebo wrote: > Lets use Skia's version in the non-windows case. Done. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... File skia/ext/SkFontHost_sandbox_win.h (right): http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2011/11/18 21:35:18, vandebo wrote: > Use Chrome filename naming conventions. Done. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:4: On 2011/11/18 21:35:18, vandebo wrote: > Add generic include guards and pragma once: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#The__define_Guard Done. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:5: #ifdef WIN32 On 2011/11/18 21:35:18, vandebo wrote: > I don't think you need this guard. Done. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:10: class ISandboxSupport { On 2011/11/18 21:35:18, vandebo wrote: > If you want to note this as an interface, the Chrome convention is an > "Interface" suffix: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Interfaces Use static global function pointer instead. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:10: class ISandboxSupport { On 2011/11/18 21:35:18, vandebo wrote: > *** I don't think we need this to be a class. Could we have just a function > prototype and a global function pointer? Done. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:14: static ISandboxSupport* g_SkiaSandboxSupport; On 2011/11/18 21:35:18, vandebo wrote: > If we keep this as a class, should we make this private and add a static > protected setter and static public getter? Use static global function pointer instead. http://codereview.chromium.org/8566026/diff/15001/skia/ext/SkFontHost_sandbox... skia/ext/SkFontHost_sandbox_win.h:14: static ISandboxSupport* g_SkiaSandboxSupport; On 2011/11/18 21:35:18, vandebo wrote: > skia/ext is considered Chrome code, so use Chrome naming conventions: > g_skia_sandbox_support_; Done. http://codereview.chromium.org/8566026/diff/15001/skia/skia.gyp File skia/skia.gyp (right): http://codereview.chromium.org/8566026/diff/15001/skia/skia.gyp#newcode481 skia/skia.gyp:481: #'../third_party/skia/src/ports/SkFontHost_sandbox_none.cpp', On 2011/11/18 21:35:18, vandebo wrote: > We can compile this for the non-Windows case. Done.
LGTM with nits. http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... File content/renderer/renderer_main_platform_delegate_win.cc (right): http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:15: #if defined (USE_SKIA) nit: defined(USE_SKIA) http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:69: void skia_ensure_font_load(LOGFONT logfont) { nit: match one of the names, i.e. either skia_pre_cache_font or skia_ensure_typeface_accessible http://codereview.chromium.org/8566026/diff/22001/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.h (right): http://codereview.chromium.org/8566026/diff/22001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:9: static void (*g_skia_sandbox_support)(LOGFONT font); nit: g_skia_ensure_typeface_accessible_upcall ?
http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... File content/renderer/renderer_main_platform_delegate_win.cc (right): http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:69: void skia_ensure_font_load(LOGFONT logfont) { chrome style is CamelCase http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:104: g_skia_sandbox_support = skia_ensure_font_load; this won't work in the component build. you'd need a function in the skia code that's exported from skia to set this
http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... File content/renderer/renderer_main_platform_delegate_win.cc (right): http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:15: #if defined (USE_SKIA) On 2011/11/22 01:15:08, vandebo wrote: > nit: defined(USE_SKIA) Done. http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:69: void skia_ensure_font_load(LOGFONT logfont) { On 2011/11/22 01:21:23, John Abd-El-Malek wrote: > chrome style is CamelCase Done. http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:69: void skia_ensure_font_load(LOGFONT logfont) { On 2011/11/22 01:15:08, vandebo wrote: > nit: match one of the names, i.e. either skia_pre_cache_font or > skia_ensure_typeface_accessible Done. http://codereview.chromium.org/8566026/diff/22001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:104: g_skia_sandbox_support = skia_ensure_font_load; On 2011/11/22 01:21:23, John Abd-El-Malek wrote: > this won't work in the component build. you'd need a function in the skia code > that's exported from skia to set this Done. http://codereview.chromium.org/8566026/diff/22001/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.h (right): http://codereview.chromium.org/8566026/diff/22001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:9: static void (*g_skia_sandbox_support)(LOGFONT font); On 2011/11/22 01:15:08, vandebo wrote: > nit: g_skia_ensure_typeface_accessible_upcall ? Done.
http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.h (right): http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:11: static SkiaEnsureTypefaceAccessible g_skia_ensure_typeface_accessible; don't need to expose this?
http://codereview.chromium.org/8566026/diff/28001/content/renderer/renderer_m... File content/renderer/renderer_main_platform_delegate_win.cc (right): http://codereview.chromium.org/8566026/diff/28001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:15: #if defined(USE_SKIA) Windows always uses Skia, you don't need this guard. http://codereview.chromium.org/8566026/diff/28001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:17: #include "third_party/skia/include/core/SkPreConfig.h" Why is SkPreConfig.h included here? For SK_API? If so, it should be in skia_sandbox_support_win.h http://codereview.chromium.org/8566026/diff/28001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:69: #if defined(USE_SKIA) No guard here either. http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.cc (right): http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:7: #if defined (OS_WIN) You don't need this anymore since the file is _win.cc http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:17: g_skia_ensure_typeface_accessible = func; dcheck that the static is currently NULL? http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.h (right): http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:9: typedef void (*SkiaEnsureTypefaceAccessible)(LOGFONT font); predeclare LOGFONT or include the header if you need to. http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:11: static SkiaEnsureTypefaceAccessible g_skia_ensure_typeface_accessible; Move the static to the .cc file? http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:13: SK_API void SetSkiaEnsureTypefaceAccessible(SkiaEnsureTypefaceAccessible); Give the argument a name.
http://codereview.chromium.org/8566026/diff/28001/content/renderer/renderer_m... File content/renderer/renderer_main_platform_delegate_win.cc (right): http://codereview.chromium.org/8566026/diff/28001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:15: #if defined(USE_SKIA) On 2011/11/22 18:23:41, vandebo wrote: > Windows always uses Skia, you don't need this guard. Done. http://codereview.chromium.org/8566026/diff/28001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:17: #include "third_party/skia/include/core/SkPreConfig.h" On 2011/11/22 18:23:41, vandebo wrote: > Why is SkPreConfig.h included here? For SK_API? If so, it should be in > skia_sandbox_support_win.h Done. http://codereview.chromium.org/8566026/diff/28001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:69: #if defined(USE_SKIA) On 2011/11/22 18:23:41, vandebo wrote: > No guard here either. Done. http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.cc (right): http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:7: #if defined (OS_WIN) On 2011/11/22 18:23:41, vandebo wrote: > You don't need this anymore since the file is _win.cc Done. http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:17: g_skia_ensure_typeface_accessible = func; On 2011/11/22 18:23:41, vandebo wrote: > dcheck that the static is currently NULL? It can be non-NULL in component builds. http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.h (right): http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:9: typedef void (*SkiaEnsureTypefaceAccessible)(LOGFONT font); On 2011/11/22 18:23:41, vandebo wrote: > predeclare LOGFONT or include the header if you need to. Done. http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:11: static SkiaEnsureTypefaceAccessible g_skia_ensure_typeface_accessible; On 2011/11/22 18:16:58, John Abd-El-Malek wrote: > don't need to expose this? Done. http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:11: static SkiaEnsureTypefaceAccessible g_skia_ensure_typeface_accessible; On 2011/11/22 18:23:41, vandebo wrote: > Move the static to the .cc file? Done. http://codereview.chromium.org/8566026/diff/28001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.h:13: SK_API void SetSkiaEnsureTypefaceAccessible(SkiaEnsureTypefaceAccessible); On 2011/11/22 18:23:41, vandebo wrote: > Give the argument a name. Done.
http://codereview.chromium.org/8566026/diff/34001/content/renderer/renderer_m... File content/renderer/renderer_main_platform_delegate_win.cc (right): http://codereview.chromium.org/8566026/diff/34001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:13: #include "skia/ext/skia_sandbox_support_win.h" nit: skia comes after sandbox http://codereview.chromium.org/8566026/diff/34001/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.cc (right): http://codereview.chromium.org/8566026/diff/34001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:5: #include "build/build_config.h" nit: I don't think you need build_config any more. http://codereview.chromium.org/8566026/diff/34001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:11: static SkiaEnsureTypefaceAccessible g_skia_ensure_typeface_accessible; nit: = NULL - I know the C spec already does it, but making it explicit makes it easier to read and reason about.
http://codereview.chromium.org/8566026/diff/34001/content/renderer/renderer_m... File content/renderer/renderer_main_platform_delegate_win.cc (right): http://codereview.chromium.org/8566026/diff/34001/content/renderer/renderer_m... content/renderer/renderer_main_platform_delegate_win.cc:13: #include "skia/ext/skia_sandbox_support_win.h" On 2011/11/22 21:07:05, vandebo wrote: > nit: skia comes after sandbox Done. http://codereview.chromium.org/8566026/diff/34001/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.cc (right): http://codereview.chromium.org/8566026/diff/34001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:5: #include "build/build_config.h" On 2011/11/22 21:07:05, vandebo wrote: > nit: I don't think you need build_config any more. Done. http://codereview.chromium.org/8566026/diff/34001/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:11: static SkiaEnsureTypefaceAccessible g_skia_ensure_typeface_accessible; On 2011/11/22 21:07:05, vandebo wrote: > nit: = NULL - I know the C spec already does it, but making it explicit makes it > easier to read and reason about. Done.
LGTM with nits. http://codereview.chromium.org/8566026/diff/37002/skia/ext/skia_sandbox_suppo... File skia/ext/skia_sandbox_support_win.cc (right): http://codereview.chromium.org/8566026/diff/37002/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:5: #include "skia_sandbox_support_win.h" nit: tools/sort-header.py says that this comes at the end. http://codereview.chromium.org/8566026/diff/37002/skia/ext/skia_sandbox_suppo... skia/ext/skia_sandbox_support_win.cc:12: // This function is supposed to be called once in process life time. nit: This function should only be called once.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/arthurhsu@chromium.org/8566026/37002
Change committed as 111400
adding bungeman |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
