Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
737103002:
Attempt to fallback to some matching font in the GetMatchingDirectWriteFontForTypeface function in … (Closed)
DescriptionAttempt to fallback to some matching font in the GetMatchingDirectWriteFontForTypeface function in case we fail to find a best match via IDWriteFontFamily::GetFirstMatchingFont.
The IDWriteFontFamily::GetFirstMatchingFont call fails on some machines with E_FAIL with well known fonts like MS UI Gothic,
Segoe UI, etc. It is not clear as to why a font would be accessible to GDI and not to DirectWrite.
Added some debug alias variables to help track down these failures. We also attempt fallback to some font in the IDWriteFontList
for the font family in question. If all fail, we CHECK as before.
Next step would be to remove the CHECKS and fallback to GDI if CreateFontFromSkia fails.
BUG=434425
Committed: https://crrev.com/ad29fae1fa8166c5755e9508c59297ddcc4ad198
Cr-Commit-Position: refs/heads/master@{#304981}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address scottmg review comments #
Total comments: 2
Patch Set 3 : Address sky review comments #Patch Set 4 : Reverted debugging change #
Total comments: 6
Patch Set 5 : Add safety checks to FindFamilyName #Patch Set 6 : Fixed alias #Patch Set 7 : Removed unnecessary alias #
Total comments: 2
Patch Set 8 : Addressed sky review comments #Messages
Total messages: 22 (5 generated)
ananta@chromium.org changed reviewers: + scottmg@chromium.org, sky@chromium.org
lgtm https://codereview.chromium.org/737103002/diff/1/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/1/ui/gfx/platform_font_win.cc#... ui/gfx/platform_font_win.cc:141: // GDI. add crbug link https://codereview.chromium.org/737103002/diff/1/ui/gfx/platform_font_win.cc#... ui/gfx/platform_font_win.cc:149: matching_font_count = matching_font_list->GetFontCount(); is it necessary to debug::Alias again after the assignment? https://codereview.chromium.org/737103002/diff/1/ui/gfx/platform_font_win.cc#... ui/gfx/platform_font_win.cc:160: fallback_to_matching_font = true; same
https://codereview.chromium.org/737103002/diff/1/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/1/ui/gfx/platform_font_win.cc#... ui/gfx/platform_font_win.cc:141: // GDI. On 2014/11/19 22:15:36, scottmg wrote: > add crbug link Done. https://codereview.chromium.org/737103002/diff/1/ui/gfx/platform_font_win.cc#... ui/gfx/platform_font_win.cc:149: matching_font_count = matching_font_list->GetFontCount(); On 2014/11/19 22:15:36, scottmg wrote: > is it necessary to debug::Alias again after the assignment? Done. https://codereview.chromium.org/737103002/diff/1/ui/gfx/platform_font_win.cc#... ui/gfx/platform_font_win.cc:160: fallback_to_matching_font = true; On 2014/11/19 22:15:36, scottmg wrote: > same Done.
https://codereview.chromium.org/737103002/diff/20001/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/20001/ui/gfx/platform_font_win... ui/gfx/platform_font_win.cc:163: base::debug::Alias(&fallback_to_matching_font); Can you move the Alias calls to where you need it? The check on 165?
https://codereview.chromium.org/737103002/diff/20001/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/20001/ui/gfx/platform_font_win... ui/gfx/platform_font_win.cc:163: base::debug::Alias(&fallback_to_matching_font); On 2014/11/19 22:33:01, sky wrote: > Can you move the Alias calls to where you need it? The check on 165? Done.
shrikant@chromium.org changed reviewers: + shrikant@chromium.org
https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win... ui/gfx/platform_font_win.cc:93: hr = font_collection->FindFamilyName(face_name, &index, &exists); Besides FAILED(hr) can you add check for index == UINT_MAX and exists == TRUE and font_family != NULL? Same with function below? In the dump that I saw, font_family was NULL and index -1 and exists FALSE. Not sure hr was S_OK after this statement. http://msdn.microsoft.com/en-us/library/windows/desktop/dd368217(v=vs.85).aspx
On 2014/11/19 22:59:26, Shrikant Kelkar wrote: > https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win.cc > File ui/gfx/platform_font_win.cc (right): > > https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win... > ui/gfx/platform_font_win.cc:93: hr = font_collection->FindFamilyName(face_name, > &index, &exists); > Besides FAILED(hr) can you add check for index == UINT_MAX and exists == TRUE > and font_family != NULL? > > Same with function below? > > In the dump that I saw, font_family was NULL and index -1 and exists FALSE. Not > sure hr was S_OK after this statement. > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd368217(v=vs.85).aspx and Font Name was "Segoe UI Semibold" in dump that I opened.
https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win... ui/gfx/platform_font_win.cc:93: hr = font_collection->FindFamilyName(face_name, &index, &exists); On 2014/11/19 22:59:25, Shrikant Kelkar wrote: > Besides FAILED(hr) can you add check for index == UINT_MAX and exists == TRUE > and font_family != NULL? > > Same with function below? > > In the dump that I saw, font_family was NULL and index -1 and exists FALSE. Not > sure hr was S_OK after this statement. > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd368217(v=vs.85).aspx Done.
https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win... ui/gfx/platform_font_win.cc:142: // http://crbug.com/434425 If you want to look for specific fonts that are causing this.. add something similar to dwrite_font_platform_win.cc base::debug::SetCrashKeyValue(kFontKeyName, base::WideToUTF8(font_key_name)); https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win... ui/gfx/platform_font_win.cc:164: base::debug::Alias(&fallback_to_matching_font); fallback_to_matching_font = true if SUCCEEDED(hr) No use of Alias then here?
https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win... ui/gfx/platform_font_win.cc:142: // http://crbug.com/434425 On 2014/11/19 23:30:47, Shrikant Kelkar wrote: > If you want to look for specific fonts that are causing this.. add something > similar to dwrite_font_platform_win.cc > > base::debug::SetCrashKeyValue(kFontKeyName, > base::WideToUTF8(font_key_name)); Will do that in a followup. Dunno what the clean up implications are here. https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win... ui/gfx/platform_font_win.cc:164: base::debug::Alias(&fallback_to_matching_font); On 2014/11/19 23:30:47, Shrikant Kelkar wrote: > fallback_to_matching_font = true if SUCCEEDED(hr) > No use of Alias then here? Moved the alias for fallback_to_matching_font above.
On 2014/11/19 23:38:58, ananta wrote: > https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win.cc > File ui/gfx/platform_font_win.cc (right): > > https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win... > ui/gfx/platform_font_win.cc:142: // http://crbug.com/434425 > On 2014/11/19 23:30:47, Shrikant Kelkar wrote: > > If you want to look for specific fonts that are causing this.. add something > > similar to dwrite_font_platform_win.cc > > > > base::debug::SetCrashKeyValue(kFontKeyName, > > base::WideToUTF8(font_key_name)); > > Will do that in a followup. Dunno what the clean up implications are here. > > https://codereview.chromium.org/737103002/diff/60001/ui/gfx/platform_font_win... > ui/gfx/platform_font_win.cc:164: base::debug::Alias(&fallback_to_matching_font); > On 2014/11/19 23:30:47, Shrikant Kelkar wrote: > > fallback_to_matching_font = true if SUCCEEDED(hr) > > No use of Alias then here? > > Moved the alias for fallback_to_matching_font above. lgtm! Point was, you won't see fallback_to_matching_font == true unless you Crash (CHECK(false)) after setting it. So either remove or fail there.
The CQ bit was checked by vitalybuka@chromium.org
The CQ bit was unchecked by vitalybuka@chromium.org
LGTM https://codereview.chromium.org/737103002/diff/120001/ui/gfx/platform_font_wi... File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/120001/ui/gfx/platform_font_wi... ui/gfx/platform_font_win.cc:152: if (FAILED(hr)) { nit: combine into a single if.
https://codereview.chromium.org/737103002/diff/120001/ui/gfx/platform_font_wi... File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/120001/ui/gfx/platform_font_wi... ui/gfx/platform_font_win.cc:152: if (FAILED(hr)) { On 2014/11/20 00:52:57, sky wrote: > nit: combine into a single if. Done.
https://codereview.chromium.org/737103002/diff/120001/ui/gfx/platform_font_wi... File ui/gfx/platform_font_win.cc (right): https://codereview.chromium.org/737103002/diff/120001/ui/gfx/platform_font_wi... ui/gfx/platform_font_win.cc:152: if (FAILED(hr)) { On 2014/11/20 00:52:57, sky wrote: > nit: combine into a single if. Done.
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/737103002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ad29fae1fa8166c5755e9508c59297ddcc4ad198 Cr-Commit-Position: refs/heads/master@{#304981} |
