SkPDF: pull out SkPDFMakeCIDGlyphWidthsArray.cpp

SkPDF: pull out SkPDFMakeCIDGlyphWidthsArray.cpp

Extract from inside SkPDFFont.cpp
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2251803002

Committed: https://skia.googlesource.com/skia/+/86b6eabeae80c3db69226cd1693d7f1c4591d88e

[hal.canary, 2016-08-16 18:52:39]

Description was changed from

==========
SkPDF: pull out SkPDFMakeCIDGlyphWidthsArray.cpp

Extract from inside SkPDFFont.cpp
==========

to

==========
SkPDF: pull out SkPDFMakeCIDGlyphWidthsArray.cpp

Extract from inside SkPDFFont.cpp
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2251803002
==========

[hal.canary, 2016-08-16 18:52:46]

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

[commit-bot: I haz the power, 2016-08-16 18:52:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hal.canary, 2016-08-16 18:56:10]

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

[hal.canary, 2016-08-16 18:56:11]

PTAL

[bungeman-skia, 2016-08-16 19:16:25]

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFFont.cpp#newco...
src/pdf/SkPDFFont.cpp:604: SkASSERT(glyphCache.get());
The above seems to be done in at least three other places now. Might be time to
factor it before adding a fourth?

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFMakeCIDGlyphWi...
File src/pdf/SkPDFMakeCIDGlyphWidthsArray.cpp (right):

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFMakeCIDGlyphWi...
src/pdf/SkPDFMakeCIDGlyphWidthsArray.cpp:287: sk_sp<SkPDFArray>
SkPDFMakeCIDGlyphWidthsArray(SkGlyphCache* cache,
It is nice to pull out this call and all it's supporting machinery into a
separate file. However, this seems to also be specific to the Type0 font
implementation. If there any way to make that more obvious, like stick this on
SkPDFType0Font?

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFMakeCIDGlyphWi...
File src/pdf/SkPDFMakeCIDGlyphWidthsArray.h (right):

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFMakeCIDGlyphWi...
src/pdf/SkPDFMakeCIDGlyphWidthsArray.h:30: sk_sp<SkPDFArray>
SkPDFMakeCIDGlyphWidthsArray(const SkPDFAdvancerMock& mock,
Having this mocking stuff seems premature if we don't have any tests using it.
Also, it seems like it would be easier to write a test that uses a checked in
font to assert that the results are as expected.

[commit-bot: I haz the power, 2016-08-16 19:21:01]

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

[commit-bot: I haz the power, 2016-08-16 19:21:02]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2016-08-16 20:42:09]

refactored everything.

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFFont.cpp#newco...
src/pdf/SkPDFFont.cpp:604: SkASSERT(glyphCache.get());
On 2016/08/16 19:16:25, bungeman-skia wrote:
> The above seems to be done in at least three other places now. Might be time
to
> factor it before adding a fourth?

Done.

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFMakeCIDGlyphWi...
File src/pdf/SkPDFMakeCIDGlyphWidthsArray.h (right):

https://codereview.chromium.org/2251803002/diff/1/src/pdf/SkPDFMakeCIDGlyphWi...
src/pdf/SkPDFMakeCIDGlyphWidthsArray.h:30: sk_sp<SkPDFArray>
SkPDFMakeCIDGlyphWidthsArray(const SkPDFAdvancerMock& mock,
On 2016/08/16 19:16:25, bungeman-skia wrote:
> Having this mocking stuff seems premature if we don't have any tests using it.
> Also, it seems like it would be easier to write a test that uses a checked in
> font to assert that the results are as expected.

Done.

[hal.canary, 2016-08-16 20:52:01]

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

[commit-bot: I haz the power, 2016-08-16 20:52:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bungeman-skia, 2016-08-16 21:09:08]

https://codereview.chromium.org/2251803002/diff/40001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/2251803002/diff/40001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:93: static SkPaint vector_paint(SkTypeface* face) {
Can this be

static SkAutoGlyphCache vector_glyph_cache(SkTypeface* face) {
    SkPaint tmpPaint;
    tmpPaint.setHinting(SkPaint::kNo_Hinting);
    SkASSERT(face);
    tmpPaint.setTypeface(sk_ref_sp(face));
    tmpPaint.setTextSize((SkScalar)face->getUnitsPerEm());

    const SkSurfaceProps props(0, kUnknown_SkPixelGeometry);
    SkAutoGlyphCache glyphCache(vector_paint(face), &props, nullptr);
    SkASSERT(glyphCache.get());
    return glyphCache;
}

and then at the call sites use like

SkAutoGlyphCache glyphCache = vector_glyph_cache(face.get());

SkAutoGlyphCache derives from unique_ptr, so hopefully this should just move. If
it isn't already, it should be movable.

[commit-bot: I haz the power, 2016-08-16 21:12:24]

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

[commit-bot: I haz the power, 2016-08-16 21:12:25]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2016-08-17 12:35:17]

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

[commit-bot: I haz the power, 2016-08-17 12:35:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hal.canary, 2016-08-17 12:38:26]

https://codereview.chromium.org/2251803002/diff/40001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/2251803002/diff/40001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:93: static SkPaint vector_paint(SkTypeface* face) {
On 2016/08/16 21:09:07, bungeman-skia wrote:
> Can this be
> 
> static SkAutoGlyphCache vector_glyph_cache(SkTypeface* face) {
>     SkPaint tmpPaint;
>     tmpPaint.setHinting(SkPaint::kNo_Hinting);
>     SkASSERT(face);
>     tmpPaint.setTypeface(sk_ref_sp(face));
>     tmpPaint.setTextSize((SkScalar)face->getUnitsPerEm());
> 
>     const SkSurfaceProps props(0, kUnknown_SkPixelGeometry);
>     SkAutoGlyphCache glyphCache(vector_paint(face), &props, nullptr);
>     SkASSERT(glyphCache.get());
>     return glyphCache;
> }
> 
> and then at the call sites use like
> 
> SkAutoGlyphCache glyphCache = vector_glyph_cache(face.get());
> 
> SkAutoGlyphCache derives from unique_ptr, so hopefully this should just move.
If
> it isn't already, it should be movable.

Done.

[commit-bot: I haz the power, 2016-08-17 12:53:33]

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

[commit-bot: I haz the power, 2016-08-17 12:53:34]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2016-08-17 14:56:08]

lgtm

[hal.canary, 2016-08-17 14:56:28]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2016-08-17 14:56:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 14:57:29]

Description was changed from

==========
SkPDF: pull out SkPDFMakeCIDGlyphWidthsArray.cpp

Extract from inside SkPDFFont.cpp
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2251803002
==========

to

==========
SkPDF: pull out SkPDFMakeCIDGlyphWidthsArray.cpp

Extract from inside SkPDFFont.cpp
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2251803002

Committed:
https://skia.googlesource.com/skia/+/86b6eabeae80c3db69226cd1693d7f1c4591d88e
==========

[commit-bot: I haz the power, 2016-08-17 14:57:30]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/86b6eabeae80c3db69226cd1693d7f1c4591d88e