Update PDF backend to support fallback fonts on Android.

Update PDF backend to support fallback fonts on Android.

R=edisonn@google.com, reed@google.com, vandebo@chromium.org

Committed: https://code.google.com/p/skia/source/detail?r=11586

[vandebo (ex-Chrome), 2013-09-24 19:19:25]

You should check that copy paste still work when fallback glyphs are used.  I
don't know, but the viewer may use the draw order of the glyphs during
selection, in which case you'd have to chunk it into runs of glyphs with the
same typeface.

[djsollen, 2013-09-24 19:37:41]

The copy/paste on my linux machine produced correct results.

[reed1, 2013-09-24 20:10:19]

https://codereview.chromium.org/24364008/diff/1/include/ports/SkTypeface_andr...
File include/ports/SkTypeface_android.h (right):

https://codereview.chromium.org/24364008/diff/1/include/ports/SkTypeface_andr...
include/ports/SkTypeface_android.h:81: SkTypeface*
SkGetTypefaceForGlyphID(uint16_t glyphID, const SkTypeface* origTypeface,
Does this call affect the refcnt() of the returned typeface? Does the caller
need to keep track of it?

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

https://codereview.chromium.org/24364008/diff/1/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1141: uint16_t* glyphIDs = NULL;
can this guy be const?

https://codereview.chromium.org/24364008/diff/1/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1153: glyphIDs =
reinterpret_cast<uint16_t*>(const_cast<void*>((text)));
nit: that's a lot (of) ((parens))

https://codereview.chromium.org/24364008/diff/1/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1165: SkTDArray<TypefaceData> typefaceData;
Can these two arrays be folded into a single one, with a struct that contains
the range+typeface?

https://codereview.chromium.org/24364008/diff/1/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1200: if (usesOriginalTypeface) {
In this case, can't we just call through to the original impl w/ no change
(other than clearing the fallback-paint-flag? Why do we need to make a copy of
the glyphs, etc.?

https://codereview.chromium.org/24364008/diff/1/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1201: visitedTypefaces.push(SkSafeRef(origFace.get()));
why do we need to ref() the typefaces?

[djsollen, 2013-09-24 20:44:17]

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

https://codereview.chromium.org/24364008/diff/1/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1200: if (usesOriginalTypeface) {
On 2013/09/24 20:10:19, reed1 wrote:
> In this case, can't we just call through to the original impl w/ no change
> (other than clearing the fallback-paint-flag? Why do we need to make a copy of
> the glyphs, etc.?

We need to make the copy for the case when we have both the original typeface
and a fallback typeface.  If we don't exclude the out of range glyphs then later
in the pdf setup we convert those glyphs to zeros.  In my testing some fonts
when given a glyphID of zero would draw empty boxes even though we really did
have the glyph (provided by another typeface).  So I was getting boxes around my
fallback glyphs.

[djsollen, 2013-10-01 20:25:06]

I'm hoping to get some more eyes on this before I land it in Android so I've
tagged a few additional reviewers.

[scroggo, 2013-10-01 21:00:14]

https://codereview.chromium.org/24364008/diff/17001/include/ports/SkTypeface_...
File include/ports/SkTypeface_android.h (right):

https://codereview.chromium.org/24364008/diff/17001/include/ports/SkTypeface_...
include/ports/SkTypeface_android.h:80: * glyphIDs that this typeface is capable
of resovling. The lower bound is
resolving*

https://codereview.chromium.org/24364008/diff/17001/include/ports/SkTypeface_...
include/ports/SkTypeface_android.h:85: int* lowerBounds, int* upperBounds);
If these are optional, should they have default values of NULL?

https://codereview.chromium.org/24364008/diff/17001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/24364008/diff/17001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1153: if (paint.getTextEncoding() !=
SkPaint::kGlyphID_TextEncoding) {
nit: I find it clearer if when there is an else statement, you check the
positive condition in the if statement.

https://codereview.chromium.org/24364008/diff/17001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1165: if (!origFace.get()) {
The style guide prefers explicit NULL checks.

https://codereview.chromium.org/24364008/diff/17001/src/ports/SkFontConfigInt...
File src/ports/SkFontConfigInterface_android.cpp (right):

https://codereview.chromium.org/24364008/diff/17001/src/ports/SkFontConfigInt...
src/ports/SkFontConfigInterface_android.cpp:694: currentTypeface =
const_cast<SkTypeface*>(origTypeface);
It appears you do not modify currentTypeface. It seems weird to do this cast
here.

https://codereview.chromium.org/24364008/diff/17001/src/ports/SkFontConfigInt...
src/ports/SkFontConfigInterface_android.cpp:711: if
((fFontFamilies[familyRecID].fPaintOptions.getFontVariant() & acceptedVariants)
!= 0) {
100 chars

https://codereview.chromium.org/24364008/diff/17001/src/ports/SkFontConfigInt...
src/ports/SkFontConfigInterface_android.cpp:723: if (currentTypeface) {
nit: explicit NULL check.

https://codereview.chromium.org/24364008/diff/17001/src/ports/SkFontConfigInt...
src/ports/SkFontConfigInterface_android.cpp:731: return
SkSafeRef(currentTypeface);
Why not return SkRef(currentTypeface) inside the if block, and return NULL here?

Also, it doesn't look like it gets unreffed?

[edisonn, 2013-10-02 12:54:23]

LGTM.

But the code is fairly complicated, are we comfortable with the test coverage
that gm is providing? AFAIK our skps are generated by Chrome (even the one from
Android tablets) so we might not hit this codepaths at all in our testing.

If we have 0 coverage in gm/skp, I would strongly suggest to add some tests.

[vandebo (ex-Chrome), 2013-10-02 15:58:02]

PDF code LGTM

https://codereview.chromium.org/24364008/diff/25001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/24364008/diff/25001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1229: SkPaintOptionsAndroid paintOpts =
tmpPaint.getPaintOptionsAndroid();
nit: you could pull paintOpts out of the loop.

[djsollen, 2013-10-02 16:16:41]

with the latest patch set I've addressed the comments, but my test case fails
for PDF if the language is not set. The failure is not a crash, but just that
the wrong glyph is shown.

https://codereview.chromium.org/24364008/diff/17001/src/ports/SkFontConfigInt...
File src/ports/SkFontConfigInterface_android.cpp (right):

https://codereview.chromium.org/24364008/diff/17001/src/ports/SkFontConfigInt...
src/ports/SkFontConfigInterface_android.cpp:694: currentTypeface =
const_cast<SkTypeface*>(origTypeface);
On 2013/10/01 21:00:14, scroggo wrote:
> It appears you do not modify currentTypeface. It seems weird to do this cast
> here.

But I do return that value and I can't return a const SkTypeface*

[scroggo, 2013-10-02 16:21:19]

https://codereview.chromium.org/24364008/diff/25001/gyp/gmslides.gypi
File gyp/gmslides.gypi (right):

https://codereview.chromium.org/24364008/diff/25001/gyp/gmslides.gypi#newcode7
gyp/gmslides.gypi:7: '../gm/androidfallback.cpp',
Is this a new file that should be included?

[reed1, 2013-10-02 16:32:08]

SkTypeface is weird 
- often, for convenience, the type is just SkTypeface* (not const)
- they are IMMUTABLE, and therefore always const

oiy

[djsollen, 2013-10-03 13:17:33]

Mike, as this is considered a public API I need to get sign-off from you as
well.

[reed1, 2013-10-03 13:26:35]

lgtm

[djsollen, 2013-10-03 14:42:27]

Committed patchset #7 manually as r11586 (presubmit successful).