RenderTextHarfBuzz: Break runs at parentheses so they aren't affected by font fallbacks

RenderTextHarfBuzz: Break runs at parentheses so they aren't affected by font fallbacks

BUG=396776
R=msw@chromium.org, msw

Committed: https://chromium.googlesource.com/chromium/src/+/f8d6f6599c9ee9d674b0eabc8df552d2daa9ae59

[msw, 2014-09-02 18:20:25]

Mostly minor comments, thanks!

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

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:311: static const char kBrackets[] = { '(', ')',
'{', '}', '<', '>' };
nit: add a comma after the last element.

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:312: static const char* kBrackets_end = kBrackets
+ arraysize(kBrackets);
nit: kBracketsEnd.

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:318: const bool first_parenthesis =
nit: |first_bracket| to match |kBrackets|; ditto for |current_*|.

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:319: std::find(kBrackets, kBrackets_end,
first_char) != kBrackets_end;
How about adding a helper function IsBracket[Character] or similar?

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1040: // This is a workaround until we can do
partial font fallback in runs. Break
I'm not sure we'll want to do partial font fallback in runs, perhaps remove that
first sentence.

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1042: // either an unusual character from forcing
a fallback on the entire run,
nit: "fallback font" here and below.

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

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:1617: // TODO(ckocagil): Enable for
RenderTextHarfBuzz. http://crbug.com/396776
Can you remove this TODO now?

[ckocagil, 2014-09-03 01:22:30]

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

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:311: static const char kBrackets[] = { '(', ')',
'{', '}', '<', '>' };
On 2014/09/02 18:20:25, msw wrote:
> nit: add a comma after the last element.

Done.

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:312: static const char* kBrackets_end = kBrackets
+ arraysize(kBrackets);
On 2014/09/02 18:20:25, msw wrote:
> nit: kBracketsEnd.

Done.

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:318: const bool first_parenthesis =
On 2014/09/02 18:20:25, msw wrote:
> nit: |first_bracket| to match |kBrackets|; ditto for |current_*|.

Done.

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:319: std::find(kBrackets, kBrackets_end,
first_char) != kBrackets_end;
On 2014/09/02 18:20:25, msw wrote:
> How about adding a helper function IsBracket[Character] or similar?

Done.

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1040: // This is a workaround until we can do
partial font fallback in runs. Break
On 2014/09/02 18:20:25, msw wrote:
> I'm not sure we'll want to do partial font fallback in runs, perhaps remove
that
> first sentence.

I'd like to go ahead and implement partial font fallback after RTHB is enabled
and all good, but that's a different topic. Removed.

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1042: // either an unusual character from forcing
a fallback on the entire run,
On 2014/09/02 18:20:25, msw wrote:
> nit: "fallback font" here and below.

Done.

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

https://codereview.chromium.org/522343002/diff/1/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:1617: // TODO(ckocagil): Enable for
RenderTextHarfBuzz. http://crbug.com/396776
On 2014/09/02 18:20:25, msw wrote:
> Can you remove this TODO now?

Done.

[msw, 2014-09-03 18:14:58]

lgtm with a nit; thanks!

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

https://codereview.chromium.org/522343002/diff/20001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:321: const bool first_bracket =
IsBracket(first_char);
nit: order this before or after the |first_block[_unusual]| lines.

[ckocagil, 2014-09-04 07:21:48]

On 2014/09/03 18:14:58, msw wrote:
> lgtm with a nit; thanks!
> 
>
https://codereview.chromium.org/522343002/diff/20001/ui/gfx/render_text_harfb...
> File ui/gfx/render_text_harfbuzz.cc (right):
> 
>
https://codereview.chromium.org/522343002/diff/20001/ui/gfx/render_text_harfb...
> ui/gfx/render_text_harfbuzz.cc:321: const bool first_bracket =
> IsBracket(first_char);
> nit: order this before or after the |first_block[_unusual]| lines.

Done, landing.

[ckocagil, 2014-09-04 07:21:52]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-09-04 07:22:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckocagil@chromium.org/522343002/40001

[commit-bot: I haz the power, 2014-09-04 08:15:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 08:15:44]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[ckocagil, 2014-09-04 15:05:14]

Committed patchset #3 (id:40001) manually as f8d6f65 (presubmit successful).

[commit-bot: I haz the power, 2014-09-10 03:31:26]

Patchset 3 (id:??) landed as
https://crrev.com/f8d6f6599c9ee9d674b0eabc8df552d2daa9ae59
Cr-Commit-Position: refs/heads/master@{#293299}