Remove the code which sorts and removes duplicates from the fallback font list

Remove the code which sorts and removes duplicates from the fallback font list

This causes wrong fallback fonts to be picked leading to text displaying badly.

To avoid duplicate fonts from being processed, the proposal is to use a set to track those
and skip them.

BUG=470397
Committed: https://crrev.com/46b2013e68f391dc8c82c12cb26f1053b63c12ec
Cr-Commit-Position: refs/heads/master@{#322320}

[ananta, 2015-03-25 21:58:23]

ananta@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2015-03-25 22:55:29]

lgtm with nits; remember to merge after verifying.

https://codereview.chromium.org/1037863002/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1037863002/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1310: // The fallback font list could contain
duplicate fonts. To avoid processing
nit: "// Use a set to track the fallback fonts and avoid duplicate entries."

https://codereview.chromium.org/1037863002/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1323: if
(duplicate_fallback_font_tracker.find(family) !=
nit: should this use LowerCaseEqualsASCII?

[ananta, 2015-03-25 23:19:48]

https://codereview.chromium.org/1037863002/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1037863002/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1310: // The fallback font list could contain
duplicate fonts. To avoid processing
On 2015/03/25 22:55:29, msw wrote:
> nit: "// Use a set to track the fallback fonts and avoid duplicate entries."

Done.

https://codereview.chromium.org/1037863002/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1323: if
(duplicate_fallback_font_tracker.find(family) !=
On 2015/03/25 22:55:29, msw wrote:
> nit: should this use LowerCaseEqualsASCII?

Passed a case insensitive comparison operator to the set.

[msw, 2015-03-25 23:23:42]

https://codereview.chromium.org/1037863002/diff/20001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1037863002/diff/20001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:474: return base::strncasecmp(a.c_str(),
b.c_str(), b.length()) < 0;
Will strncasecmp work if a.length() != b.length()? Maybe use strcasecmp?

https://codereview.chromium.org/1037863002/diff/20001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:1319: duplicate_fallback_font_tracker;
optional nit: rename |tried_fallback_fonts| or similar.

[ananta, 2015-03-25 23:39:49]

https://codereview.chromium.org/1037863002/diff/20001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1037863002/diff/20001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:474: return base::strncasecmp(a.c_str(),
b.c_str(), b.length()) < 0;
On 2015/03/25 23:23:42, msw wrote:
> Will strncasecmp work if a.length() != b.length()? Maybe use strcasecmp? 

It should. As per docs, it compares upto count characters and stops if
a NULL is seen in either string.

https://codereview.chromium.org/1037863002/diff/20001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:1319: duplicate_fallback_font_tracker;
On 2015/03/25 23:23:42, msw wrote:
> optional nit: rename |tried_fallback_fonts| or similar.

Renamed to duplicate_fallback_fonts

[msw, 2015-03-25 23:42:07]

https://codereview.chromium.org/1037863002/diff/20001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1037863002/diff/20001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:1319: duplicate_fallback_font_tracker;
On 2015/03/25 23:39:49, ananta wrote:
> On 2015/03/25 23:23:42, msw wrote:
> > optional nit: rename |tried_fallback_fonts| or similar.
> 
> Renamed to duplicate_fallback_fonts

Eh, they aren't duplicate themselves... the name |fallback_fonts| would be okay
with me, given the comment and simple usage.

https://codereview.chromium.org/1037863002/diff/40001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1037863002/diff/40001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:1329: duplicate_fallback_fonts.end())
nit: fits on line above.

[ananta, 2015-03-25 23:47:27]

https://codereview.chromium.org/1037863002/diff/40001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1037863002/diff/40001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:1329: duplicate_fallback_fonts.end())
On 2015/03/25 23:42:07, msw wrote:
> nit: fits on line above.

Done.

[msw, 2015-03-25 23:51:10]

lgtm!

[ananta, 2015-03-26 01:17:41]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-03-26 01:18:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1037863002/60001

[commit-bot: I haz the power, 2015-03-26 04:38:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-26 04:39:31]

Patchset 4 (id:??) landed as
https://crrev.com/46b2013e68f391dc8c82c12cb26f1053b63c12ec
Cr-Commit-Position: refs/heads/master@{#322320}