Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1327673002:
Added GDI font emulation support for Flash. (Closed)
DescriptionAdded GDI font emulation support for Flash.
This patch adds simple GDI font enumeration emulation support to get
Flash running under win32k lockdown.
BUG=523278
Committed: https://crrev.com/69dcd1b889f26db433643f12ce21194301266eb5
Cr-Commit-Position: refs/heads/master@{#350245}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Updates from review. #
Total comments: 2
Patch Set 3 : Changed comment and used common check #
Total comments: 31
Patch Set 4 : Implemented review changes. #
Total comments: 16
Patch Set 5 : Fixed from review. #
Total comments: 8
Patch Set 6 : More fixes for review. #
Depends on Patchset: Messages
Total messages: 45 (16 generated)
forshaw@chromium.org changed reviewers: + palmer@chromium.org, wfh@chromium.org
Please take a look.
https://codereview.chromium.org/1327673002/diff/1/content/common/render_font_... File content/common/render_font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/1/content/common/render_font_... content/common/render_font_warmup_win.cc:213: return NULL; nullptr? I don't know if it's now-standard in Windows-specific areas of Chromium, but it is elsewhere. https://codereview.chromium.org/1327673002/diff/1/content/common/render_font_... content/common/render_font_warmup_win.cc:226: for (auto i = objects_.begin(); i != objects_.end(); ++i) { Does for (const auto& object : objects_) { ... } work here? https://codereview.chromium.org/1327673002/diff/1/content/common/render_font_... File content/common/render_font_warmup_win_unittest.cc (right): https://codereview.chromium.org/1327673002/diff/1/content/common/render_font_... content/common/render_font_warmup_win_unittest.cc:25: size_t dataLength) : Is this what "git cl format" outputs? https://codereview.chromium.org/1327673002/diff/1/content/common/render_font_... content/common/render_font_warmup_win_unittest.cc:44: return NULL; nullptr again https://codereview.chromium.org/1327673002/diff/1/content/ppapi_plugin/ppapi_... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/1327673002/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:430: // its GDI font enumeration code. Nit: The code is admirably clear, so I don't think this comment is strictly necessary.
Updated, PTAL. https://codereview.chromium.org/1327673002/diff/1/content/common/render_font_... File content/common/render_font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/1/content/common/render_font_... content/common/render_font_warmup_win.cc:213: return NULL; On 2015/09/11 22:34:15, palmer wrote: > nullptr? I don't know if it's now-standard in Windows-specific areas of > Chromium, but it is elsewhere. Will change. https://codereview.chromium.org/1327673002/diff/1/content/common/render_font_... content/common/render_font_warmup_win.cc:226: for (auto i = objects_.begin(); i != objects_.end(); ++i) { On 2015/09/11 22:34:16, palmer wrote: > Does > > for (const auto& object : objects_) { ... } > > work here? Unless I'm missing something I need the iterator to pass into erase so that the element can be removed from the vector, so probably not.
LGTM, with a nit for clarification. https://codereview.chromium.org/1327673002/diff/20001/content/common/font_war... File content/common/font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/20001/content/common/font_war... content/common/font_warmup_win.cc:360: // getTableData takes care of lpvBuffer being nullptr. Swap tag to counter Nit: This is some subtle stuff, and the comment should be as clear as possible. The sentence structure is a bit complex; here's an attempt to simplify it: |getTableData| can handle the case that |lpvBuffer| is nullptr. However, if it is, pass INT32_MAX as the length. (Otherwise, |getTableData| returns the minimum of the table size and the passed-in length. If the caller were to pass 0, which |GetFontData| allows, then |getTableData| would return 0, which is incorrect.) Swap the bytes of the |dwTable| tag to counter an effect of the underlying implementation (specifically, that TODO). (I.e. can you explain what effect you're countering?) And, does my rewrite evince understanding? If not, well, ... :) )
https://codereview.chromium.org/1327673002/diff/20001/content/common/font_war... File content/common/font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/20001/content/common/font_war... content/common/font_warmup_win.cc:360: // getTableData takes care of lpvBuffer being nullptr. Swap tag to counter On 2015/09/15 21:13:28, palmer wrote: > Nit: This is some subtle stuff, and the comment should be as clear as possible. > The sentence structure is a bit complex; here's an attempt to simplify it: > > |getTableData| can handle the case that |lpvBuffer| is nullptr. However, if it > is, pass INT32_MAX as the length. (Otherwise, |getTableData| returns the minimum > of the table size and the passed-in length. If the caller were to pass 0, which > |GetFontData| allows, then |getTableData| would return 0, which is incorrect.) > > Swap the bytes of the |dwTable| tag to counter an effect of the underlying > implementation (specifically, that TODO). > > (I.e. can you explain what effect you're countering?) > > And, does my rewrite evince understanding? If not, well, ... :) ) Reworded the comment.
Still LGTM. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... File content/common/font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:363: // common Windows idiom is to pass 0 as cbSize when passing nullptr, which Nit: Don't forget to use |...| to distinguish identifiers (|cbSize| and |dwTable|).
jam@chromium.org changed reviewers: + jam@chromium.org
perhaps send to scottmg since i think he worked in that area? then you can get an owner approval from top level owners in security like creis or nasko
forshaw@chromium.org changed reviewers: + scottmg@chromium.org
@scottmg could you take a look at the GDI font emulation work which is based on the back of your original direct write font manager work? It's implemented to support the basic needs of the PPAPI Flash plugin (at least if we can't change Flash itself). Thanks very much.
On 2015/09/17 22:01:53, forshaw wrote: > @scottmg could you take a look at the GDI font emulation work which is based on > the back of your original direct write font manager work? It's implemented to > support the basic needs of the PPAPI Flash plugin (at least if we can't change > Flash itself). Thanks very much. While this approach will work, wouldn't it be better to fix pepper flash to use skia or DirectWrite directly? Feels strange to be intercepting API's for a product which we ship along with Chrome.
On 2015/09/17 22:46:11, ananta wrote: > On 2015/09/17 22:01:53, forshaw wrote: > > @scottmg could you take a look at the GDI font emulation work which is based > on > > the back of your original direct write font manager work? It's implemented to > > support the basic needs of the PPAPI Flash plugin (at least if we can't change > > Flash itself). Thanks very much. > > While this approach will work, wouldn't it be better to fix pepper flash to use > skia or DirectWrite directly? > > Feels strange to be intercepting API's for a product which we ship along with > Chrome. @anata, certainly removing all traces of GDI from Flash itself would be preferable but I don't believe we have a lot of say on exactly what goes into Flash (at least in the sense that we don't actually build it ourselves) so would need to get it through Adobe who might not want to support the GDI and alternative route, although who knows. Adding Skia or Direct Write would be a challenge to do inside Flash itself because of the assumptions that Chrome makes on access to fonts etc. The best approach would be to extend slightly the PPAPI to handle this use case but that seems like a lot of effort for minimal gain. Imo at least the usage of these APIs is very specific and limited to a small subset of the functionality provided by GDI which makes it relatively easy to emulate through hooking and perhaps this can be a temporary measure to tide us over until we can change Flash. The alternative is to drop this functionality and then get no test data what so ever because we're waiting on a new version of Flash to come about (and PDFium still needs a patch as well so without at least one thing this whole effort is wasted). With the known attacks against Chrome sandbox through the Win32k vector I'm of the opinion to take the quick, but not ideal approach over nothing at all.
Looks fine (except maybe changing the list to a map), but I had the same concern as Ananta -- can't we just switch Pepper to using DW instead? https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... File content/common/font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:248: uint32_t magic_; it would pack better on x64 to have magic_ lower down, but if think it's useful to have it at the head for memory inspection, ok to leave it there. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:251: static uintptr_t curr_handle_; separate statics from members https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:252: static std::list<scoped_refptr<FakeGdiObject>> objects_; is a map from handle->FakeGdiObject not sensible for some reason? (with validation on lookup still) https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:256: friend class base::RefCountedThreadSafe<FakeGdiObject>; move friend to first after private: https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:312: LPLOGFONTW lpLogfont, i know they're copied from the declarations, but no hungarian/TitleCaps, just log_font, enum_font_fam_ex_proc, etc. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:377: HGDIOBJ WINAPI SelectObjectPatch(HDC hdc, HGDIOBJ hgdiobj) { is it never possible for these patches to get called with real gdi objects? https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... File content/common/font_warmup_win_unittest.cc (right): https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:19: class TestSkTypeface : public SkTypeface { all in namespace content { https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:83: /** Returns the family name of the typeface as known by its font manager. no /** or /* comments unless in .c files. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:131: static inline DWORD SwapEndian(DWORD dwTable) { replace with base/sys_byteorder.h https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:222: memcpy(logfont->lfFaceName, fontname, probably better to use the (small) target buffer size even though it's just test code https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:259: EXPECT_TRUE(hdc != nullptr); EXPECT_NE(nullptr, hdc) and similar below https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:268: EXPECT_FALSE(!patch_data); does EXPECT_TRUE() not compile? https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:340: HDC hdc = reinterpret_cast<HDC>(0x11223344); maybe use something other than 11223344 unless you specifically mean for it to be the same as kTestFontTableTag https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:425: EXPECT_TRUE(DeleteDC(hdc)); is it worth verifying the globals are empty in some tests?
https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... File content/common/font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:248: uint32_t magic_; On 2015/09/17 23:24:24, scottmg wrote: > it would pack better on x64 to have magic_ lower down, but if think it's useful > to have it at the head for memory inspection, ok to leave it there. I could move it, as this class (presumably) has a virtual destructor it won't be directly at the start of the object anyway although the number of in flight copies of this class hopefully will be very low so an additional 4 bytes might not matter too much. I'll move anyway. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:251: static uintptr_t curr_handle_; On 2015/09/17 23:24:25, scottmg wrote: > separate statics from members Acknowledged. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:252: static std::list<scoped_refptr<FakeGdiObject>> objects_; On 2015/09/17 23:24:24, scottmg wrote: > is a map from handle->FakeGdiObject not sensible for some reason? (with > validation on lookup still) If I can just use map directly then I guess it shouldn't be an issue, just key off void* and I should be fine. This is probably just a vestige of a previous versions. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:256: friend class base::RefCountedThreadSafe<FakeGdiObject>; On 2015/09/17 23:24:24, scottmg wrote: > move friend to first after private: Acknowledged. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:312: LPLOGFONTW lpLogfont, On 2015/09/17 23:24:24, scottmg wrote: > i know they're copied from the declarations, but no hungarian/TitleCaps, just > log_font, enum_font_fam_ex_proc, etc. Acknowledged. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:363: // common Windows idiom is to pass 0 as cbSize when passing nullptr, which On 2015/09/17 21:12:52, palmer wrote: > Nit: Don't forget to use |...| to distinguish identifiers (|cbSize| and > |dwTable|). Will do. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win.cc:377: HGDIOBJ WINAPI SelectObjectPatch(HDC hdc, HGDIOBJ hgdiobj) { On 2015/09/17 23:24:24, scottmg wrote: > is it never possible for these patches to get called with real gdi objects? These patches should only be enabled if the win32k system calls have been disabled, so in theory there can't be any read gdi objects in process as you can only get a real handle from the kernel. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... File content/common/font_warmup_win_unittest.cc (right): https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:19: class TestSkTypeface : public SkTypeface { On 2015/09/17 23:24:25, scottmg wrote: > all in namespace content { Acknowledged. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:83: /** Returns the family name of the typeface as known by its font manager. On 2015/09/17 23:24:25, scottmg wrote: > no /** or /* comments unless in .c files. Okay, this was just C&P from the skia headers, will remove and probably don't need comments here. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:131: static inline DWORD SwapEndian(DWORD dwTable) { On 2015/09/17 23:24:25, scottmg wrote: > replace with base/sys_byteorder.h Acknowledged. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:222: memcpy(logfont->lfFaceName, fontname, On 2015/09/17 23:24:25, scottmg wrote: > probably better to use the (small) target buffer size even though it's just test > code Acknowledged. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:259: EXPECT_TRUE(hdc != nullptr); On 2015/09/17 23:24:25, scottmg wrote: > EXPECT_NE(nullptr, hdc) and similar below Acknowledged. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:268: EXPECT_FALSE(!patch_data); On 2015/09/17 23:24:25, scottmg wrote: > does EXPECT_TRUE() not compile? Acknowledged. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:340: HDC hdc = reinterpret_cast<HDC>(0x11223344); On 2015/09/17 23:24:25, scottmg wrote: > maybe use something other than 11223344 unless you specifically mean for it to > be the same as kTestFontTableTag Acknowledged. https://codereview.chromium.org/1327673002/diff/40001/content/common/font_war... content/common/font_warmup_win_unittest.cc:425: EXPECT_TRUE(DeleteDC(hdc)); On 2015/09/17 23:24:25, scottmg wrote: > is it worth verifying the globals are empty in some tests? As in the global state of handles? Yeah probably, will look at it.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
PTAL, I've implemented the requested review changes.
LGTM. Do we have anyone tasked with or a bug for following up with Adobe to have them avoid calling GDI instead?
forshaw@chromium.org changed reviewers: - jam@chromium.org
forshaw@chromium.org changed reviewers: + nasko@chromium.org
nasko@ could you review these changes for me. Thanks.
Mostly style issues. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... File content/common/font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:208: // The returned object is either nullptr or the new object. This method doesn't allocate an object, so the comment saying it returns "the new object" is a bit strange. Isn't it an existing object? https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:265: static base::CheckedNumeric<uintptr_t> curr_handle_; Static members should come prior to member variables. Also, in general, the pattern of having static class variables isn't used in Chromium, instead a lazy instance variable in anonymous namespace is used. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:320: return FALSE; Lowercase false. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:326: return FALSE; Lowercase false. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:337: LOG(ERROR) << "Invalid DC Object"; LOG(ERROR) is usually avoided. Please convert it to some form of D*LOG or just remove. Same for the LOGs below. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... File content/common/font_warmup_win.h (right): https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.h:48: CONTENT_EXPORT void SetPreSandboxWarmupFontMgr(SkFontMgr* fontmgr); Testing only methods should indicate this somehow in the name. A common pattern is to have "ForTesting" suffix. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... File content/common/font_warmup_win_unittest.cc (right): https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win_unittest.cc:151: /** May return nullptr if the name is not found. */ Use // for comments. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win_unittest.cc:159: if (strcmp(familyName, kTestFontFamily) == 0) { nit: no need for {} in one line if statements.
Updated from review, PTAL. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... File content/common/font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:208: // The returned object is either nullptr or the new object. On 2015/09/21 15:58:20, nasko (slow to review) wrote: > This method doesn't allocate an object, so the comment saying it returns "the > new object" is a bit strange. Isn't it an existing object? Acknowledged. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:265: static base::CheckedNumeric<uintptr_t> curr_handle_; On 2015/09/21 15:58:20, nasko (slow to review) wrote: > Static members should come prior to member variables. > > Also, in general, the pattern of having static class variables isn't used in > Chromium, instead a lazy instance variable in anonymous namespace is used. Acknowledged. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:320: return FALSE; On 2015/09/21 15:58:20, nasko (slow to review) wrote: > Lowercase false. Acknowledged. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:326: return FALSE; On 2015/09/21 15:58:20, nasko (slow to review) wrote: > Lowercase false. Acknowledged. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.cc:337: LOG(ERROR) << "Invalid DC Object"; On 2015/09/21 15:58:20, nasko (slow to review) wrote: > LOG(ERROR) is usually avoided. Please convert it to some form of D*LOG or just > remove. Same for the LOGs below. Acknowledged. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... File content/common/font_warmup_win.h (right): https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win.h:48: CONTENT_EXPORT void SetPreSandboxWarmupFontMgr(SkFontMgr* fontmgr); On 2015/09/21 15:58:20, nasko (slow to review) wrote: > Testing only methods should indicate this somehow in the name. A common pattern > is to have "ForTesting" suffix. Acknowledged. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... File content/common/font_warmup_win_unittest.cc (right): https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win_unittest.cc:151: /** May return nullptr if the name is not found. */ On 2015/09/21 15:58:20, nasko (slow to review) wrote: > Use // for comments. Acknowledged. https://codereview.chromium.org/1327673002/diff/100001/content/common/font_wa... content/common/font_warmup_win_unittest.cc:159: if (strcmp(familyName, kTestFontFamily) == 0) { On 2015/09/21 15:58:20, nasko (slow to review) wrote: > nit: no need for {} in one line if statements. Acknowledged.
Another round of nits. https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... File content/common/font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... content/common/font_warmup_win.cc:208: void SetTypeface(const skia::RefPtr<SkTypeface>& typeface) { set_typeface(...) https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... content/common/font_warmup_win.cc:212: skia::RefPtr<SkTypeface> GetTypeface() { return typeface_; } Simple accessors are just named after the variable and use hacker_case. typeface() in this case. handle() and magic() for the ones below. https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... content/common/font_warmup_win.cc:230: class FakeGdiObjectFactory { A small class level comment explaining the goal of this class will be useful. https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... content/common/font_warmup_win.cc:288: base::LazyInstance<FakeGdiObjectFactory>::Leaky g_fake_gdi_object_factory = I'd move this right above where the other constants for this piece of code are located.
Hopefully fixed the nits. https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... File content/common/font_warmup_win.cc (right): https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... content/common/font_warmup_win.cc:208: void SetTypeface(const skia::RefPtr<SkTypeface>& typeface) { On 2015/09/21 22:58:19, nasko (slow to review) wrote: > set_typeface(...) Acknowledged. https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... content/common/font_warmup_win.cc:212: skia::RefPtr<SkTypeface> GetTypeface() { return typeface_; } On 2015/09/21 22:58:19, nasko (slow to review) wrote: > Simple accessors are just named after the variable and use hacker_case. > typeface() in this case. handle() and magic() for the ones below. Acknowledged. https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... content/common/font_warmup_win.cc:230: class FakeGdiObjectFactory { On 2015/09/21 22:58:19, nasko (slow to review) wrote: > A small class level comment explaining the goal of this class will be useful. Acknowledged. https://codereview.chromium.org/1327673002/diff/120001/content/common/font_wa... content/common/font_warmup_win.cc:288: base::LazyInstance<FakeGdiObjectFactory>::Leaky g_fake_gdi_object_factory = On 2015/09/21 22:58:19, nasko (slow to review) wrote: > I'd move this right above where the other constants for this piece of code are > located. Hopefully I've moved it to where you expected, well I moved the constants down below this line. Otherwise more refactoring would be required to get it to the top of the source file.
lgtm
On 2015/09/21 23:45:26, nasko (slow to review) wrote: > lgtm Awesome, thanks so much nasko@.
The CQ bit was checked by forshaw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1327673002/#ps140001 (title: "More fixes for review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1327673002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1327673002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by forshaw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1327673002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1327673002/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/69dcd1b889f26db433643f12ce21194301266eb5 Cr-Commit-Position: refs/heads/master@{#350245}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
clang-cl complains:
FAILED: ninja -t msvc -e environment.x86 --
"..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m32 /nologo
/showIncludes /FC @obj\content\common\content.font_warmup_win.obj.rsp /c
..\..\content\common\font_warmup_win.cc
/Foobj\content\common\content.font_warmup_win.obj /Fdobj\content\content.cc.pdb
..\..\content\common\font_warmup_win.cc(202,9) : error: field 'magic_' will be
initialized after field 'handle_' [-Werror,-Wreorder]
: magic_(magic), handle_(handle) {}
^
..\..\content\common\font_warmup_win.cc(356,37) : error: suggest braces around
initialization of subobject [-Werror,-Wmissing-braces]
ENUMLOGFONTEXDVW enum_log_font = {0};
^
{}
..\..\content\common\font_warmup_win.cc(356,37) : error: suggest braces around
initialization of subobject [-Werror,-Wmissing-braces]
ENUMLOGFONTEXDVW enum_log_font = {0};
^
{}
..\..\content\common\font_warmup_win.cc(360,35) : error: suggest braces around
initialization of subobject [-Werror,-Wmissing-braces]
NEWTEXTMETRICEXW text_metric = {0};
^
{}
..\..\content\common\font_warmup_win.cc(203,3) : error: [chromium-style] Classes
that are ref-counted should have destructors that are declared protected or
private.
~FakeGdiObject() {}
^
..\..\content\common\font_warmup_win.cc(199,23) : note: [chromium-style]
'FakeGdiObject' inherits from 'base::RefCountedThreadSafe<FakeGdiObject>' here
class FakeGdiObject : public base::RefCountedThreadSafe<FakeGdiObject> {
^
5 errors generated.
Can you take a look? I'd fix it myself, but I'm on hotel wireless and the blink
stuff landed, so `git pull` seems to be taking ~hours over here :-(
The CQ bit was checked by forshaw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, nasko@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1327673002/#ps160001 (title: "Fix clang errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1327673002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1327673002/160001
The CQ bit was unchecked by forshaw@chromium.org
Patchset #7 (id:160001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
