Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
439913002:
Add a cache for FillFontFamilyMap. (Closed)
DescriptionAdd a cache for FillFontFamilyMap.
The function is called ~20,000 times on a fresh launch of Chrome on my MacBook
Pro. Adding a dumb cache to reduce redundant fetches reduced cpu cycles in the
function by 75% (profiled by DTrace).
The class FontFamilyCache performs the caching, and the observation of the
PrefService to ensure that the cache does not become stale.
The Profile owns the FontFamilyCache to ensure that the cache does not
outlive the PrefService.
BUG=308095
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287953
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Comments from avi. #
Total comments: 7
Patch Set 3 : Comments from avi, round 2. #
Total comments: 15
Patch Set 4 : Comments from thestig. #
Total comments: 7
Patch Set 5 : Comments from thestig round 2. #
Total comments: 5
Patch Set 6 : Comments from thestig, round 3. #Patch Set 7 : Rebase against top of tree. #Patch Set 8 : Fix broken include. #Patch Set 9 : Add profile destruction observer. #
Total comments: 4
Patch Set 10 : Add DCHECK as requested by thestig. #Patch Set 11 : Minor unit test change. #
Messages
Total messages: 40 (0 generated)
avi: Please review. (I know that you're not an OWNER of the relevant directories. If you don't want to do the review let me know and I will finder another reviewer).
I'm not an expert on the subject matter but that part looks reasonable. Your method of attachment reminds me of base::SupportsUserData which solves your layering problem in almost exactly the same way you solve it here. https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:29: // (font family / script) combinations - see http://crbug.com/308095. Is this comment still relevant? https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... File chrome/browser/font_family_cache.h (right): https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:31: ~FontFamilyCache(); Since FontFamilyCache is going to be deleted through its base-class, the destructor needs to be virtual, as does PrefServiceCache's https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:52: // This method needs to be very fast, because its called ~20,000 times on a s/its/it's/
avi: PTAL I switched over to base::SupportsUserData. I had noticed it before, but the comments and name made me think that it allowed end users to arbitrarily set key/value pairs. I didn't realize that users referred to Chrome. https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:29: // (font family / script) combinations - see http://crbug.com/308095. On 2014/08/05 01:50:26, Avi wrote: > Is this comment still relevant? Yeah. It's referring to the fact that we're scanning over ~1000 pref entries. https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... File chrome/browser/font_family_cache.h (right): https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:31: ~FontFamilyCache(); On 2014/08/05 01:50:26, Avi wrote: > Since FontFamilyCache is going to be deleted through its base-class, the > destructor needs to be virtual, as does PrefServiceCache's ack, thanks. Too much ObjC. https://codereview.chromium.org/439913002/diff/20001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:52: // This method needs to be very fast, because its called ~20,000 times on a On 2014/08/05 01:50:26, Avi wrote: > s/its/it's/ Done.
errr 1 sec, have to update some comments.
PTAL
https://codereview.chromium.org/439913002/diff/40001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/439913002/diff/40001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:515: profile->SetUserData(&chrome::kFontFamilyCacheKey, cache); And you set it on the BrowserContext, which is already supporting user data! Nice! https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:74: size_t map_name_length = strlen(map_name); OnPrefsChanged doesn't need to be fast, so can't we do std::string map_name = it->first; do the conversion to a std::string once, and have all the following code simplified? https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... File chrome/browser/font_family_cache.h (right): https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:7: // is a unique string. Move this class-level comment right above the FontFamilyCache declaration. https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:48: typedef base::hash_map<const char*, ScriptFontMap> FontFamilyMap; What is the ownership story for these char*s?
avi: PTAL https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:74: size_t map_name_length = strlen(map_name); On 2014/08/05 18:07:36, Avi wrote: > OnPrefsChanged doesn't need to be fast, so can't we do > > std::string map_name = it->first; > > do the conversion to a std::string once, and have all the following code > simplified? There are 7 entries in the outer hash_map, ~150 per inner hash_map, which is large enough that I'd still like to avoid unnecessary object construction. Added a comment. https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... File chrome/browser/font_family_cache.h (right): https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:7: // is a unique string. On 2014/08/05 18:07:37, Avi wrote: > Move this class-level comment right above the FontFamilyCache declaration. Done. https://codereview.chromium.org/439913002/diff/40001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:48: typedef base::hash_map<const char*, ScriptFontMap> FontFamilyMap; On 2014/08/05 18:07:36, Avi wrote: > What is the ownership story for these char*s? They're defined at compile time, so we don't have to worry about ownership. Added a comment to FetchAndCacheFont indicating the requirement that the keys of the hash_maps must outlive the cache.
The overall structure LGTM. I'm not that familiar with the font stuff and the like; if you want to get someone to double-check that stuff that'd be cool.
thestig: Looking for an OWNER review of chrome/browser/*
https://codereview.chromium.org/439913002/diff/60001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:506: void FillFontFamilyMap(Profile* profile, How about making this a static method in FontFamilyCache? https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:17: const char* kFontFamilyCacheKey = "FontFamilyCacheKey"; const char kFontFamilyCacheKey[] https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:71: size_t delimiter_length = 1; nit: const https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:72: for (FontFamilyMap::iterator it = font_family_map_.begin(); Not sure why you are iterating through the key values when you can just do a find(). How about: std::vector<std::string> pref_name_split = base::SplitString(pref_name, '.', &pref_name_split); if (pref_name_split.size() != 2) return; FontFamilyMap::iterator map_it = font_family_map_.find(pref_name_split[0]); if (map_it == font_family_map_.end() return; ScriptFontMap& map = it->second; size_t erased = map.erase(pref_name_split[1]); if (erased == 0) return; profile_pref_registrar_.Remove(pref_name.c_str()); https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... File chrome/browser/font_family_cache.h (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:19: namespace chrome { We decided at one point to no longer have 'namespace chrome'. Please try not to add more of it. https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... File chrome/browser/font_family_cache_unittest.cc (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache_unittest.cc:53: EXPECT_EQ(result, font1); nit: EXPECT_EQ(expected, actual) Otherwise if this fails, the message gtest prints looks funny.
thestig: PTAL https://codereview.chromium.org/439913002/diff/60001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:506: void FillFontFamilyMap(Profile* profile, On 2014/08/05 19:26:49, Lei Zhang wrote: > How about making this a static method in FontFamilyCache? Done. https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:17: const char* kFontFamilyCacheKey = "FontFamilyCacheKey"; On 2014/08/05 19:26:49, Lei Zhang wrote: > const char kFontFamilyCacheKey[] Done. https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:71: size_t delimiter_length = 1; On 2014/08/05 19:26:49, Lei Zhang wrote: > nit: const Done. https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:72: for (FontFamilyMap::iterator it = font_family_map_.begin(); On 2014/08/05 19:26:49, Lei Zhang wrote: > Not sure why you are iterating through the key values when you can just do a > find(). How about: > > std::vector<std::string> pref_name_split = base::SplitString(pref_name, '.', > &pref_name_split); > if (pref_name_split.size() != 2) > return; > FontFamilyMap::iterator map_it = font_family_map_.find(pref_name_split[0]); > if (map_it == font_family_map_.end() > return; > ScriptFontMap& map = it->second; > size_t erased = map.erase(pref_name_split[1]); > if (erased == 0) > return; > profile_pref_registrar_.Remove(pref_name.c_str()); I tried out your suggestion. For find() to work, a non-default key comparator needs to be passed as a template argument to the hash_map. We use different implementations of hash_map on different platforms, so there's no simple way to do this. It would also introduce insertion/lookup inefficiencies. By relying on the assumption that the keys are compile time constants, the current hash_map uses pointer comparison instead of cstring comparison. std::find works, but requires us to define a custom comparator anyways, at which point we're not saving on complexity or lines of code. I added further documentation to the header file. https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... File chrome/browser/font_family_cache.h (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:19: namespace chrome { On 2014/08/05 19:26:49, Lei Zhang wrote: > We decided at one point to no longer have 'namespace chrome'. Please try not to > add more of it. Ah. Good to know. Done. https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... File chrome/browser/font_family_cache_unittest.cc (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache_unittest.cc:53: EXPECT_EQ(result, font1); On 2014/08/05 19:26:49, Lei Zhang wrote: > nit: EXPECT_EQ(expected, actual) > > Otherwise if this fails, the message gtest prints looks funny. Done.
https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:72: for (FontFamilyMap::iterator it = font_family_map_.begin(); On 2014/08/05 20:49:23, erikchen wrote: > On 2014/08/05 19:26:49, Lei Zhang wrote: > > Not sure why you are iterating through the key values when you can just do a > > find(). How about: > > > > std::vector<std::string> pref_name_split = base::SplitString(pref_name, '.', > > &pref_name_split); > > if (pref_name_split.size() != 2) > > return; > > FontFamilyMap::iterator map_it = font_family_map_.find(pref_name_split[0]); > > if (map_it == font_family_map_.end() > > return; > > ScriptFontMap& map = it->second; > > size_t erased = map.erase(pref_name_split[1]); > > if (erased == 0) > > return; > > profile_pref_registrar_.Remove(pref_name.c_str()); > > I tried out your suggestion. For find() to work, a non-default key comparator > needs to be passed as a template argument to the hash_map. We use different > implementations of hash_map on different platforms, so there's no simple way to > do this. It would also introduce insertion/lookup inefficiencies. By relying on > the assumption that the keys are compile time constants, the current hash_map > uses pointer comparison instead of cstring comparison. > > std::find works, but requires us to define a custom comparator anyways, at which > point we're not saving on complexity or lines of code. > > I added further documentation to the header file. Ok, but can't you still split the pref_name to help you simplify the code a little? https://codereview.chromium.org/439913002/diff/80001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/439913002/diff/80001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:505: #if !defined(OS_ANDROID) && defined(OS_POSIX) && !defined(OS_MACOSX) nit: #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID) https://codereview.chromium.org/439913002/diff/80001/chrome/browser/font_fami... File chrome/browser/font_family_cache.h (right): https://codereview.chromium.org/439913002/diff/80001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:48: typedef base::hash_map<const char*, base::string16> ScriptFontMap; nit: add a new line after. https://codereview.chromium.org/439913002/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/439913002/diff/80001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:453: 'browser/font_family_cache.cc', How about putting these inside the 'chrome_browser_non_android_sources' variable for now? Do the same for the unit test to exclude it on Android.
thestig: PTAL https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:72: for (FontFamilyMap::iterator it = font_family_map_.begin(); On 2014/08/05 21:15:08, Lei Zhang wrote: > On 2014/08/05 20:49:23, erikchen wrote: > > On 2014/08/05 19:26:49, Lei Zhang wrote: > > > Not sure why you are iterating through the key values when you can just do a > > > find(). How about: > > > > > > std::vector<std::string> pref_name_split = base::SplitString(pref_name, '.', > > > &pref_name_split); > > > if (pref_name_split.size() != 2) > > > return; > > > FontFamilyMap::iterator map_it = font_family_map_.find(pref_name_split[0]); > > > if (map_it == font_family_map_.end() > > > return; > > > ScriptFontMap& map = it->second; > > > size_t erased = map.erase(pref_name_split[1]); > > > if (erased == 0) > > > return; > > > profile_pref_registrar_.Remove(pref_name.c_str()); > > > > I tried out your suggestion. For find() to work, a non-default key comparator > > needs to be passed as a template argument to the hash_map. We use different > > implementations of hash_map on different platforms, so there's no simple way > to > > do this. It would also introduce insertion/lookup inefficiencies. By relying > on > > the assumption that the keys are compile time constants, the current hash_map > > uses pointer comparison instead of cstring comparison. > > > > std::find works, but requires us to define a custom comparator anyways, at > which > > point we're not saving on complexity or lines of code. > > > > I added further documentation to the header file. > > Ok, but can't you still split the pref_name to help you simplify the code a > little? Not without making another assumption about delimiters. Separating out |map_name| from |script| in |pref_name| relies on the assumption that |script| does not contain any '.' (map_name contains many '.'). This assumption is currently true, but I didn't see a need to introduce another assumption if its unneeded. If you prefer, I can update the code to use that assumption and separate out |script| and |map_name| from |pref_name| https://codereview.chromium.org/439913002/diff/80001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/439913002/diff/80001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:505: #if !defined(OS_ANDROID) && defined(OS_POSIX) && !defined(OS_MACOSX) On 2014/08/05 21:15:08, Lei Zhang wrote: > nit: #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID) Is the goal to get all the positive conditionals before all the negative conditionals? If so, done. https://codereview.chromium.org/439913002/diff/80001/chrome/browser/font_fami... File chrome/browser/font_family_cache.h (right): https://codereview.chromium.org/439913002/diff/80001/chrome/browser/font_fami... chrome/browser/font_family_cache.h:48: typedef base::hash_map<const char*, base::string16> ScriptFontMap; On 2014/08/05 21:15:08, Lei Zhang wrote: > nit: add a new line after. Done. https://codereview.chromium.org/439913002/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/439913002/diff/80001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:453: 'browser/font_family_cache.cc', On 2014/08/05 21:15:08, Lei Zhang wrote: > How about putting these inside the 'chrome_browser_non_android_sources' variable > for now? Do the same for the unit test to exclude it on Android. Did as suggested for this file. I explicitly excluded the unit test on Android to match the formatting of that gypi file.
https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/60001/chrome/browser/font_fami... chrome/browser/font_family_cache.cc:72: for (FontFamilyMap::iterator it = font_family_map_.begin(); On 2014/08/05 21:34:16, erikchen wrote: > On 2014/08/05 21:15:08, Lei Zhang wrote: > > On 2014/08/05 20:49:23, erikchen wrote: > > > On 2014/08/05 19:26:49, Lei Zhang wrote: > > > > Not sure why you are iterating through the key values when you can just do > a > > > > find(). How about: > > > > > > > > std::vector<std::string> pref_name_split = base::SplitString(pref_name, > '.', > > > > &pref_name_split); > > > > if (pref_name_split.size() != 2) > > > > return; > > > > FontFamilyMap::iterator map_it = > font_family_map_.find(pref_name_split[0]); > > > > if (map_it == font_family_map_.end() > > > > return; > > > > ScriptFontMap& map = it->second; > > > > size_t erased = map.erase(pref_name_split[1]); > > > > if (erased == 0) > > > > return; > > > > profile_pref_registrar_.Remove(pref_name.c_str()); > > > > > > I tried out your suggestion. For find() to work, a non-default key > comparator > > > needs to be passed as a template argument to the hash_map. We use different > > > implementations of hash_map on different platforms, so there's no simple way > > to > > > do this. It would also introduce insertion/lookup inefficiencies. By relying > > on > > > the assumption that the keys are compile time constants, the current > hash_map > > > uses pointer comparison instead of cstring comparison. > > > > > > std::find works, but requires us to define a custom comparator anyways, at > > which > > > point we're not saving on complexity or lines of code. > > > > > > I added further documentation to the header file. > > > > Ok, but can't you still split the pref_name to help you simplify the code a > > little? > > Not without making another assumption about delimiters. Separating out > |map_name| from |script| in |pref_name| relies on the assumption that |script| > does not contain any '.' (map_name contains many '.'). This assumption is > currently true, but I didn't see a need to introduce another assumption if its > unneeded. > > If you prefer, I can update the code to use that assumption and separate out > |script| and |map_name| from |pref_name| Ah, n/m, I had a mental model that |pref_name| is just foo.bar. https://codereview.chromium.org/439913002/diff/80001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/439913002/diff/80001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:505: #if !defined(OS_ANDROID) && defined(OS_POSIX) && !defined(OS_MACOSX) On 2014/08/05 21:34:17, erikchen wrote: > On 2014/08/05 21:15:08, Lei Zhang wrote: > > nit: #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID) > > Is the goal to get all the positive conditionals before all the negative > conditionals? If so, done. Yes, it's more readable. https://codereview.chromium.org/439913002/diff/100001/chrome/browser/font_fam... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/100001/chrome/browser/font_fam... chrome/browser/font_family_cache.cc:94: if (pref_name.compare(0, map_name_length, map_name) != 0) Shouldn't you also check and make sure |pref_name| has a delimiter at the |map_name_length|-th spot to avoid the corner case of "foo.bar_qux" vs "foo.bar.qux" ? https://codereview.chromium.org/439913002/diff/100001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/439913002/diff/100001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:2586: 'browser/font_family_cache_unittest.cc', This is in a "sources!" block. You also need it in the "sources" block like you did before.
thestig: PTAL https://codereview.chromium.org/439913002/diff/100001/chrome/browser/font_fam... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/100001/chrome/browser/font_fam... chrome/browser/font_family_cache.cc:94: if (pref_name.compare(0, map_name_length, map_name) != 0) On 2014/08/05 21:48:55, Lei Zhang wrote: > Shouldn't you also check and make sure |pref_name| has a delimiter at the > |map_name_length|-th spot to avoid the corner case of "foo.bar_qux" vs > "foo.bar.qux" ? Good call. Done. https://codereview.chromium.org/439913002/diff/100001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/439913002/diff/100001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:2586: 'browser/font_family_cache_unittest.cc', On 2014/08/05 21:48:55, Lei Zhang wrote: > This is in a "sources!" block. You also need it in the "sources" block like you > did before. It's already there at line 1026.
lgtm https://codereview.chromium.org/439913002/diff/100001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/439913002/diff/100001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:2586: 'browser/font_family_cache_unittest.cc', On 2014/08/05 22:03:04, erikchen wrote: > On 2014/08/05 21:48:55, Lei Zhang wrote: > > This is in a "sources!" block. You also need it in the "sources" block like > you > > did before. > > It's already there at line 1026. Whoops, was looking at the wrong diff.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/439913002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/37723) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42889) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/37744) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/439913002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...) mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...) mac_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/439913002/180001
The CQ bit was unchecked by erikchen@chromium.org
thestig: PTAL See the diff from patch set 9 against patch set 8. Profile destruction was segfaulting. I added an observer of profile destruction to FontFamilyCache so that it could perform appropriate cleanup.
https://codereview.chromium.org/439913002/diff/240001/chrome/browser/font_fam... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/240001/chrome/browser/font_fam... chrome/browser/font_family_cache.cc:25: chrome::NOTIFICATION_PROFILE_DESTROYED, We are trying to move away from Notifications. avi: Is there a replacement for this? https://codereview.chromium.org/439913002/diff/240001/chrome/browser/font_fam... chrome/browser/font_family_cache.cc:132: profile_pref_registrar_.RemoveAll(); DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
https://codereview.chromium.org/439913002/diff/240001/chrome/browser/font_fam... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/240001/chrome/browser/font_fam... chrome/browser/font_family_cache.cc:25: chrome::NOTIFICATION_PROFILE_DESTROYED, On 2014/08/06 20:56:55, Lei Zhang wrote: > We are trying to move away from Notifications. > > avi: Is there a replacement for this? No, there currently isn't a replacement for this. Let this go.
lgtm++ then
https://codereview.chromium.org/439913002/diff/240001/chrome/browser/font_fam... File chrome/browser/font_family_cache.cc (right): https://codereview.chromium.org/439913002/diff/240001/chrome/browser/font_fam... chrome/browser/font_family_cache.cc:132: profile_pref_registrar_.RemoveAll(); On 2014/08/06 20:56:54, Lei Zhang wrote: > DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type); Done.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/439913002/260001
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/439913002/280001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Message was sent while issue was closed.
Change committed as 287953 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
