Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2246903002:
SkPDF: SkPDFFont class changes (Closed)
|
Created:
4 years, 4 months ago by hal.canary Modified:
4 years, 4 months ago Reviewers:
bungeman-skia CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPDF: SkPDFFont class changes
SkPDFFont:
* inline some one-line methdods.
- SkPDFFont::typeface()
- SkPDFFont::fontInfo()
- SkPDFFont::firstGlyphID()
- SkPDFFont::lastGlyphID()
- SkPDFFont::getFontDescriptor()
* de-virtualize some methods:
- SkPDFFont::getType()
- SkPDFFont::multiByteGlyphs()
* Constructor takes more arguments:
fontType, multiByteGlyphs
* re-order fields (pointers before shorts)
* use sk_sp<T> more, T* less
SkAdvancedTypefaceMetrics:
* SkAdvancedTypefaceMetrics::fFont now a uint8_t
* other enumes are sized.
* SkAdvancedTypefaceMetrics::fStyle now big enough.
* remove use of SkTBitOr, replaced with fancy templates
No public API changes.
TBR=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002
Committed: https://skia.googlesource.com/skia/+/3287588a467ee579c3947fe13c6add5048b14aa9
Patch Set 1 #Patch Set 2 : 2016-08-15 (Monday) 12:00:31 EDT #
Total comments: 16
Patch Set 3 : 2016-08-15 (Monday) 14:05:05 EDT #
Total comments: 12
Patch Set 4 : 2016-08-15 (Monday) 15:02:10 EDT #Patch Set 5 : 2016-08-15 (Monday) 17:11:22 EDT #
Total comments: 6
Patch Set 6 : 2016-08-15 (Monday) 21:25:47 EDT #
Messages
Total messages: 49 (34 generated)
Description was changed from ========== work in progress COMMIT=false BUG=skia: ========== to ========== work in progress COMMIT=false BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002 ==========
Description was changed from ========== work in progress COMMIT=false BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002 ========== to ========== SkPDF: SkPDFFont class changes GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002 ==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== SkPDF: SkPDFFont class changes GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002 ========== to ========== SkPDF: SkPDFFont class changes SkPDFFont: * inline some one-line methdods. - SkPDFFont::typeface() - SkPDFFont::fontInfo() - SkPDFFont::firstGlyphID() - SkPDFFont::lastGlyphID() - SkPDFFont::getFontDescriptor() * de-virtualize some methods: - SkPDFFont::getType() - SkPDFFont::multiByteGlyphs() * Constructor takes more arguments: fontType, multiByteGlyphs * re-order fields (pointers before shorts) * use sk_sp<T> more, T* less SkAdvancedTypefaceMetrics: * SkAdvancedTypefaceMetrics::fFont now a uint8_t * other enumes are sized. * SkAdvancedTypefaceMetrics::fStyle now big enough. * remove use of SkTBitOr GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002 ==========
halcanary@google.com changed reviewers: + bungeman@google.com
PTAL no difference in output. passes valgrind.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2246903002/diff/20001/src/core/SkAdvancedType... File src/core/SkAdvancedTypefaceMetrics.h (right): https://codereview.chromium.org/2246903002/diff/20001/src/core/SkAdvancedType... src/core/SkAdvancedTypefaceMetrics.h:65: uint8_t fFlags; Seems less clear now, before I knew these were FontFlags, now it just happens that it has the same underlying type. Of course, part of the issue is that these are flags, not enumerations, which is always fighting against the language. If we really need this to be an integer, then skstd::underlying_type_t<FontFlags> should be the type. If we just need '|' and '&' can we just provide those operators? Same goes for StyleFlags and fStyle below. It's rather annoying that we don't have a nice template-y thing for creating the appropriate operators. Don't try to use std::underlying_type directly, Chromium currently can't rely on it. https://codereview.chromium.org/2246903002/diff/20001/src/core/SkAdvancedType... src/core/SkAdvancedTypefaceMetrics.h:72: kFixedPitch_Style = 0x00001, Not really needed, but while we're touching these, can these constants not be 5 digits? Would be nice if they were full 8. Also, were the last three enum values here being effectively ignored due to fStyle being uint16_t? https://codereview.chromium.org/2246903002/diff/20001/src/core/SkTypeface.cpp File src/core/SkTypeface.cpp (left): https://codereview.chromium.org/2246903002/diff/20001/src/core/SkTypeface.cpp... src/core/SkTypeface.cpp:301: result->fFlags = SkTBitOr<SkAdvancedTypefaceMetrics::FontFlags>( Not sure why SkAdvancedTypefaceMetrics::FontFlags was mentioned explicitly here, since it could have been inferred. https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:548: auto info = (SkTypeface::PerGlyphInfo)( This is a really gross use of a c style cast and auto. https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:593: uint8_t flags = info ? info->fFlags : 0; This uint8_t is really opaque. Before the type was self descriptive, now it's three steps removed. https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:638: fDescriptor->insertInt("Flags", (int32_t)(fFontInfo->fStyle | kPdfSymbolic)); Much prefer to write an operator '|' and static_cast to size_t if we need to, c style casting this uint32_t to int32_t is weird. https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:972: std::move(descriptor)); nit: fits on previous line? https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.h File src/pdf/SkPDFFont.h (right): https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.h#new... src/pdf/SkPDFFont.h:163: sk_sp<const SkAdvancedTypefaceMetrics>& fontInfo() { return fFontInfo; } This is a convoluted and very restraining signature. Normally we would just have const SkAdvancedTypefaceMetrics* getFontInfo(); and if enough users actually wanted a 'ref' we could add sk_sp<SkAdvancedTypefaceMetrics> refFontInfo(); same for getFontDescriptor below. Generally references to smart pointers run into problems.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2246903002/diff/20001/src/core/SkAdvancedType... File src/core/SkAdvancedTypefaceMetrics.h (right): https://codereview.chromium.org/2246903002/diff/20001/src/core/SkAdvancedType... src/core/SkAdvancedTypefaceMetrics.h:65: uint8_t fFlags; On 2016/08/15 16:49:58, bungeman-skia wrote: > Seems less clear now, before I knew these were FontFlags, now it just happens > that it has the same underlying type. Of course, part of the issue is that these > are flags, not enumerations, which is always fighting against the language. > > If we really need this to be an integer, then > skstd::underlying_type_t<FontFlags> should be the type. If we just need '|' and > '&' can we just provide those operators? Same goes for StyleFlags and fStyle > below. It's rather annoying that we don't have a nice template-y thing for > creating the appropriate operators. > > Don't try to use std::underlying_type directly, Chromium currently can't rely on > it. Done. https://codereview.chromium.org/2246903002/diff/20001/src/core/SkAdvancedType... src/core/SkAdvancedTypefaceMetrics.h:72: kFixedPitch_Style = 0x00001, On 2016/08/15 16:49:58, bungeman-skia wrote: > Not really needed, but while we're touching these, can these constants not be 5 > digits? Would be nice if they were full 8. done. > Also, were the last three enum values here being effectively ignored due to > fStyle being uint16_t? they weren't ever set. https://codereview.chromium.org/2246903002/diff/20001/src/core/SkTypeface.cpp File src/core/SkTypeface.cpp (left): https://codereview.chromium.org/2246903002/diff/20001/src/core/SkTypeface.cpp... src/core/SkTypeface.cpp:301: result->fFlags = SkTBitOr<SkAdvancedTypefaceMetrics::FontFlags>( On 2016/08/15 16:49:58, bungeman-skia wrote: > Not sure why SkAdvancedTypefaceMetrics::FontFlags was mentioned explicitly here, > since it could have been inferred. Acknowledged. https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:548: auto info = (SkTypeface::PerGlyphInfo)( On 2016/08/15 16:49:58, bungeman-skia wrote: > This is a really gross use of a c style cast and auto. Done. https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:593: uint8_t flags = info ? info->fFlags : 0; On 2016/08/15 16:49:58, bungeman-skia wrote: > This uint8_t is really opaque. Before the type was self descriptive, now it's > three steps removed. Done. https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:638: fDescriptor->insertInt("Flags", (int32_t)(fFontInfo->fStyle | kPdfSymbolic)); On 2016/08/15 16:49:58, bungeman-skia wrote: > Much prefer to write an operator '|' and static_cast to size_t if we need to, c > style casting this uint32_t to int32_t is weird. Done. https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:972: std::move(descriptor)); On 2016/08/15 16:49:58, bungeman-skia wrote: > nit: fits on previous line? Done. https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.h File src/pdf/SkPDFFont.h (right): https://codereview.chromium.org/2246903002/diff/20001/src/pdf/SkPDFFont.h#new... src/pdf/SkPDFFont.h:163: sk_sp<const SkAdvancedTypefaceMetrics>& fontInfo() { return fFontInfo; } On 2016/08/15 16:49:58, bungeman-skia wrote: > This is a convoluted and very restraining signature. Normally we would just have > > const SkAdvancedTypefaceMetrics* getFontInfo(); > > and if enough users actually wanted a 'ref' we could add > > sk_sp<SkAdvancedTypefaceMetrics> refFontInfo(); > > same for getFontDescriptor below. > > Generally references to smart pointers run into problems. Done.
The CQ bit was unchecked by halcanary@google.com
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Since I'm afraid of 'friend definitions' and since enums in SkTypeface and about
a hundred other enums could use this, why not a new file (could be in src/core
for now, maybe include/private which starts out with
#include "SkTLogic.h"
namespace skstd {
template <typename T> struct is_bitmask_enum : std::false_type {};
}
template <typename E> SK_WHEN(skstd::is_bitmask_enum<E>::value, E) operator|(E
l, E r) {
using U = skstd::underlying_type_t<E>;
return static_cast<E>(static_cast<U>(l) | static_cast<U>(r));
}
template <typename E> SK_WHEN(skstd::is_bitmask_enum<E>::value, E&)
operator|=(E& l, E r) {
return l = l | r;
}
template <typename E> SK_WHEN(skstd::is_bitmask_enum<E>::value, E) operator&(E
l, E r) {
using U = skstd::underlying_type_t<E>;
return static_cast<E>(static_cast<U>(l) & static_cast<U>(r));
}
template <typename E> SK_WHEN(skstd::is_bitmask_enum<E>::value, E&)
operator&=(E& l, E r) {
return l = l & r;
}
Then put
namespace skstd {
template <> struct is_bitmask_enum<SkAdvancedTypefaceMetrics::FontFlags> :
std::true_type {};
template <> struct is_bitmask_enum<SkAdvancedTypefaceMetrics::StyleFlags> :
std::true_type {};
}
at the bottom of SkAdvancedTypefaceMetrics.h. This has the nicety of not using
friend definitions from inside macros.
https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType...
File src/core/SkAdvancedTypefaceMetrics.h (right):
https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType...
src/core/SkAdvancedTypefaceMetrics.h:26: #define
SK_DEFINE_FLAG_OPERATORS(ENUM_T, UNDERLYING_T) \
Why force the user to specify UNDERLYING_T, skstd::underlying_type_t<ENUM_T>
works.
https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType...
src/core/SkAdvancedTypefaceMetrics.h:27: friend ENUM_T operator&(ENUM_T u,
ENUM_T v) { \
Ah, yes, friend definitions. I had almost forgotten about them.
https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType...
src/core/SkAdvancedTypefaceMetrics.h:28: return (ENUM_T)((UNDERLYING_T)u &
(UNDERLYING_T)v); \
I would much prefer
return
static_cast<ENUM_T>((static_cast<skstd::underlying_type_t<ENUM_T>>(u) & \
static_cast<skstd::underlying_type_t<ENUM_T>>(v))); \
https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType...
src/core/SkAdvancedTypefaceMetrics.h:45: , fFlags((FontFlags)0)
why not kEmpty_FontFlag since it exists. Or remove kEmpty_FontFlag. It seems odd
to do this but leave the value.
https://codereview.chromium.org/2246903002/diff/60001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):
https://codereview.chromium.org/2246903002/diff/60001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:542: SkTypeface::PerGlyphInfo info =
(SkTypeface::PerGlyphInfo)(
Seems like SkTypeface::PerGlyphInfo needs flag operators as well.
https://codereview.chromium.org/2246903002/diff/60001/src/ports/SkFontHost_Fr...
File src/ports/SkFontHost_FreeType.cpp (right):
https://codereview.chromium.org/2246903002/diff/60001/src/ports/SkFontHost_Fr...
src/ports/SkFontHost_FreeType.cpp:497: info->fStyle =
(SkAdvancedTypefaceMetrics::StyleFlags)0;
Too bad '0' doesn't have a distinct type, so you can overload '=' on it (like
smart pointers do with nullptr).
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
take a look at SkEnumOperators.h https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType... File src/core/SkAdvancedTypefaceMetrics.h (right): https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType... src/core/SkAdvancedTypefaceMetrics.h:26: #define SK_DEFINE_FLAG_OPERATORS(ENUM_T, UNDERLYING_T) \ On 2016/08/15 20:17:03, bungeman-skia wrote: > Why force the user to specify UNDERLYING_T, skstd::underlying_type_t<ENUM_T> > works. Acknowledged. https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType... src/core/SkAdvancedTypefaceMetrics.h:27: friend ENUM_T operator&(ENUM_T u, ENUM_T v) { \ On 2016/08/15 20:17:03, bungeman-skia wrote: > Ah, yes, friend definitions. I had almost forgotten about them. Acknowledged. https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType... src/core/SkAdvancedTypefaceMetrics.h:28: return (ENUM_T)((UNDERLYING_T)u & (UNDERLYING_T)v); \ On 2016/08/15 20:17:03, bungeman-skia wrote: > I would much prefer > > return > static_cast<ENUM_T>((static_cast<skstd::underlying_type_t<ENUM_T>>(u) & \ > > static_cast<skstd::underlying_type_t<ENUM_T>>(v))); \ Acknowledged. https://codereview.chromium.org/2246903002/diff/60001/src/core/SkAdvancedType... src/core/SkAdvancedTypefaceMetrics.h:45: , fFlags((FontFlags)0) On 2016/08/15 20:17:03, bungeman-skia wrote: > why not kEmpty_FontFlag since it exists. Or remove kEmpty_FontFlag. It seems odd > to do this but leave the value. I'll just do away with it. https://codereview.chromium.org/2246903002/diff/60001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2246903002/diff/60001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:542: SkTypeface::PerGlyphInfo info = (SkTypeface::PerGlyphInfo)( On 2016/08/15 20:17:03, bungeman-skia wrote: > Seems like SkTypeface::PerGlyphInfo needs flag operators as well. I agree. Later CL. https://codereview.chromium.org/2246903002/diff/60001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/2246903002/diff/60001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:497: info->fStyle = (SkAdvancedTypefaceMetrics::StyleFlags)0; On 2016/08/15 20:17:03, bungeman-skia wrote: > Too bad '0' doesn't have a distinct type, so you can overload '=' on it (like > smart pointers do with nullptr). I agree. This defaults to 0 in the constructor, so I'll delete this line.
take a look at SkEnumOperators.h
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2246903002/diff/120001/src/core/SkAdvancedTyp... File src/core/SkAdvancedTypefaceMetrics.h (right): https://codereview.chromium.org/2246903002/diff/120001/src/core/SkAdvancedTyp... src/core/SkAdvancedTypefaceMetrics.h:100: std::true_type {}; nit: it shouldn't be necessary to put this on the next line, it's a bit weird looking to do so. https://codereview.chromium.org/2246903002/diff/120001/src/core/SkEnumOperato... File src/core/SkEnumOperators.h (right): https://codereview.chromium.org/2246903002/diff/120001/src/core/SkEnumOperato... src/core/SkEnumOperators.h:8: #define SkEnumOperators_DEFINED I don't think these are generally desirable enum operators, they're only wanted for enums that are flags. Maybe SkBitmaskEnum.h or something (it would be nice for the file and trait to have similar names). https://codereview.chromium.org/2246903002/diff/120001/src/core/SkEnumOperato... src/core/SkEnumOperators.h:17: SK_WHEN(skstd::is_bitmask_enum<E>::value, E) operator|(E l, E r) { nit: I personally prefer lines 16 and 17 here on one line since they fit.
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2246903002/diff/120001/src/core/SkAdvancedTyp... File src/core/SkAdvancedTypefaceMetrics.h (right): https://codereview.chromium.org/2246903002/diff/120001/src/core/SkAdvancedTyp... src/core/SkAdvancedTypefaceMetrics.h:100: std::true_type {}; On 2016/08/15 21:41:36, bungeman-skia wrote: > nit: it shouldn't be necessary to put this on the next line, it's a bit weird > looking to do so. Done. https://codereview.chromium.org/2246903002/diff/120001/src/core/SkEnumOperato... File src/core/SkEnumOperators.h (right): https://codereview.chromium.org/2246903002/diff/120001/src/core/SkEnumOperato... src/core/SkEnumOperators.h:8: #define SkEnumOperators_DEFINED On 2016/08/15 21:41:36, bungeman-skia wrote: > I don't think these are generally desirable enum operators, they're only wanted > for enums that are flags. Maybe SkBitmaskEnum.h or something (it would be nice > for the file and trait to have similar names). Done. https://codereview.chromium.org/2246903002/diff/120001/src/core/SkEnumOperato... src/core/SkEnumOperators.h:17: SK_WHEN(skstd::is_bitmask_enum<E>::value, E) operator|(E l, E r) { On 2016/08/15 21:41:36, bungeman-skia wrote: > nit: I personally prefer lines 16 and 17 here on one line since they fit. Done.
While at it, I did PerGlyphInfo
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from
==========
SkPDF: SkPDFFont class changes
SkPDFFont:
* inline some one-line methdods.
- SkPDFFont::typeface()
- SkPDFFont::fontInfo()
- SkPDFFont::firstGlyphID()
- SkPDFFont::lastGlyphID()
- SkPDFFont::getFontDescriptor()
* de-virtualize some methods:
- SkPDFFont::getType()
- SkPDFFont::multiByteGlyphs()
* Constructor takes more arguments:
fontType, multiByteGlyphs
* re-order fields (pointers before shorts)
* use sk_sp<T> more, T* less
SkAdvancedTypefaceMetrics:
* SkAdvancedTypefaceMetrics::fFont now a uint8_t
* other enumes are sized.
* SkAdvancedTypefaceMetrics::fStyle now big enough.
* remove use of SkTBitOr
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002
==========
to
==========
SkPDF: SkPDFFont class changes
SkPDFFont:
* inline some one-line methdods.
- SkPDFFont::typeface()
- SkPDFFont::fontInfo()
- SkPDFFont::firstGlyphID()
- SkPDFFont::lastGlyphID()
- SkPDFFont::getFontDescriptor()
* de-virtualize some methods:
- SkPDFFont::getType()
- SkPDFFont::multiByteGlyphs()
* Constructor takes more arguments:
fontType, multiByteGlyphs
* re-order fields (pointers before shorts)
* use sk_sp<T> more, T* less
SkAdvancedTypefaceMetrics:
* SkAdvancedTypefaceMetrics::fFont now a uint8_t
* other enumes are sized.
* SkAdvancedTypefaceMetrics::fStyle now big enough.
* remove use of SkTBitOr, replaced with fancy templates
No public API changes.
TBR=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002
==========
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by halcanary@google.com
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
SkPDF: SkPDFFont class changes
SkPDFFont:
* inline some one-line methdods.
- SkPDFFont::typeface()
- SkPDFFont::fontInfo()
- SkPDFFont::firstGlyphID()
- SkPDFFont::lastGlyphID()
- SkPDFFont::getFontDescriptor()
* de-virtualize some methods:
- SkPDFFont::getType()
- SkPDFFont::multiByteGlyphs()
* Constructor takes more arguments:
fontType, multiByteGlyphs
* re-order fields (pointers before shorts)
* use sk_sp<T> more, T* less
SkAdvancedTypefaceMetrics:
* SkAdvancedTypefaceMetrics::fFont now a uint8_t
* other enumes are sized.
* SkAdvancedTypefaceMetrics::fStyle now big enough.
* remove use of SkTBitOr, replaced with fancy templates
No public API changes.
TBR=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002
==========
to
==========
SkPDF: SkPDFFont class changes
SkPDFFont:
* inline some one-line methdods.
- SkPDFFont::typeface()
- SkPDFFont::fontInfo()
- SkPDFFont::firstGlyphID()
- SkPDFFont::lastGlyphID()
- SkPDFFont::getFontDescriptor()
* de-virtualize some methods:
- SkPDFFont::getType()
- SkPDFFont::multiByteGlyphs()
* Constructor takes more arguments:
fontType, multiByteGlyphs
* re-order fields (pointers before shorts)
* use sk_sp<T> more, T* less
SkAdvancedTypefaceMetrics:
* SkAdvancedTypefaceMetrics::fFont now a uint8_t
* other enumes are sized.
* SkAdvancedTypefaceMetrics::fStyle now big enough.
* remove use of SkTBitOr, replaced with fancy templates
No public API changes.
TBR=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246903002
Committed:
https://skia.googlesource.com/skia/+/3287588a467ee579c3947fe13c6add5048b14aa9
==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://skia.googlesource.com/skia/+/3287588a467ee579c3947fe13c6add5048b14aa9 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
