Disable subpixel positioning if DirectWrite isn't initialized

Disable subpixel positioning if DirectWrite isn't initialized

BUG=439572
Committed: https://crrev.com/f9c458fb8d7b47305dd9e484c49704ca5577054a
Cr-Commit-Position: refs/heads/master@{#308213}

[ckocagil, 2014-12-10 20:50:23]

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

[ckocagil, 2014-12-10 20:59:11]

PTAL. I relax the test to ignore pixels that have a little bit of paint due to
antialiasing. If we make the run width include those pixels, runs get extra
spaces which looks bad and also a bunch of other tests fail.

Alternatives:
1. Maybe I can keep track of those pixels and add them right before calculating
the entire string width. I'm not sure whether this would pass the tests.

2. Or I can add a different API to query the visual bounds.

[msw, 2014-12-10 23:55:50]

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_win.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_win.cc:38: params_->subpixel_positioning =
win::IsDirectWriteEnabled();
Is this correct, or should we simply not round the run widths for RTHB + GDI?
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/render_text...
Please add a comment here, if this is needed, to explain its reasoning.

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2405: EXPECT_LT(230U,
color_utils::GetLuminanceForColor(color))
Add a comment explaining the lax condition here.

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/win/direct_write.cc
File ui/gfx/win/direct_write.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/win/direct_write....
ui/gfx/win/direct_write.cc:114: dwrite_enabled = true;
Why can't callers check ShouldUseDirectWrite?

[ckocagil, 2014-12-11 01:07:11]

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_win.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_win.cc:38: params_->subpixel_positioning =
win::IsDirectWriteEnabled();
On 2014/12/10 23:55:50, msw wrote:
> Is this correct, or should we simply not round the run widths for RTHB + GDI?

Rounding in RTHB and letting Skia do the rounding gives different results. When
we round in RTHB, we always start glyphs at integers. I remember trying both,
and ours giving visually better results as well as matching the native behavior.

I don't think we need a comment here though. DirectWrite supports subpixel
positioning, GDI does not. Whether we round ourselves or let Skia do it isn't a
concern here.

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/win/direct_write.cc
File ui/gfx/win/direct_write.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/win/direct_write....
ui/gfx/win/direct_write.cc:114: dwrite_enabled = true;
On 2014/12/10 23:55:50, msw wrote:
> Why can't callers check ShouldUseDirectWrite?

Because some processes can choose not to initialize DirectWrite even if
ShouldUseDirectWrite is true. For example DirectWrite is always disabled in unit
tests.

[msw, 2014-12-11 23:30:13]

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_win.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_win.cc:38: params_->subpixel_positioning =
win::IsDirectWriteEnabled();
On 2014/12/11 01:07:11, ckocagil wrote:
> On 2014/12/10 23:55:50, msw wrote:
> > Is this correct, or should we simply not round the run widths for RTHB +
GDI?
> 
> Rounding in RTHB and letting Skia do the rounding gives different results.
When
> we round in RTHB, we always start glyphs at integers. I remember trying both,
> and ours giving visually better results as well as matching the native
behavior.
> 
> I don't think we need a comment here though. DirectWrite supports subpixel
> positioning, GDI does not. Whether we round ourselves or let Skia do it isn't
a
> concern here.

Optionally: // GDI does not support subpixel positioning.

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2405: EXPECT_LT(230U,
color_utils::GetLuminanceForColor(color))
On 2014/12/10 23:55:50, msw wrote:
> Add a comment explaining the lax condition here.

Should this perhaps just allow the first column past the expected width to have
anti-aliased content? Optionally: "// Allow [a single column of] anti-aliased
pixels past the expected width."

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/win/direct_write.cc
File ui/gfx/win/direct_write.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/win/direct_write....
ui/gfx/win/direct_write.cc:114: dwrite_enabled = true;
On 2014/12/11 01:07:11, ckocagil wrote:
> On 2014/12/10 23:55:50, msw wrote:
> > Why can't callers check ShouldUseDirectWrite?
> 
> Because some processes can choose not to initialize DirectWrite even if
> ShouldUseDirectWrite is true. For example DirectWrite is always disabled in
unit
> tests.

Okay, fair enough.

[ckocagil, 2014-12-12 09:52:11]

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_win.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_win.cc:38: params_->subpixel_positioning =
win::IsDirectWriteEnabled();
On 2014/12/11 23:30:12, msw wrote:
> On 2014/12/11 01:07:11, ckocagil wrote:
> > On 2014/12/10 23:55:50, msw wrote:
> > > Is this correct, or should we simply not round the run widths for RTHB +
> GDI?
> > 
> > Rounding in RTHB and letting Skia do the rounding gives different results.
> When
> > we round in RTHB, we always start glyphs at integers. I remember trying
both,
> > and ours giving visually better results as well as matching the native
> behavior.
> > 
> > I don't think we need a comment here though. DirectWrite supports subpixel
> > positioning, GDI does not. Whether we round ourselves or let Skia do it
isn't
> a
> > concern here.
> 
> Optionally: // GDI does not support subpixel positioning.

Done.

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/785763002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2405: EXPECT_LT(230U,
color_utils::GetLuminanceForColor(color))
On 2014/12/11 23:30:12, msw wrote:
> On 2014/12/10 23:55:50, msw wrote:
> > Add a comment explaining the lax condition here.
> 
> Should this perhaps just allow the first column past the expected width to
have
> anti-aliased content? Optionally: "// Allow [a single column of] anti-aliased
> pixels past the expected width."

Done.

[msw, 2014-12-12 18:58:43]

lgtm with optional nits.

https://codereview.chromium.org/785763002/diff/60001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/785763002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2401: ASSERT_GT(kCanvasSize.width() - kTestWidth,
width);
optional nit: this might be a bit clearer:
  ASSERT_LT(width + kTestWidth, kCanvasSize.width())

https://codereview.chromium.org/785763002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2407: for (int x = 0; x < kTestWidth; ++x) {
optional nit: this might be a bit clearer:
  for (int y = 0; y < kCanvasSize.height(); ++y) {
    // Allow one column of anti-aliased pixels past the expected width.
    SkColor color = buffer[width + y * kCanvasSize.width()];
    EXPECT_LT(230U, color_utils::GetLuminanceForColor(color)) << string;
    for (int x = 1; x < kTestWidth; ++x) {
      color = buffer[width + x + y * kCanvasSize.width()];
      EXPECT_EQ(SK_ColorWHITE, color) << string;
    }
  }

[ckocagil, 2014-12-12 22:16:24]

https://codereview.chromium.org/785763002/diff/60001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/785763002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2401: ASSERT_GT(kCanvasSize.width() - kTestWidth,
width);
On 2014/12/12 18:58:43, msw wrote:
> optional nit: this might be a bit clearer:
>   ASSERT_LT(width + kTestWidth, kCanvasSize.width())

Done.

https://codereview.chromium.org/785763002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2407: for (int x = 0; x < kTestWidth; ++x) {
On 2014/12/12 18:58:43, msw wrote:
> optional nit: this might be a bit clearer:
>   for (int y = 0; y < kCanvasSize.height(); ++y) {
>     // Allow one column of anti-aliased pixels past the expected width.
>     SkColor color = buffer[width + y * kCanvasSize.width()];
>     EXPECT_LT(230U, color_utils::GetLuminanceForColor(color)) << string;
>     for (int x = 1; x < kTestWidth; ++x) {
>       color = buffer[width + x + y * kCanvasSize.width()];
>       EXPECT_EQ(SK_ColorWHITE, color) << string;
>     }
>   }

Done.

[ckocagil, 2014-12-12 22:16:27]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-12-12 22:16:51]

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

[commit-bot: I haz the power, 2014-12-12 22:21:57]

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

[commit-bot: I haz the power, 2014-12-12 22:21:57]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_compile_dbg_32_ng 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...)
  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_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ckocagil, 2014-12-13 00:00:35]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-12-13 00:02:27]

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

[commit-bot: I haz the power, 2014-12-13 01:25:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-13 01:26:13]

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