Attempt to improve lifetime management of SkGlyphCache in Ganesh atlas text code.

Attempt to improve lifetime management of SkGlyphCache in Ganesh atlas text code.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1983353003

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

[bsalomon, 2016-05-18 01:47:17]

Description was changed from

==========
Attempt to improve lifetime management of SkGlyphCache lifetime in Ganesh atlas
text code.
==========

to

==========
Attempt to improve lifetime management of SkGlyphCache lifetime in Ganesh atlas
text code.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bsalomon, 2016-05-18 01:53:21]

bsalomon@google.com changed reviewers:
+ bungeman@google.com

[bsalomon, 2016-05-18 01:53:22]

https://codereview.chromium.org/1983353003/diff/1/src/gpu/text/GrAtlasTextBlo...
File src/gpu/text/GrAtlasTextBlob_regenInBatch.cpp (right):

https://codereview.chromium.org/1983353003/diff/1/src/gpu/text/GrAtlasTextBlo...
src/gpu/text/GrAtlasTextBlob_regenInBatch.cpp:146: if (!fCache ||
!SkTypeface::Equal(fTypeface, run->fTypeface) ||
Ben, is it possible for the descriptors to match without the typeface being the
same? From reading the comments on SkGlyphCache I don't think it is.

"""
This class represents a strike: a specific combination of typeface, size,
matrix, etc., and holds the glyphs for that strike.
"""

If that's right then we can simply use SkAutoGlyphCache and get rid of this
LazyGlyphCache object since it is just a SkGlyphCache and SkTypeface and the
SkTypeface::Equal check is the only use of the typeface.

[bsalomon, 2016-05-18 01:55:47]

Description was changed from

==========
Attempt to improve lifetime management of SkGlyphCache lifetime in Ganesh atlas
text code.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Attempt to improve lifetime management of SkGlyphCache in Ganesh atlas text
code.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[reed1, 2016-05-18 13:58:38]

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

[reed1, 2016-05-18 13:58:38]

drive-by lgtm

[bungeman-skia, 2016-05-18 15:33:07]

https://codereview.chromium.org/1983353003/diff/20001/src/gpu/text/GrAtlasTex...
File src/gpu/text/GrAtlasTextBlob.h (right):

https://codereview.chromium.org/1983353003/diff/20001/src/gpu/text/GrAtlasTex...
src/gpu/text/GrAtlasTextBlob.h:569: const SkTypeface*   fTypeface =  nullptr;
nit: why two spaces between '=' and 'nullptr'?

https://codereview.chromium.org/1983353003/diff/20001/src/gpu/text/GrAtlasTex...
File src/gpu/text/GrAtlasTextBlob_regenInBatch.cpp (right):

https://codereview.chromium.org/1983353003/diff/20001/src/gpu/text/GrAtlasTex...
src/gpu/text/GrAtlasTextBlob_regenInBatch.cpp:146: if (!fCache ||
!SkTypeface::Equal(fTypeface, run->fTypeface) ||
if instead of using fTypeface here, we just used
fCache->getScalerContext()->getTypeface() would that be enough? The one issue I
can see is that fCache->getScalerContext()->getTypeface() is always resolved (if
you pass in nullptr this will be the default font). If run->fTypeface is ever
nullptr then this may not be equivalent, I'm not sure if that's allowed or not.
If it does work it means fTypeface isn't needed.

https://codereview.chromium.org/1983353003/diff/20001/src/gpu/text/GrAtlasTex...
src/gpu/text/GrAtlasTextBlob_regenInBatch.cpp:147: fCache->getDescriptor() !=
*newDesc) {
nit: I know it isn't a documented style, but it would be nice if this were more
like

if (!fCache ||
    !SkTypeface::Equal(fTypeface, run->fTypeface) ||
    fCache->getDescriptor() != *newDesc)
{

so that the logic is easier to follow, and so the '{' breaks up the equal
indentation of the conditional and the body. As things are, it's difficult to
visually distinguish between the conditional logic and the body.

[bsalomon, 2016-05-18 20:24:10]

Updated to remove the lazy cache object since we don't really need to retain the
SkTypeface used to find the previous SkGlyphCache.

[bungeman-skia, 2016-05-18 21:18:59]

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

[commit-bot: I haz the power, 2016-05-18 21:19:03]

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

[bungeman-skia, 2016-05-18 21:36:06]

I am going to claim this lgtm, even though it makes me slightly nervous.
https://codereview.chromium.org/1992053002/ would make me slightly less nervous.

[commit-bot: I haz the power, 2016-05-18 21:38:08]

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

[commit-bot: I haz the power, 2016-05-18 21:38:09]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-05-18 22:49:01]

On 2016/05/18 21:36:06, bungeman-skia wrote:
> I am going to claim this lgtm, even though it makes me slightly nervous.
> https://codereview.chromium.org/1992053002/ would make me slightly less
nervous.

Agreed. I'll wait for that to land before landing this.

[bsalomon, 2016-05-19 19:50:40]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-19 19:50:41]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1983353003/#ps40001
(title: "Updated to only compare descriptors")

[commit-bot: I haz the power, 2016-05-19 19:50:51]

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

[commit-bot: I haz the power, 2016-05-19 19:51:49]

Description was changed from

==========
Attempt to improve lifetime management of SkGlyphCache in Ganesh atlas text
code.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Attempt to improve lifetime management of SkGlyphCache in Ganesh atlas text
code.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

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

[commit-bot: I haz the power, 2016-05-19 19:51:50]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/d1c71fd56c293afb986cb5e7546a3a8e4b190d48