Remove deferred fade shader workaround from RenderText

Remove deferred fade shader workaround from RenderText

BUG=388084
Committed: https://crrev.com/ff68cff3bf62886d125ce8ae385e7084face6e39
Cr-Commit-Position: refs/heads/master@{#292650}

[Tomasz Moniuszko, 2014-08-27 07:20:35]

tmoniuszko@opera.com changed reviewers:
+ msw@chromium.org

[msw, 2014-08-27 19:16:40]

I wonder if this is the right fix, given the work done in 
<https://code.google.com/p/skia/issues/detail?id=590>. Perhaps
SkGradientShader::CreateLinear (via ui/gfx/render_text.cc's CreateFadeShader)
does the right thing as-is after picking up that Skia fix by updating our DEPs?
(if so, perhaps this fix wouldn't be needed). What do you (or other CC'ed folks)
think?

Also, note that I dup'ed crbug.com/407654 to crbug.com/388084, please update the
BUG= accordingly, thanks!

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

https://codereview.chromium.org/493363006/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:221: paint_.setFlags(SkSetClearMask(paint_.getFlags(),
background_is_transparent,
Can you make this part of SetFontRenderParams?

https://codereview.chromium.org/493363006/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:222: SkPaint::kGenA8FromLCD_Flag));
This is cited as a "hack for GDI -- do not use if you can help it" by
third_party/skia/include/core/SkFont.h. Put a comment here citing that we should
remove this hack when we switch to DirectWrite on Windows; but maybe we can't
actually remove it entirely until we drop support for Windows machines without
DirectWrite support (Win XP and maybe some Vista systems only have GDI, afaik).

https://codereview.chromium.org/493363006/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:269: // Work-around for http://crbug.com/122743, where
non-ClearType text is
Can we remove this workaround given the changes in this CL?

[Tomasz Moniuszko, 2014-08-29 13:11:23]

On 2014/08/27 19:16:40, msw wrote:
> I wonder if this is the right fix, given the work done in 
> <https://code.google.com/p/skia/issues/detail?id=590>. Perhaps
> SkGradientShader::CreateLinear (via ui/gfx/render_text.cc's CreateFadeShader)
> does the right thing as-is after picking up that Skia fix by updating our
DEPs?
> (if so, perhaps this fix wouldn't be needed). What do you (or other CC'ed
folks)
> think?

You are right. My patch has no effect now. The issue fixed by patch in Skia
already.

> 
> Also, note that I dup'ed crbug.com/407654 to crbug.com/388084, please update
the
> BUG= accordingly, thanks!

Done.

>
https://codereview.chromium.org/493363006/diff/1/ui/gfx/render_text.cc#newcod...
> ui/gfx/render_text.cc:269: // Work-around for http://crbug.com/122743, where
> non-ClearType text is
> Can we remove this workaround given the changes in this CL?

I removed the work-around in Patch Set 2. Works well without it now.

[Alexei Svitkine (slow), 2014-08-29 14:19:25]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-08-29 14:19:25]

sweet, lgtm!

[Tomasz Moniuszko, 2014-08-29 14:33:38]

The CQ bit was checked by tmoniuszko@opera.com

[commit-bot: I haz the power, 2014-08-29 14:34:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmoniuszko@opera.com/493363006/20001

[commit-bot: I haz the power, 2014-08-29 15:46:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-29 16:32:54]

Committed patchset #2 (id:20001) as 7b27f09661cf8814103dee68a36fb3d59533d0b0

[msw, 2014-08-29 17:00:33]

Thanks for cleaning up the workaround, just a could comments, would you mind
landing a small followup CL?

https://codereview.chromium.org/493363006/diff/20001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/493363006/diff/20001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:21: #include "third_party/skia/include/core/SkRect.h"
This is no longer needed (at least for the header), can you remove it or move it
to the code file?

https://codereview.chromium.org/493363006/diff/20001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:105: bool started_drawing_;
This is now unused, can you remove this?

[Tomasz Moniuszko, 2014-09-02 14:55:15]

On 2014/08/29 17:00:33, msw wrote:
> Thanks for cleaning up the workaround, just a could comments, would you mind
> landing a small followup CL?
> 
> https://codereview.chromium.org/493363006/diff/20001/ui/gfx/render_text.h
> File ui/gfx/render_text.h (right):
> 
>
https://codereview.chromium.org/493363006/diff/20001/ui/gfx/render_text.h#new...
> ui/gfx/render_text.h:21: #include "third_party/skia/include/core/SkRect.h"
> This is no longer needed (at least for the header), can you remove it or move
it
> to the code file?
> 
>
https://codereview.chromium.org/493363006/diff/20001/ui/gfx/render_text.h#new...
> ui/gfx/render_text.h:105: bool started_drawing_;
> This is now unused, can you remove this?

Done in https://codereview.chromium.org/493363006/.

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

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