Pass effects directly to fontcache

Pass effects directly to fontcache

BUG=skia:5176
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1863013003

Committed: https://skia.googlesource.com/skia/+/c79172857c3f69cc46837e1beeae0c1ead377bb2

[reed1, 2016-04-06 12:55:52]

Description was changed from

==========
WIP: idea for passing effects directly to fontcache

BUG=skia:5176
==========

to

==========
WIP: idea for passing effects directly to fontcache

BUG=skia:5176
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[reed1, 2016-04-06 12:56:53]

reed@google.com changed reviewers:
+ bungeman@google.com, djsollen@google.com, fmalita@chromium.org,
mtklein@google.com

[reed1, 2016-04-06 12:56:53]

incomplete and experimental

[f(malita), 2016-04-06 13:28:32]

https://codereview.chromium.org/1863013003/diff/20001/src/core/SkScalerContex...
File src/core/SkScalerContext.cpp (right):

https://codereview.chromium.org/1863013003/diff/20001/src/core/SkScalerContex...
src/core/SkScalerContext.cpp:86: , fRasterizer(effects.fRasterizer)
Is there a way to take advantage of move semantics here (maybe pass effects by
val)?

[reed1, 2016-04-06 16:58:24]

reed@google.com changed reviewers:
+ msarett@google.com

[msarett, 2016-04-06 17:05:38]

This lines up with what I was thinking of what I was thinking.  +1 for
SkScalerContextEffects instead of a bunch of extra parameters.

https://codereview.chromium.org/1863013003/diff/20001/src/core/SkScalerContex...
File src/core/SkScalerContext.cpp (right):

https://codereview.chromium.org/1863013003/diff/20001/src/core/SkScalerContex...
src/core/SkScalerContext.cpp:65: static SkFlattenable* load_flattenable(const
SkDescriptor* desc, uint32_t tag,
I think you can delete this function.

[bungeman-skia, 2016-04-06 17:52:11]

seems like a good idea now. If SkPaint were also ref counted we could do the
same thing with it and not even have a descriptor. Indeed, now that I've written
that, there doesn't seem to be much difference between SkScalerContextEffects +
descriptor and an SkPaint copy, other than that the descriptor makes it easier
to find the scaler context in the cache.

https://codereview.chromium.org/1863013003/diff/20001/src/ports/SkFontHost_ma...
File src/ports/SkFontHost_mac.cpp (right):

https://codereview.chromium.org/1863013003/diff/20001/src/ports/SkFontHost_ma...
src/ports/SkFontHost_mac.cpp:648: SkScalerContext_Mac(const Effects&, const
SkDescriptor*);
I'm assuming you mean SkScalerContextEffects here? (and below)

https://codereview.chromium.org/1863013003/diff/20001/src/ports/SkFontHost_ma...
src/ports/SkFontHost_mac.cpp:1956: SkScalerContext*
SkTypeface_Mac::onCreateScalerContext(const SkDescriptor* desc) const {
And this would need updated? I rather like that the cast happens here as well
(where it's obvious that 'this' is SkTypeface_Mac).

[reed1, 2016-04-06 20:48:21]

rev #3 builds, but I have stubbed out some GPU logic, where I think I'll need to
plumb-through some mutable Effects (not sure yet).

See SK_DUMMY_EFFECTS for the yet-to-be-understood parts

[reed1, 2016-04-07 23:54:52]

Description was changed from

==========
WIP: idea for passing effects directly to fontcache

BUG=skia:5176
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Pass effects directly to fontcache

BUG=skia:5176
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[reed1, 2016-04-07 23:55:40]

ptal -- will update win and freetype scalercontexts next

[reed1, 2016-04-08 00:13:15]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 00:13:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863013003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863013003/60001

[commit-bot: I haz the power, 2016-04-08 00:14:40]

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

[commit-bot: I haz the power, 2016-04-08 00:14:42]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[bungeman-skia, 2016-04-08 14:42:06]

https://codereview.chromium.org/1863013003/diff/60001/src/gpu/text/GrStencilA...
File src/gpu/text/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/1863013003/diff/60001/src/gpu/text/GrStencilA...
src/gpu/text/GrStencilAndCoverTextContext.cpp:542: glyphs =
ctx->resourceProvider()->createGlyphs(fFont.getTypeface(),{},nullptr, fStroke);
This is kind of like passing 'false', at the call site this is hard to tell what
this is. Or we could be even more opaque and try to use {} instead of nullptr
too... (let's not).

[reed1, 2016-04-08 19:24:22]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 19:24:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863013003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863013003/100001

[commit-bot: I haz the power, 2016-04-08 19:32:13]

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

[commit-bot: I haz the power, 2016-04-08 19:32:14]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[reed1, 2016-04-08 19:37:20]

https://codereview.chromium.org/1863013003/diff/60001/src/gpu/text/GrStencilA...
File src/gpu/text/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/1863013003/diff/60001/src/gpu/text/GrStencilA...
src/gpu/text/GrStencilAndCoverTextContext.cpp:542: glyphs =
ctx->resourceProvider()->createGlyphs(fFont.getTypeface(),{},nullptr, fStroke);
On 2016/04/08 14:42:06, bungeman1 wrote:
> This is kind of like passing 'false', at the call site this is hard to tell
what
> this is. Or we could be even more opaque and try to use {} instead of nullptr
> too... (let's not).

Done.

[reed1, 2016-04-08 19:37:27]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 19:37:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863013003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863013003/120001

[reed1, 2016-04-08 19:42:22]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-04-08 19:43:48]

ptal

[reed1, 2016-04-11 11:41:16]

matt's work on drawables is somewhat dependent on this change. Are there other
concerns?

[bungeman-skia, 2016-04-11 14:19:21]

lgtm

https://codereview.chromium.org/1863013003/diff/120001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1863013003/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:352: SkAutoTDelete<SkScalerContext>
ctx(this->createScalerContext({}, desc, true));
nit: It would be nice for this to be SkScalerContextEffects() instead, so we
know what this is at the call site.

[reed1, 2016-04-11 14:40:48]

https://codereview.chromium.org/1863013003/diff/120001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1863013003/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:352: SkAutoTDelete<SkScalerContext>
ctx(this->createScalerContext({}, desc, true));
On 2016/04/11 14:19:21, bungeman1 wrote:
> nit: It would be nice for this to be SkScalerContextEffects() instead, so we
> know what this is at the call site.

Done.
Meant to fix that before, so thanks for checking again.

[reed1, 2016-04-11 14:40:58]

The CQ bit was checked by reed@google.com

[reed1, 2016-04-11 14:40:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com
Link to the patchset: https://codereview.chromium.org/1863013003/#ps140001
(title: "name the empty effects")

[commit-bot: I haz the power, 2016-04-11 14:41:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863013003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863013003/140001

[commit-bot: I haz the power, 2016-04-11 14:51:10]

Description was changed from

==========
Pass effects directly to fontcache

BUG=skia:5176
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Pass effects directly to fontcache

BUG=skia:5176
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/c79172857c3f69cc46837e1beeae0c1ead377bb2
==========

[commit-bot: I haz the power, 2016-04-11 14:51:11]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/c79172857c3f69cc46837e1beeae0c1ead377bb2

[borenet, 2016-04-11 17:25:11]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1872253004/ by borenet@google.com.

The reason for reverting is: Seems to have broken the DEPS roll..

[bungeman-skia, 2016-04-11 18:02:51]

On 2016/04/11 17:25:11, borenet wrote:
> A revert of this CL (patchset #8 id:140001) has been created in
> https://codereview.chromium.org/1872253004/ by mailto:borenet@google.com.
> 
> The reason for reverting is: Seems to have broken the DEPS roll..

Due to Chromium's content\child\font_warmup_win_unittest.cc .