SkAdvancedTypefaceMetrics: getAdvanceData uses std::function

SkAdvancedTypefaceMetrics: getAdvanceData uses std::function

Reduce templatedness.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1955053002

Committed: https://skia.googlesource.com/skia/+/57cd94a9defda8fb37913049f80544d292d99232

[hal.canary, 2016-05-06 15:41:19]

Description was changed from

==========
SkAdvancedTypefaceMetrics: getAdvanceData uses std::function

Reduce templatedness.
==========

to

==========
SkAdvancedTypefaceMetrics: getAdvanceData uses std::function

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

[hal.canary, 2016-05-06 15:45:41]

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

[commit-bot: I haz the power, 2016-05-06 15:45:55]

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

[hal.canary, 2016-05-06 15:49:53]

halcanary@google.com changed reviewers:
+ herb@google.com, tomhudson@google.com

[hal.canary, 2016-05-06 15:49:54]

PTAL

[herb_g, 2016-05-06 15:54:16]

https://codereview.chromium.org/1955053002/diff/1/src/ports/SkFontHost_FreeTy...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/1955053002/diff/1/src/ports/SkFontHost_FreeTy...
src/ports/SkFontHost_FreeType.cpp:617: std::function<bool(int gId, int16_t*
data)> getWidthAdvanceFn =
Use auto instead of std::function...
Here and other places.

[tomhudson, 2016-05-06 15:56:38]

OK, this is more type-safe than the version that used void*, but lambdas? Please
*at least* update the comment for getAdvanceData(), which didn't even mention
getAdvance().

[tomhudson, 2016-05-06 15:58:02]

https://codereview.chromium.org/1955053002/diff/1/src/ports/SkFontHost_FreeTy...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/1955053002/diff/1/src/ports/SkFontHost_FreeTy...
src/ports/SkFontHost_FreeType.cpp:617: std::function<bool(int gId, int16_t*
data)> getWidthAdvanceFn =
On 2016/05/06 15:54:16, herb_g wrote:
> Use auto instead of std::function...
> Here and other places.

Nit: with auto, the return value is totally implicit, even if the parameter
types are still easily inspectable?

[hal.canary, 2016-05-06 16:00:33]

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

[commit-bot: I haz the power, 2016-05-06 16:00:42]

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

[hal.canary, 2016-05-06 16:00:56]

https://codereview.chromium.org/1955053002/diff/1/src/ports/SkFontHost_FreeTy...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/1955053002/diff/1/src/ports/SkFontHost_FreeTy...
src/ports/SkFontHost_FreeType.cpp:617: std::function<bool(int gId, int16_t*
data)> getWidthAdvanceFn =
On 2016/05/06 15:54:16, herb_g wrote:
> Use auto instead of std::function...
> Here and other places.

Done.

[hal.canary, 2016-05-06 16:01:31]

On 2016/05/06 15:56:38, tomhudson wrote:
> Please *at least* update the comment for getAdvanceData(), which
> didn't even mention getAdvance().

Done.

[hal.canary, 2016-05-06 16:01:43]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-05-06 16:03:12]

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

[commit-bot: I haz the power, 2016-05-06 16:03:13]

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...)

[hal.canary, 2016-05-06 16:08:58]

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

[commit-bot: I haz the power, 2016-05-06 16:09:02]

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

[commit-bot: I haz the power, 2016-05-06 16:33:14]

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

[commit-bot: I haz the power, 2016-05-06 16:33:15]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2016-05-09 00:14:08]

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

[bungeman-skia, 2016-05-09 00:14:09]

It generally seems like a good idea.

https://codereview.chromium.org/1955053002/diff/60001/src/ports/SkFontHost_Fr...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/1955053002/diff/60001/src/ports/SkFontHost_Fr...
src/ports/SkFontHost_FreeType.cpp:618: [face](int gId, int16_t* data) {
I kinda understand why, but this indentation is just weird. The brace is at the
end of this line but the beginning of the line doesn't line up with the closing
brace below. I think this awkwardness (and the somewhat awkward name
'getWidthAdvanceFn') can be alleviated by just not naming it.

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

https://codereview.chromium.org/1955053002/diff/60001/src/ports/SkFontHost_ma...
src/ports/SkFontHost_mac.cpp:1628: std::function<bool(int gId, int16_t* data)>
getWidthAdvanceFn =
There's no particular reason to give these function objects names, this could
all be

            CTFontRef borrowedCTFont = ctFont.get();
           
info->fGlyphWidths.reset(skia_advanced_typeface_metrics_utils::getAdvanceData(
                SkToInt(glyphCount),
                glyphIDs,
                glyphIDsCount,
                std::function<bool(int, int16_t*)>([borrowedCTFont](int gId,
int16_t* data) {
                    CGSize advance;
                    advance.width = 0;
                    CGGlyph glyph = gId;
                    CTFontGetAdvancesForGlyphs(borrowedCTFont,
kCTFontHorizontalOrientation,
                                               &glyph, &advance, 1);
                    *data = sk_float_round2int(advance.width);
                    return true;
                })
            ));

Which also inlines the actual work, which only wasn't being done before because
lambdas didn't exist at the time. Now that we're using a lambda, it seems
awkward to leave the work out of line.

[hal.canary, 2016-05-09 12:13:52]

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

[commit-bot: I haz the power, 2016-05-09 12:14:03]

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

[hal.canary, 2016-05-09 12:14:26]

Patchset #2 (id:40001) has been deleted

[hal.canary, 2016-05-09 12:15:08]

https://codereview.chromium.org/1955053002/diff/60001/src/ports/SkFontHost_Fr...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/1955053002/diff/60001/src/ports/SkFontHost_Fr...
src/ports/SkFontHost_FreeType.cpp:618: [face](int gId, int16_t* data) {
On 2016/05/09 00:14:08, bungeman-skia wrote:
> I kinda understand why, but this indentation is just weird. The brace is at
the
> end of this line but the beginning of the line doesn't line up with the
closing
> brace below. I think this awkwardness (and the somewhat awkward name
> 'getWidthAdvanceFn') can be alleviated by just not naming it.

Done.

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

https://codereview.chromium.org/1955053002/diff/60001/src/ports/SkFontHost_ma...
src/ports/SkFontHost_mac.cpp:1628: std::function<bool(int gId, int16_t* data)>
getWidthAdvanceFn =
On 2016/05/09 00:14:08, bungeman-skia wrote:
> There's no particular reason to give these function objects names, this could
> all be
> 
>             CTFontRef borrowedCTFont = ctFont.get();
>            
> info->fGlyphWidths.reset(skia_advanced_typeface_metrics_utils::getAdvanceData(
>                 SkToInt(glyphCount),
>                 glyphIDs,
>                 glyphIDsCount,
>                 std::function<bool(int, int16_t*)>([borrowedCTFont](int gId,
> int16_t* data) {
>                     CGSize advance;
>                     advance.width = 0;
>                     CGGlyph glyph = gId;
>                     CTFontGetAdvancesForGlyphs(borrowedCTFont,
> kCTFontHorizontalOrientation,
>                                                &glyph, &advance, 1);
>                     *data = sk_float_round2int(advance.width);
>                     return true;
>                 })
>             ));
> 
> Which also inlines the actual work, which only wasn't being done before
because
> lambdas didn't exist at the time. Now that we're using a lambda, it seems
> awkward to leave the work out of line.

Done.

[commit-bot: I haz the power, 2016-05-09 12:18:13]

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

[commit-bot: I haz the power, 2016-05-09 12:18:14]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  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...)

[tomhudson, 2016-05-09 13:01:56]

Don't entirely agree with Ben, but sure, if we're going to go lambda we should
go lambda all the way.

https://codereview.chromium.org/1955053002/diff/80001/src/core/SkAdvancedType...
File src/core/SkAdvancedTypefaceMetrics.h (right):

https://codereview.chromium.org/1955053002/diff/80001/src/core/SkAdvancedType...
src/core/SkAdvancedTypefaceMetrics.h:61: std::function<bool(int gId, int16_t*
data)> getAdvance);
This signature appears six times; worth a typedef?

[hal.canary, 2016-05-09 13:08:24]

Patchset #3 (id:80001) has been deleted

[hal.canary, 2016-05-09 13:08:39]

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

[commit-bot: I haz the power, 2016-05-09 13:08:44]

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

[hal.canary, 2016-05-09 13:13:45]

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

[commit-bot: I haz the power, 2016-05-09 13:13:57]

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

[hal.canary, 2016-05-09 13:19:11]

On 2016/05/09 13:01:56, tomhudson wrote:
> Don't entirely agree with Ben, but sure, if we're going to go lambda we should
> go lambda all the way.
> 
>
https://codereview.chromium.org/1955053002/diff/80001/src/core/SkAdvancedType...
> File src/core/SkAdvancedTypefaceMetrics.h (right):
> 
>
https://codereview.chromium.org/1955053002/diff/80001/src/core/SkAdvancedType...
> src/core/SkAdvancedTypefaceMetrics.h:61: std::function<bool(int gId, int16_t*
> data)> getAdvance);
> This signature appears six times; worth a typedef?

Done

[tomhudson, 2016-05-09 13:25:18]

lgtm

[commit-bot: I haz the power, 2016-05-09 13:32:45]

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

[commit-bot: I haz the power, 2016-05-09 13:32:47]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2016-05-09 13:33:42]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2016-05-09 13:33:44]

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

[commit-bot: I haz the power, 2016-05-09 13:34:42]

Description was changed from

==========
SkAdvancedTypefaceMetrics: getAdvanceData uses std::function

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

to

==========
SkAdvancedTypefaceMetrics: getAdvanceData uses std::function

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

Committed:
https://skia.googlesource.com/skia/+/57cd94a9defda8fb37913049f80544d292d99232
==========

[commit-bot: I haz the power, 2016-05-09 13:34:44]

Committed patchset #4 (id:120001) as
https://skia.googlesource.com/skia/+/57cd94a9defda8fb37913049f80544d292d99232