Add flag to enable rendering of text using signed distance fields.

Add flag to enable rendering of text using signed distance fields.

This change plumbs through a flag that turns on distance
field text in Skia when doing GPU rasterization, as requested
by vangelis@chromium.org.

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

[Vangelis Kokkevis, 2014-05-05 18:33:22]

The ResourceProvider is currently dealing mostly with backing surfaces for tiles
so it seems a bit odd to add the DFF setting there. I've cc-ed a few more folks
who can answer that question better.

I think the alternative could be to set the value on the SkPaint objects at
record time and ignore it at playback if we're not in Ganesh raster mode.

[jvanverth1, 2014-05-05 18:58:22]

On 2014/05/05 18:33:22, Vangelis Kokkevis wrote:
> The ResourceProvider is currently dealing mostly with backing surfaces for
tiles
> so it seems a bit odd to add the DFF setting there. I've cc-ed a few more
folks
> who can answer that question better.
> 
> I think the alternative could be to set the value on the SkPaint objects at
> record time and ignore it at playback if we're not in Ganesh raster mode.

We'd prefer to get rid of the flag in SkPaint, as it requires us to always set
up a GrDistanceFieldTextContext in case it needs be used. We'd rather drive that
by passing in the TextRenderMode flag to SkSurface::NewRenderTarget() and its
variants, if possible.

[enne (OOO), 2014-05-05 21:01:01]

If it needs to get set in SkSurface::NewRenderTarget, then I don't see a better
way to plumb this through than the way it is now.

[piman, 2014-05-05 21:04:54]

https://codereview.chromium.org/266743003/diff/60001/content/public/common/co...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/266743003/diff/60001/content/public/common/co...
content/public/common/content_switches.h:49: CONTENT_EXPORT extern const char
kDisableDistanceFieldText[];
Can you add those to chrome/browser/chromeos/login/chrome_restart_request.cc so
that they also apply to Chrome OS guest mode?

[alokp, 2014-05-05 21:07:38]

It looks ugly but necessary for now. Once RasterBuffer moves out of
ResourceProvider to various RasterWorkerPool implementations, it will be less
objectionable.

[reveman, 2014-05-05 21:15:18]

https://codereview.chromium.org/266743003/diff/60001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/266743003/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.h:496: bool use_distance_field_text_;
Can this be "const bool" and set by the ctor instead of having
SetUseDistanceFieldText?

[jvanverth1, 2014-05-06 14:27:21]

I also fixed a bug in RenderThreadImpl::Init() which would have turned on
distance field text all the time.

PTAL

https://codereview.chromium.org/266743003/diff/60001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/266743003/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.h:496: bool use_distance_field_text_;
On 2014/05/05 21:15:18, reveman wrote:
> Can this be "const bool" and set by the ctor instead of having
> SetUseDistanceFieldText?

Done.

https://codereview.chromium.org/266743003/diff/60001/content/public/common/co...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/266743003/diff/60001/content/public/common/co...
content/public/common/content_switches.h:49: CONTENT_EXPORT extern const char
kDisableDistanceFieldText[];
On 2014/05/05 21:04:55, piman wrote:
> Can you add those to chrome/browser/chromeos/login/chrome_restart_request.cc
so
> that they also apply to Chrome OS guest mode?

Done.

[piman, 2014-05-06 17:58:50]

lgtm

[enne (OOO), 2014-05-06 18:19:54]

lgtm

Please format your change description to wrap at 72 columns, if you don't mind.

https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
content/renderer/render_thread_impl.cc:438: is_gpu_rasterization_enabled_ ||
is_gpu_rasterization_forced_;
I don't really like flags turning on other features indirectly unless it's a
requirement.  Can you remove the is_gpu_rasterization_forced_ clause?

[jvanverth1, 2014-05-06 18:30:31]

On 2014/05/06 18:19:54, enne wrote:
> lgtm
> 
> Please format your change description to wrap at 72 columns, if you don't
mind.

Done.

>
https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
> content/renderer/render_thread_impl.cc:438: is_gpu_rasterization_enabled_ ||
> is_gpu_rasterization_forced_;
> I don't really like flags turning on other features indirectly unless it's a
> requirement.  Can you remove the is_gpu_rasterization_forced_ clause?

Just to clarify -- change the entire two lines to
"is_distance_field_text_enabled_ = true;", or just remove the
is_gpu_rasterization_forced_ part? My thinking there was that distance field
text only works if GPU rasterization is enabled, but I don't have a strong
opinion about it.

[enne (OOO), 2014-05-06 18:38:04]

>
https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
> > File content/renderer/render_thread_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
> > content/renderer/render_thread_impl.cc:438: is_gpu_rasterization_enabled_ ||
> > is_gpu_rasterization_forced_;
> > I don't really like flags turning on other features indirectly unless it's a
> > requirement.  Can you remove the is_gpu_rasterization_forced_ clause?
> 
> Just to clarify -- change the entire two lines to
> "is_distance_field_text_enabled_ = true;", or just remove the
> is_gpu_rasterization_forced_ part? My thinking there was that distance field
> text only works if GPU rasterization is enabled, but I don't have a strong
> opinion about it.

If it's a no-op to have it enabled without GPU rasterization, then I'd say just
change it to "true".  Your flag checking misses a bunch of cases from
IsGpuRasterizationEnabled(), so you might as well just skip the checks entirely
if it's not harmful to have it turned on.

[alokp, 2014-05-06 18:41:10]

lgtm

https://codereview.chromium.org/266743003/diff/80001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/266743003/diff/80001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:472: SkSurface::TextRenderMode trm =
nit: style guide does not like abbreviations.

https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
content/renderer/render_thread_impl.cc:438: is_gpu_rasterization_enabled_ ||
is_gpu_rasterization_forced_;
I agree. This flag will be ignored anyway if gpu-rasterization is not used.

[jvanverth1, 2014-05-06 20:09:38]

https://codereview.chromium.org/266743003/diff/80001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/266743003/diff/80001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:472: SkSurface::TextRenderMode trm =
On 2014/05/06 18:41:11, Alok Priyadarshi wrote:
> nit: style guide does not like abbreviations.

Done.

https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/266743003/diff/80001/content/renderer/render_...
content/renderer/render_thread_impl.cc:438: is_gpu_rasterization_enabled_ ||
is_gpu_rasterization_forced_;
On 2014/05/06 18:19:54, enne wrote:
> I don't really like flags turning on other features indirectly unless it's a
> requirement.  Can you remove the is_gpu_rasterization_forced_ clause?

Done.

[jvanverth1, 2014-05-06 20:14:19]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-05-06 20:16:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jvanverth@google.com/266743003/90001

[jvanverth1, 2014-05-06 21:07:54]

The CQ bit was unchecked by jvanverth@google.com

[jvanverth1, 2014-05-06 21:55:42]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-05-06 21:56:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jvanverth@google.com/266743003/110001

[commit-bot: I haz the power, 2014-05-07 14:48:53]

Change committed as 268758