Prepare to enable RenderTextHarfBuzz by default

Prepare to enable RenderTextHarfBuzz by default

- Fix and partially disable the RenderText unit tests to make sure RenderTextHarfBuzz passes the tests. Call EnsureLayout() in GetGlyphBounds() since it has been made public by r252873.

- Add a "--disable-harfbuzz-rendertext" switch to make enabling RTHB by default easier.

BUG=321868
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285761

[ckocagil, 2014-07-25 05:17:26]

Here we go.

[msw, 2014-07-25 08:00:11]

You may find it helpful (as I did for NativeTextfieldViews) to leave the enable*
flags and strings around and use a MULTI_VALUE_TYPE for about_flags. That way,
you'd avoid merge conflicts and only need to modify RenderText::CreateInstance
to revert the default behavior, should the need arise.

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

https://codereview.chromium.org/419753002/diff/1/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:828: #if 0
Leave these tests running with CreateNativeInstance + TODOs.

https://codereview.chromium.org/419753002/diff/1/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:2001: // TODO(ckocagil): Remove when
RenderTextWin goes away.
Does RenderTextHarfBuzz not suffer from the http://crbug.com/278913 defect?

[ckocagil, 2014-07-26 01:55:04]

On 2014/07/25 08:00:11, msw wrote:
> You may find it helpful (as I did for NativeTextfieldViews) to leave the
enable*
> flags and strings around and use a MULTI_VALUE_TYPE for about_flags. That way,
> you'd avoid merge conflicts and only need to modify RenderText::CreateInstance
> to revert the default behavior, should the need arise.

I made it an ENABLE_DISABLE_VALUE_TYPE. IMO we should enable in a separate CL
that only flips the switch, for easy reverting.

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

https://codereview.chromium.org/419753002/diff/1/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:828: #if 0
On 2014/07/25 08:00:10, msw wrote:
> Leave these tests running with CreateNativeInstance + TODOs.

Done.

https://codereview.chromium.org/419753002/diff/1/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:2001: // TODO(ckocagil): Remove when
RenderTextWin goes away.
On 2014/07/25 08:00:10, msw wrote:
> Does RenderTextHarfBuzz not suffer from the http://crbug.com/278913 defect?

No, and there's a HarfBuzz version of this test, namely
HarfBuzz_BreakRunsByUnicodeBlocks.

[msw, 2014-07-26 02:54:14]

LGTM with a nit. Enabling by default in a separate CL sgtm. Update the CL title
and description (perhaps also mention why you added EnsureLayout in
GetGlyphBounds)

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

https://codereview.chromium.org/419753002/diff/50001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:634: Range
RenderTextHarfBuzz::GetGlyphBounds(size_t index) {
nit: reorder this definition to match its declaration order.

[ckocagil, 2014-07-26 03:55:59]

On 2014/07/26 02:54:14, msw wrote:
> LGTM with a nit. Enabling by default in a separate CL sgtm. Update the CL
title
> and description (perhaps also mention why you added EnsureLayout in
> GetGlyphBounds)

Done.

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

https://codereview.chromium.org/419753002/diff/50001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:634: Range
RenderTextHarfBuzz::GetGlyphBounds(size_t index) {
On 2014/07/26 02:54:14, msw wrote:
> nit: reorder this definition to match its declaration order.

Done.

[ckocagil, 2014-07-26 03:56:27]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-07-26 05:00:35]

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

[ckocagil, 2014-07-26 13:33:55]

The CQ bit was unchecked by ckocagil@chromium.org

[ckocagil, 2014-07-26 13:43:37]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-07-26 13:45:02]

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

[ckocagil, 2014-07-26 16:10:24]

The CQ bit was unchecked by ckocagil@chromium.org

[ckocagil, 2014-07-26 16:10:57]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-07-26 16:11:20]

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

[commit-bot: I haz the power, 2014-07-26 16:20:34]

Change committed as 285761