Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
152073003:
Adding code to calculate Underline Thickness from Font Metrics, this will be useful when Skia is us… (Closed)
|
Created:
6 years, 10 months ago by h.joshi Modified:
6 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionAdding code to calculate Underline Thickness from Font Metrics, this will be useful when Skia is used with Blink/Chrome. Blink changes are uploaded with code change in patch https://codereview.chromium.org/147703002/
BUG=skia:
Committed: http://code.google.com/p/skia/source/detail?r=13635
Patch Set 1 #
Total comments: 2
Patch Set 2 : Comment fix and addition of underline position #
Total comments: 2
Patch Set 3 : Fixing comments, adding bitfield to FontMetrics #
Total comments: 10
Patch Set 4 : Adding Bitfield to Font Metrics Structure #
Total comments: 8
Patch Set 5 : Fixing comments for Underline patch #
Total comments: 18
Patch Set 6 : Fixing comments for Underline patch #
Total comments: 7
Patch Set 7 : Fixing comments for Underline Patch #
Total comments: 8
Patch Set 8 : Fixing comments #
Total comments: 2
Patch Set 9 : Fixing comments, updating comments for Underline position in Header #
Total comments: 6
Patch Set 10 : Fixing comments and making Underline position as per Skia terms #Patch Set 11 : Updating AUTHORS file #Patch Set 12 : Updating files as per HEAD #
Messages
Total messages: 59 (0 generated)
While reading Font metrics Skia is not storing Underline thinkness which is useful while drawing underline, stike though etc line styles.
On 2014/02/01 05:50:04, h.joshi wrote: > While reading Font metrics Skia is not storing Underline thinkness which is > useful while drawing underline, stike though etc line styles. Pls review this patch, changes are also submitted to Blink which make changes for underline in case Mac is used. This patch will help to have same behavior in case of Chrome on Android
lgtm, with the big caveat that I have little knowledge or opinion here. You want reed's review.
https://codereview.chromium.org/152073003/diff/1/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/1/include/core/SkPaint.h#newco... include/core/SkPaint.h:745: SkScalar fUnderlineThickness; //!< underline thickness only for scalable fonts The underline thickness and position both come from the 'post' table, so if you have one you have the other, seems that both should be used. Also, all versions of the 'OS/2' table have yStrikeoutSize and yStrikeoutPosition, and it looks like you want this as well. Interestingly, FreeType and CoreText appear not to want to give out the strikeout metrics. I see no reason for FreeType to be doing this, but CoreText appears to want to handle any strikeout itself. At the very least, if adding 'underline thickness' here, 'underline position' should also be here (even if it should probably be ignored in vertical). The 'only for scalable fonts' here is misleading. This comment should instead state 'underline thickness, or 0 if cannot be determined'. The scalability of the font has little to do with this, though OpenType does require the 'post' table be present.
Will work on shared comments. https://codereview.chromium.org/152073003/diff/1/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/1/include/core/SkPaint.h#newco... include/core/SkPaint.h:745: SkScalar fUnderlineThickness; //!< underline thickness only for scalable fonts Thank you, will modify comment and also add "underline position" calculation. Comment "only for scalable fonts" was added as per comments in FreeType.h "underline_position - The position, in font units, of the underline line for this face. It is the center of the underlining stem. Only relevant for scalable formats." On 2014/02/10 19:06:22, bungeman1 wrote: > The underline thickness and position both come from the 'post' table, so if you > have one you have the other, seems that both should be used. Also, all versions > of the 'OS/2' table have yStrikeoutSize and yStrikeoutPosition, and it looks > like you want this as well. Interestingly, FreeType and CoreText appear not to > want to give out the strikeout metrics. I see no reason for FreeType to be doing > this, but CoreText appears to want to handle any strikeout itself. At the very > least, if adding 'underline thickness' here, 'underline position' should also be > here (even if it should probably be ignored in vertical). > > The 'only for scalable fonts' here is misleading. This comment should instead > state 'underline thickness, or 0 if cannot be determined'. The scalability of > the font has little to do with this, though OpenType does require the 'post' > table be present.
On 2014/02/11 04:41:59, h.joshi wrote: > Will work on shared comments. > > https://codereview.chromium.org/152073003/diff/1/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/1/include/core/SkPaint.h#newco... > include/core/SkPaint.h:745: SkScalar fUnderlineThickness; //!< underline > thickness only for scalable fonts > Thank you, will modify comment and also add "underline position" calculation. > Comment "only for scalable fonts" was added as per comments in FreeType.h > "underline_position - The position, in font units, of the underline line for > this face. It is the center of the underlining stem. > Only relevant for scalable formats." > > On 2014/02/10 19:06:22, bungeman1 wrote: > > The underline thickness and position both come from the 'post' table, so if > you > > have one you have the other, seems that both should be used. Also, all > versions > > of the 'OS/2' table have yStrikeoutSize and yStrikeoutPosition, and it looks > > like you want this as well. Interestingly, FreeType and CoreText appear not to > > want to give out the strikeout metrics. I see no reason for FreeType to be > doing > > this, but CoreText appears to want to handle any strikeout itself. At the very > > least, if adding 'underline thickness' here, 'underline position' should also > be > > here (even if it should probably be ignored in vertical). > > > > The 'only for scalable fonts' here is misleading. This comment should instead > > state 'underline thickness, or 0 if cannot be determined'. The scalability of > > the font has little to do with this, though OpenType does require the 'post' > > table be present. Fixed comments and added Underline Position to Font Metrics
lgtm @reed: I think we should get your lgtm on the SkPaint.h changes.
https://codereview.chromium.org/152073003/diff/110001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/110001/include/core/SkPaint.h#... include/core/SkPaint.h:745: SkScalar fUnderlineThickness; //!< underline thickness, or 0 if cannot be determined I suggest a bitfield at the beginning, with some bits defined to identify which values are unknown (e.g. underline). We may also find other uses besides just what is valid, but that's all I can think of for now.)
https://codereview.chromium.org/152073003/diff/110001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/110001/include/core/SkPaint.h#... include/core/SkPaint.h:745: SkScalar fUnderlineThickness; //!< underline thickness, or 0 if cannot be determined Added bitfield as per comments, pls check On 2014/02/11 15:59:52, reed1 wrote: > I suggest a bitfield at the beginning, with some bits defined to identify which > values are unknown (e.g. underline). We may also find other uses besides just > what is valid, but that's all I can think of for now.)
Made code changes and added bit field pls check. On 2014/02/11 15:59:52, reed1 wrote: > https://codereview.chromium.org/152073003/diff/110001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/110001/include/core/SkPaint.h#... > include/core/SkPaint.h:745: SkScalar fUnderlineThickness; //!< underline > thickness, or 0 if cannot be determined > I suggest a bitfield at the beginning, with some bits defined to identify which > values are unknown (e.g. underline). We may also find other uses besides just > what is valid, but that's all I can think of for now.)
https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:736: enum FontMetricsBitSet { Skia convention would name this FontMetricsFlags, and then use that as a suffix e.g. kUnderlineIsValid_FontMetricFlag = 1 << ..., https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:736: enum FontMetricsBitSet { Possibly we could place this enum inside FontMetrics itself, making it... struct FontMetrics { enum Flags { ... }; https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:740: kFontMetrics_Bottom = 1 << 4, //!< Bit flag for FontMetrics Bottom Do we really need/want a bit for every field? I thought only underline was in question here. The comment on each line may be unnecessary -- it doesn't say anything that isn't quite clear from the enum name. https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:750: kFontMetrics_Max = 0xFFFF, //!< Max Bit flag Why do we need the Max? https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:754: uint16_t fFontMetricsFlag; //!< Bit field to identify which values are unknown definitely make this uint32_t (or possibly 64bit) for alignment (and future proofing).
Will submit new patch with suggested changes. https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:736: enum FontMetricsBitSet { Okey, will change enum variable names. On 2014/02/12 17:29:08, reed1 wrote: > Skia convention would name this FontMetricsFlags, and then use that as a suffix > > e.g. > > kUnderlineIsValid_FontMetricFlag = 1 << ..., > https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:736: enum FontMetricsBitSet { Okey, will shift emum inside FontMetrics structure. On 2014/02/12 17:29:08, reed1 wrote: > Possibly we could place this enum inside FontMetrics itself, making it... > > struct FontMetrics { > enum Flags { > ... > }; https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:750: kFontMetrics_Max = 0xFFFF, //!< Max Bit flag Just added to signify for max/default value, thinking maybe if required in future. Should I remove this? On 2014/02/12 17:29:08, reed1 wrote: > Why do we need the Max? https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:754: uint16_t fFontMetricsFlag; //!< Bit field to identify which values are unknown Okey will make it 32 bit. On 2014/02/12 17:29:08, reed1 wrote: > definitely make this uint32_t (or possibly 64bit) for alignment (and future > proofing).
https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... include/core/SkPaint.h:740: kFontMetrics_Bottom = 1 << 4, //!< Bit flag for FontMetrics Bottom Will remove comments from enum. Underline will get assigned similar to Ascent etc variables, so added emum for all. Pls suggest how should I follow up here. On 2014/02/12 17:29:08, reed1 wrote: > Do we really need/want a bit for every field? I thought only underline was in > question here. > > The comment on each line may be unnecessary -- it doesn't say anything that > isn't quite clear from the enum name.
Added Bitfield and removed check for other FontMetrics fields. Pls review. On 2014/02/13 02:43:19, h.joshi wrote: > https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/170001/include/core/SkPaint.h#... > include/core/SkPaint.h:740: kFontMetrics_Bottom = 1 << 4, //!< Bit > flag for FontMetrics Bottom > Will remove comments from enum. > Underline will get assigned similar to Ascent etc variables, so added emum for > all. Pls suggest how should I follow up here. > > On 2014/02/12 17:29:08, reed1 wrote: > > Do we really need/want a bit for every field? I thought only underline was in > > question here. > > > > The comment on each line may be unnecessary -- it doesn't say anything that > > isn't quite clear from the enum name.
https://codereview.chromium.org/152073003/diff/370001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/370001/include/core/SkPaint.h#... include/core/SkPaint.h:734: /** Describes defined to identify which values are unknown, later can be nit: this comment needs to be un-indented This comment is difficult to parse, maybe something like "Flags which indicate the confidence level of various metrics. A set flag indicates that the metric may be trusted." https://codereview.chromium.org/152073003/diff/370001/include/core/SkPaint.h#... include/core/SkPaint.h:763: bool isFontMetricsFlagSet(FontMetrics * fObject, FontMetrics::FontMetricsFlags bitSet) const; This seems like a really odd public interface to have on SkPaint. At the very least this seems like something that would go in the FontMetrics struct, since you're passing a 'this' pointer (which should never be NULL) here anyway. After putting it there, it really needs a different name, something more like "check(FontMetricsFlag)" so that one can write something like "fontMetrics.check(kUnderlineThinknessIsValid_FontMetricFlag)". Reed may have other suggestions for a name. https://codereview.chromium.org/152073003/diff/370001/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/152073003/diff/370001/src/core/SkPaint.cpp#ne... src/core/SkPaint.cpp:1305: if ( (metrics->fUnderlineThickness == 0) || (metrics->fUnderlineThickness >= metrics->fAscent/2)) Why would these checks only be done when there is a 'scale'? These seem like odd checks here in any event, it seems that the knowledge as to whether we actually have good values should come from the port, not have some policy hard coded here. If this is the sort of policy checks that, say, Blink would like, then Blink should implement such a policy. https://codereview.chromium.org/152073003/diff/370001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/152073003/diff/370001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1422: underlineThickness = 0; Here is where the 'unknown' flag should be set (or the 'known' flag unset) stating that we don't know what the value is. The entire point of having the bitfield was to avoid using this '0 means unknown' convention.
Thank you, will be working on comments. https://codereview.chromium.org/152073003/diff/370001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/370001/include/core/SkPaint.h#... include/core/SkPaint.h:734: /** Describes defined to identify which values are unknown, later can be Okey, will make suggested changes On 2014/02/17 19:10:06, bungeman1 wrote: > nit: this comment needs to be un-indented > > This comment is difficult to parse, maybe something like "Flags which indicate > the confidence level of various metrics. A set flag indicates that the metric > may be trusted." https://codereview.chromium.org/152073003/diff/370001/include/core/SkPaint.h#... include/core/SkPaint.h:763: bool isFontMetricsFlagSet(FontMetrics * fObject, FontMetrics::FontMetricsFlags bitSet) const; Pls suggest should I move method inside FontMetrics? On 2014/02/17 19:10:06, bungeman1 wrote: > This seems like a really odd public interface to have on SkPaint. At the very > least this seems like something that would go in the FontMetrics struct, since > you're passing a 'this' pointer (which should never be NULL) here anyway. > > After putting it there, it really needs a different name, something more like > "check(FontMetricsFlag)" so that one can write something like > "fontMetrics.check(kUnderlineThinknessIsValid_FontMetricFlag)". Reed may have > other suggestions for a name. https://codereview.chromium.org/152073003/diff/370001/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/152073003/diff/370001/src/core/SkPaint.cpp#ne... src/core/SkPaint.cpp:1305: if ( (metrics->fUnderlineThickness == 0) || (metrics->fUnderlineThickness >= metrics->fAscent/2)) Added these to check faulty font metrics for Underline, as Underline is defined for scaled fonts only so added check here, from previous comments I thought Skia needs to check if Font has faulty Underline metrics and then set respective flag. On 2014/02/17 19:10:06, bungeman1 wrote: > Why would these checks only be done when there is a 'scale'? These seem like odd > checks here in any event, it seems that the knowledge as to whether we actually > have good values should come from the port, not have some policy hard coded > here. If this is the sort of policy checks that, say, Blink would like, then > Blink should implement such a policy. https://codereview.chromium.org/152073003/diff/370001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/152073003/diff/370001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1422: underlineThickness = 0; Okey, will make required changes. On 2014/02/17 19:10:06, bungeman1 wrote: > Here is where the 'unknown' flag should be set (or the 'known' flag unset) > stating that we don't know what the value is. The entire point of having the > bitfield was to avoid using this '0 means unknown' convention.
Added new patch with fixes for comments shared, pls review. On 2014/02/18 05:48:48, h.joshi wrote: > Thank you, will be working on comments. > > https://codereview.chromium.org/152073003/diff/370001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/370001/include/core/SkPaint.h#... > include/core/SkPaint.h:734: /** Describes defined to identify which values are > unknown, later can be > Okey, will make suggested changes > > On 2014/02/17 19:10:06, bungeman1 wrote: > > nit: this comment needs to be un-indented > > > > This comment is difficult to parse, maybe something like "Flags which indicate > > the confidence level of various metrics. A set flag indicates that the metric > > may be trusted." > > https://codereview.chromium.org/152073003/diff/370001/include/core/SkPaint.h#... > include/core/SkPaint.h:763: bool isFontMetricsFlagSet(FontMetrics * fObject, > FontMetrics::FontMetricsFlags bitSet) const; > Pls suggest should I move method inside FontMetrics? > > On 2014/02/17 19:10:06, bungeman1 wrote: > > This seems like a really odd public interface to have on SkPaint. At the very > > least this seems like something that would go in the FontMetrics struct, since > > you're passing a 'this' pointer (which should never be NULL) here anyway. > > > > After putting it there, it really needs a different name, something more like > > "check(FontMetricsFlag)" so that one can write something like > > "fontMetrics.check(kUnderlineThinknessIsValid_FontMetricFlag)". Reed may have > > other suggestions for a name. > > https://codereview.chromium.org/152073003/diff/370001/src/core/SkPaint.cpp > File src/core/SkPaint.cpp (right): > > https://codereview.chromium.org/152073003/diff/370001/src/core/SkPaint.cpp#ne... > src/core/SkPaint.cpp:1305: if ( (metrics->fUnderlineThickness == 0) || > (metrics->fUnderlineThickness >= metrics->fAscent/2)) > Added these to check faulty font metrics for Underline, as Underline is defined > for scaled fonts only so added check here, from previous comments I thought Skia > needs to check if Font has faulty Underline metrics and then set respective > flag. > > On 2014/02/17 19:10:06, bungeman1 wrote: > > Why would these checks only be done when there is a 'scale'? These seem like > odd > > checks here in any event, it seems that the knowledge as to whether we > actually > > have good values should come from the port, not have some policy hard coded > > here. If this is the sort of policy checks that, say, Blink would like, then > > Blink should implement such a policy. > > https://codereview.chromium.org/152073003/diff/370001/src/ports/SkFontHost_Fr... > File src/ports/SkFontHost_FreeType.cpp (right): > > https://codereview.chromium.org/152073003/diff/370001/src/ports/SkFontHost_Fr... > src/ports/SkFontHost_FreeType.cpp:1422: underlineThickness = 0; > Okey, will make required changes. > > On 2014/02/17 19:10:06, bungeman1 wrote: > > Here is where the 'unknown' flag should be set (or the 'known' flag unset) > > stating that we don't know what the value is. The entire point of having the > > bitfield was to avoid using this '0 means unknown' convention.
At PS5 this is looking much better to me. However, there are still a few issues which could use some attention. https://codereview.chromium.org/152073003/diff/480001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/480001/include/core/SkPaint.h#... include/core/SkPaint.h:758: @param fObject Pointer of FontMetrics object Here and on the other two... there is no longer an fObject param since it's just the implicit 'this' now. Should remove this param from these comments. https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp#ne... src/core/SkPaint.cpp:2263: { Style nit: we usually put this '{' at the end of the line above. https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp#ne... src/core/SkPaint.cpp:2264: this->fFontMetricsFlag = this->fFontMetricsFlag | flag; This might be clearer like fFontMetricsFlag |= flag; https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp#ne... src/core/SkPaint.cpp:2269: this->fFontMetricsFlag = this->fFontMetricsFlag ^ flag; This will toggle, not unset. Also, since members begin with the 'f' prefix in Skia, the 'this->' is unnecessary. fFontMetricsFlag &= ~flag; https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1403: underlinePosition = SkIntToScalar(face->underline_position) / upem; At this point we know that these values are trusted. https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1429: underlinePosition = 0; At this point we know these values are not trusted. https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1462: if(!underlineThickness) These checks here (and below) are incorrect and the flags should be set based on the knowledge gained above on lines 1429 and 1403. https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_ma... File src/ports/SkFontHost_mac.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:1385: if(!theMetrics.fUnderlineThickness) There is no reason for any of these tests; in this case we must just always set that we trust these as CoreText only supports fonts which have this information. Essentially, CoreText is claiming that these values are valid. If it returns a value of 0, we still need to trust that, as it's perfectly valid to have the underline position on the baseline. https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... File src/ports/SkFontHost_win.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... src/ports/SkFontHost_win.cpp:1067: if(!mx->fUnderlineThickness) If we get here, we're only dealing with fonts which will have this information, so these checks don't make sense, and we should just set that we trust these two values. (Same below.) Note that the fonts which do not have this information have already been dealt with above, and we are zeroing the entire FontMetrics struct, so those already state not to trust these values (which is good since they're just 0). https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... File src/ports/SkFontHost_win_dw.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... src/ports/SkFontHost_win_dw.cpp:880: if(!mx->fUnderlineThickness) Here and below: I don't believe that DirectWrite supports any font formats which do not have this information, so there is no need for these checks.
Will make suggested changes. https://codereview.chromium.org/152073003/diff/480001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/480001/include/core/SkPaint.h#... include/core/SkPaint.h:758: @param fObject Pointer of FontMetrics object Forgot to remove comments, will make changes. On 2014/02/19 15:39:17, bungeman1 wrote: > Here and on the other two... there is no longer an fObject param since it's just > the implicit 'this' now. Should remove this param from these comments. https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp#ne... src/core/SkPaint.cpp:2263: { Will make changes On 2014/02/19 15:39:17, bungeman1 wrote: > Style nit: we usually put this '{' at the end of the line above. https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp#ne... src/core/SkPaint.cpp:2264: this->fFontMetricsFlag = this->fFontMetricsFlag | flag; Will make changes On 2014/02/19 15:39:17, bungeman1 wrote: > This might be clearer like > > fFontMetricsFlag |= flag; https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp#ne... src/core/SkPaint.cpp:2269: this->fFontMetricsFlag = this->fFontMetricsFlag ^ flag; Will make changes On 2014/02/19 15:39:17, bungeman1 wrote: > This will toggle, not unset. Also, since members begin with the 'f' prefix in > Skia, the 'this->' is unnecessary. > > fFontMetricsFlag &= ~flag; https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1462: if(!underlineThickness) Okey, Added these checks below as "face->underline_thickness" and "face->underline_position" can be zero. Will make changes On 2014/02/19 15:39:17, bungeman1 wrote: > These checks here (and below) are incorrect and the flags should be set based on > the knowledge gained above on lines 1429 and 1403. https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_ma... File src/ports/SkFontHost_mac.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:1385: if(!theMetrics.fUnderlineThickness) Okey, will make changes. On 2014/02/19 15:39:17, bungeman1 wrote: > There is no reason for any of these tests; in this case we must just always set > that we trust these as CoreText only supports fonts which have this information. > Essentially, CoreText is claiming that these values are valid. If it returns a > value of 0, we still need to trust that, as it's perfectly valid to have the > underline position on the baseline. https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... File src/ports/SkFontHost_win.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... src/ports/SkFontHost_win.cpp:1067: if(!mx->fUnderlineThickness) Okey, Added these checks below as "face->underline_thickness" and "face->underline_position" can be zero. Will make changes On 2014/02/19 15:39:17, bungeman1 wrote: > If we get here, we're only dealing with fonts which will have this information, > so these checks don't make sense, and we should just set that we trust these two > values. (Same below.) Note that the fonts which do not have this information > have already been dealt with above, and we are zeroing the entire FontMetrics > struct, so those already state not to trust these values (which is good since > they're just 0). https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... File src/ports/SkFontHost_win_dw.cpp (right): https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... src/ports/SkFontHost_win_dw.cpp:880: if(!mx->fUnderlineThickness) Okey, Added these checks below as "face->underline_thickness" and "face->underline_position" can be zero. Will make changes On 2014/02/19 15:39:17, bungeman1 wrote: > Here and below: I don't believe that DirectWrite supports any font formats which > do not have this information, so there is no need for these checks.
Code added with suggested changes, pls review. On 2014/02/19 17:12:03, h.joshi wrote: > Will make suggested changes. > > https://codereview.chromium.org/152073003/diff/480001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/480001/include/core/SkPaint.h#... > include/core/SkPaint.h:758: @param fObject Pointer of FontMetrics object > Forgot to remove comments, will make changes. > > On 2014/02/19 15:39:17, bungeman1 wrote: > > Here and on the other two... there is no longer an fObject param since it's > just > > the implicit 'this' now. Should remove this param from these comments. > > https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp > File src/core/SkPaint.cpp (right): > > https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp#ne... > src/core/SkPaint.cpp:2263: { > Will make changes > On 2014/02/19 15:39:17, bungeman1 wrote: > > Style nit: we usually put this '{' at the end of the line above. > > https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp#ne... > src/core/SkPaint.cpp:2264: this->fFontMetricsFlag = this->fFontMetricsFlag | > flag; > Will make changes > On 2014/02/19 15:39:17, bungeman1 wrote: > > This might be clearer like > > > > fFontMetricsFlag |= flag; > > https://codereview.chromium.org/152073003/diff/480001/src/core/SkPaint.cpp#ne... > src/core/SkPaint.cpp:2269: this->fFontMetricsFlag = this->fFontMetricsFlag ^ > flag; > Will make changes > On 2014/02/19 15:39:17, bungeman1 wrote: > > This will toggle, not unset. Also, since members begin with the 'f' prefix in > > Skia, the 'this->' is unnecessary. > > > > fFontMetricsFlag &= ~flag; > > https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_Fr... > File src/ports/SkFontHost_FreeType.cpp (right): > > https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_Fr... > src/ports/SkFontHost_FreeType.cpp:1462: if(!underlineThickness) > Okey, Added these checks below as "face->underline_thickness" and > "face->underline_position" can be zero. Will make changes > > On 2014/02/19 15:39:17, bungeman1 wrote: > > These checks here (and below) are incorrect and the flags should be set based > on > > the knowledge gained above on lines 1429 and 1403. > > https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_ma... > File src/ports/SkFontHost_mac.cpp (right): > > https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_ma... > src/ports/SkFontHost_mac.cpp:1385: if(!theMetrics.fUnderlineThickness) > Okey, will make changes. > > On 2014/02/19 15:39:17, bungeman1 wrote: > > There is no reason for any of these tests; in this case we must just always > set > > that we trust these as CoreText only supports fonts which have this > information. > > Essentially, CoreText is claiming that these values are valid. If it returns a > > value of 0, we still need to trust that, as it's perfectly valid to have the > > underline position on the baseline. > > https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... > File src/ports/SkFontHost_win.cpp (right): > > https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... > src/ports/SkFontHost_win.cpp:1067: if(!mx->fUnderlineThickness) > Okey, Added these checks below as "face->underline_thickness" and > "face->underline_position" can be zero. Will make changes > > On 2014/02/19 15:39:17, bungeman1 wrote: > > If we get here, we're only dealing with fonts which will have this > information, > > so these checks don't make sense, and we should just set that we trust these > two > > values. (Same below.) Note that the fonts which do not have this information > > have already been dealt with above, and we are zeroing the entire FontMetrics > > struct, so those already state not to trust these values (which is good since > > they're just 0). > > https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... > File src/ports/SkFontHost_win_dw.cpp (right): > > https://codereview.chromium.org/152073003/diff/480001/src/ports/SkFontHost_wi... > src/ports/SkFontHost_win_dw.cpp:880: if(!mx->fUnderlineThickness) > Okey, Added these checks below as "face->underline_thickness" and > "face->underline_position" can be zero. Will make changes > > On 2014/02/19 15:39:17, bungeman1 wrote: > > Here and below: I don't believe that DirectWrite supports any font formats > which > > do not have this information, so there is no need for these checks.
PS6 lgtm reed, take another look?
much simpler/cleaner, thanks. It seems odd to have a public uint32 and getters/setters. A couple of ideas: 1. just expose the uint32 2. expose full-on getters for each (don't need setters to be public actually) e.g. /** * If the fontmetrics has a valid underlinethickness, return true, and set the thickness param to that value. If it doesn't return false and ignore the thickness param. */ bool metrics->hasUnderlineThickness(SkScalar* thickness) const;
Hi, I have few doubts: 1. with "just expose the uint32" do you mean to make "uint32_t fFontMetricsFlag;" member variable of SkPaint? Pls suggest On 2014/02/19 22:42:44, reed1 wrote: > much simpler/cleaner, thanks. > > It seems odd to have a public uint32 and getters/setters. > > A couple of ideas: > > 1. just expose the uint32 > 2. expose full-on getters for each (don't need setters to be public actually) > > e.g. > > /** > * If the fontmetrics has a valid underlinethickness, return true, and set the > thickness param to that value. If it doesn't return false and ignore the > thickness param. > */ > bool metrics->hasUnderlineThickness(SkScalar* thickness) const;
Here's one suggestion for what I might like to see. reed, does this sound good to you? I'm fine with the code on the implementation side (the FontHost changes), but public API changes must be made with some care. https://codereview.chromium.org/152073003/diff/590001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/590001/include/core/SkPaint.h#... include/core/SkPaint.h:737: enum FontMetricsFlags { Since this is in FontMetrics (already namespaced) this should be called ValidityFlags. There is no need to repeat 'FontMetrics' here. https://codereview.chromium.org/152073003/diff/590001/include/core/SkPaint.h#... include/core/SkPaint.h:739: kUnderlinePositionIsValid_FontMetricsflag = 1 << 1, kUnderlineThinkness_ValidityFlag kUnderlinePosition_ValidityFlag https://codereview.chromium.org/152073003/diff/590001/include/core/SkPaint.h#... include/core/SkPaint.h:742: uint32_t fFontMetricsFlag; //!< Bit field to identify which values are unknown fValidityFlags https://codereview.chromium.org/152073003/diff/590001/include/core/SkPaint.h#... include/core/SkPaint.h:760: void setFontMetricsFlag(FontMetrics::FontMetricsFlags bitSet); validate(uint32_t flags) or validate(ValidityFlags flag) or remove. Note that these getters and setters are somewhat superfluous. We only expect Skia to be setting or unsetting these flags, and Skia is fine with inline bit twiddling. Something like isValid is nice to have for users though. Since the implementations of these three methods are trivial, they should be inline here instead of implemented in the cpp file. No need to have the overhead of a function call for a bit twiddle. https://codereview.chromium.org/152073003/diff/590001/include/core/SkPaint.h#... include/core/SkPaint.h:765: void unsetFontMetricsFlag(FontMetrics::FontMetricsFlags bitSet); invalidate(uint32_t flags) or invalidate(ValidityFlags flag) or remove. https://codereview.chromium.org/152073003/diff/590001/include/core/SkPaint.h#... include/core/SkPaint.h:771: bool checkFontMetricsFlag(FontMetrics::FontMetricsFlags bitSet) const; isValid(uint32_t flags) or isValid(ValidityFlags flag) Note that by taking an uint32_t here, it is possible to write something like fontMetrics->isValid(SkPaint::FontMetrics::kUnderlineThinkness_ValidityFlag | SkPaint::FontMetrics::kUnderlinePosition_ValidityFlag) if one wishes to check both of these at the same time. In theory if the implementation is inline here, something like fontMetrics->isValid(SkPaint::FontMetrics::kUnderlineThinkness_ValidityFlag) && fontMetrics->isValid(SkPaint::FontMetrics::kUnderlinePosition_ValidityFlag) should resolve to the same code with basic optimizations. I believe that part of reed's earlier comment was that the user can still just do (fontMetrics->fValidityFlags & SkPaint::FontMetrics::kUnderlineThinkness_ValidityFlag) && (fontMetrics->fValidityFlags & SkPaint::FontMetrics::kUnderlinePosition_ValidityFlag) themselves, something like isValid is just syntactic sugar which isn't really required. https://codereview.chromium.org/152073003/diff/590001/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/152073003/diff/590001/src/core/SkPaint.cpp#ne... src/core/SkPaint.cpp:2271: return SkToBool(fFontMetricsFlag & flag); If going with the uint32_t version of these, the setters above are still good, but this would become SkToBool((fFontMetricsFlag & flag) == flag); or something equivalent. I do believe that all of these implementations should be inlined (in SkPaint.h) as they're just named bit twiddles.
I think I'd be fine with...
uint32_t fFlags;
enum Flags {
kUnderlineThicknessValid_Flag = 1 << 0,
...
};
New patch is added after fixing suggested changes, pls review.
On 2014/02/21 21:25:03, reed1 wrote:
> I think I'd be fine with...
>
> uint32_t fFlags;
>
> enum Flags {
> kUnderlineThicknessValid_Flag = 1 << 0,
> ...
> };
https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... include/core/SkPaint.h:738: kUnderlineThinknessIsValid_Flag = 0x01, tiny nit: Since we're using 1 << 1 (which I like) for the 2nd bit, I think kUnderlineThicknessIsValid_Flag = 1 << 0, is very clear and consistent. https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... include/core/SkPaint.h:754: SkScalar fUnderlineThickness; //!< underline thickness, or 0 if cannot be determined Documentation question about position. If the thickness is 2 (lets say), what does position mean? - the top of the "rectangle that is the underline" is 2 below the baseline? - the center of the underline (however thick) is 2 below the baseline? - other? https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... include/core/SkPaint.h:777: } I don't think we should have any of these construction/setter helpers -- this class is used by clients as purely something to read that Skia built. I would remove validate/invalidate/isValid. https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... include/core/SkPaint.h:789: bool hasUnderlinePosition(SkScalar* thickness) const; These two getters are OK in my opinion, though not required. I think if we keep them, they can both trivially be inlined (helps in the dox). bool hasUnderlneThickness(thickenss) { if (fFlags & kUnderlin..._Flag) { *thickness = value; return true; } return false; }
Comments have been fixed, added new patch with comment fixed. If the thickness is 2 (lets say), what does position mean? Himanshu >> For Position we can have following values - Positive - means underline should be drawn above baseline. - Negative - means below baseline. - Zero - mean underline should be drawn on baseline. On 2014/02/24 13:26:21, reed1 wrote: > https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... > include/core/SkPaint.h:738: kUnderlineThinknessIsValid_Flag = 0x01, > tiny nit: > > Since we're using 1 << 1 (which I like) for the 2nd bit, I think > > kUnderlineThicknessIsValid_Flag = 1 << 0, > > is very clear and consistent. > > https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... > include/core/SkPaint.h:754: SkScalar fUnderlineThickness; //!< underline > thickness, or 0 if cannot be determined > Documentation question about position. > > If the thickness is 2 (lets say), what does position mean? > > - the top of the "rectangle that is the underline" is 2 below the baseline? > - the center of the underline (however thick) is 2 below the baseline? > - other? > > https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... > include/core/SkPaint.h:777: } > I don't think we should have any of these construction/setter helpers -- this > class is used by clients as purely something to read that Skia built. I would > remove validate/invalidate/isValid. > > https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... > include/core/SkPaint.h:789: bool hasUnderlinePosition(SkScalar* thickness) > const; > These two getters are OK in my opinion, though not required. I think if we keep > them, they can both trivially be inlined (helps in the dox). > > bool hasUnderlneThickness(thickenss) { > if (fFlags & kUnderlin..._Flag) { > *thickness = value; > return true; > } > return false; > }
All comments have been fixed, pls review. https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... include/core/SkPaint.h:738: kUnderlineThinknessIsValid_Flag = 0x01, Done, fixed in new patch. On 2014/02/24 13:26:22, reed1 wrote: > tiny nit: > > Since we're using 1 << 1 (which I like) for the 2nd bit, I think > > kUnderlineThicknessIsValid_Flag = 1 << 0, > > is very clear and consistent. https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... include/core/SkPaint.h:777: } Done, removed getter and setter methods. On 2014/02/24 13:26:22, reed1 wrote: > I don't think we should have any of these construction/setter helpers -- this > class is used by clients as purely something to read that Skia built. I would > remove validate/invalidate/isValid. https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... include/core/SkPaint.h:789: bool hasUnderlinePosition(SkScalar* thickness) const; Done, these are made inline now in new patch. On 2014/02/24 13:26:22, reed1 wrote: > These two getters are OK in my opinion, though not required. I think if we keep > them, they can both trivially be inlined (helps in the dox). > > bool hasUnderlneThickness(thickenss) { > if (fFlags & kUnderlin..._Flag) { > *thickness = value; > return true; > } > return false; > }
https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... include/core/SkPaint.h:754: SkScalar fUnderlineThickness; //!< underline thickness, or 0 if cannot be determined For Position we can have following values - Positive - means underline should be drawn above baseline. - Negative - means below baseline. - Zero - mean underline should be drawn on baseline. On 2014/02/24 13:26:22, reed1 wrote: > Documentation question about position. > > If the thickness is 2 (lets say), what does position mean? > > - the top of the "rectangle that is the underline" is 2 below the baseline? > - the center of the underline (however thick) is 2 below the baseline? > - other?
On 2014/02/24 18:22:38, h.joshi wrote: > https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/750001/include/core/SkPaint.h#... > include/core/SkPaint.h:754: SkScalar fUnderlineThickness; //!< underline > thickness, or 0 if cannot be determined > For Position we can have following values > - Positive - means underline should be drawn above baseline. > - Negative - means below baseline. > - Zero - mean underline should be drawn on baseline. > > On 2014/02/24 13:26:22, reed1 wrote: > > Documentation question about position. > > > > If the thickness is 2 (lets say), what does position mean? > > > > - the top of the "rectangle that is the underline" is 2 below the baseline? > > - the center of the underline (however thick) is 2 below the baseline? > > - other? Still need you to document exactly how to interpret the thickness value (relative to the position value). The documentation needs to be in the header.
https://codereview.chromium.org/152073003/diff/800001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/800001/include/core/SkPaint.h#... include/core/SkPaint.h:767: *thickness = 0; don't change thickness if we return false. https://codereview.chromium.org/152073003/diff/800001/include/core/SkPaint.h#... include/core/SkPaint.h:781: *thickness = 0; Skia protocol for this sort of getter is (generally) to ignore the output-parameter if we're returning false.
Made changes as per On 2014/02/24 18:30:09, reed1 wrote: > https://codereview.chromium.org/152073003/diff/800001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/800001/include/core/SkPaint.h#... > include/core/SkPaint.h:767: *thickness = 0; > don't change thickness if we return false. > > https://codereview.chromium.org/152073003/diff/800001/include/core/SkPaint.h#... > include/core/SkPaint.h:781: *thickness = 0; > Skia protocol for this sort of getter is (generally) to ignore the > output-parameter if we're returning false.
Made changes as per suggestion, pls review. On 2014/02/24 18:30:09, reed1 wrote: > https://codereview.chromium.org/152073003/diff/800001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/800001/include/core/SkPaint.h#... > include/core/SkPaint.h:767: *thickness = 0; > don't change thickness if we return false. > > https://codereview.chromium.org/152073003/diff/800001/include/core/SkPaint.h#... > include/core/SkPaint.h:781: *thickness = 0; > Skia protocol for this sort of getter is (generally) to ignore the > output-parameter if we're returning false.
much better, thanks. lgtm w/ style nits https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... include/core/SkPaint.h:756: /** Underline Position - position of the top of the Underline stroke relative to the baseline. nit: exceeds 100 cols https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... include/core/SkPaint.h:758: - Positive - means underline should be drawn above baseline. Our other fontmetrics always follow's Skia coordinate system, which is < 0 above the baseline https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... include/core/SkPaint.h:770: { Skia style places the { on the same line as the "if"
https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... include/core/SkPaint.h:756: /** Underline Position - position of the top of the Underline stroke relative to the baseline. Okey, will make require changes. On 2014/02/25 13:16:51, reed1 wrote: > nit: exceeds 100 cols https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... include/core/SkPaint.h:758: - Positive - means underline should be drawn above baseline. Okey, will make require changes same as done for ascent. On 2014/02/25 13:16:51, reed1 wrote: > Our other fontmetrics always follow's Skia coordinate system, which is < 0 above > the baseline https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... include/core/SkPaint.h:770: { Okey, will make require changes. On 2014/02/25 13:16:51, reed1 wrote: > Skia style places the { on the same line as the "if"
New Patch added with suggested changes, pls review. On 2014/02/25 18:11:10, h.joshi wrote: > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... > include/core/SkPaint.h:756: /** Underline Position - position of the top of the > Underline stroke relative to the baseline. > Okey, will make require changes. > > On 2014/02/25 13:16:51, reed1 wrote: > > nit: exceeds 100 cols > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... > include/core/SkPaint.h:758: - Positive - means underline should be drawn above > baseline. > Okey, will make require changes same as done for ascent. > > On 2014/02/25 13:16:51, reed1 wrote: > > Our other fontmetrics always follow's Skia coordinate system, which is < 0 > above > > the baseline > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... > include/core/SkPaint.h:770: { > Okey, will make require changes. > > On 2014/02/25 13:16:51, reed1 wrote: > > Skia style places the { on the same line as the "if"
In new patch, underline position is made negative of the value we get from font similar to Ascent is stored in Skia. Comments in header is also updated with above changes. On 2014/02/25 18:11:40, h.joshi wrote: > New Patch added with suggested changes, pls review. > > On 2014/02/25 18:11:10, h.joshi wrote: > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h > > File include/core/SkPaint.h (right): > > > > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... > > include/core/SkPaint.h:756: /** Underline Position - position of the top of > the > > Underline stroke relative to the baseline. > > Okey, will make require changes. > > > > On 2014/02/25 13:16:51, reed1 wrote: > > > nit: exceeds 100 cols > > > > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... > > include/core/SkPaint.h:758: - Positive - means underline should be drawn above > > baseline. > > Okey, will make require changes same as done for ascent. > > > > On 2014/02/25 13:16:51, reed1 wrote: > > > Our other fontmetrics always follow's Skia coordinate system, which is < 0 > > above > > > the baseline > > > > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... > > include/core/SkPaint.h:770: { > > Okey, will make require changes. > > > > On 2014/02/25 13:16:51, reed1 wrote: > > > Skia style places the { on the same line as the "if"
Hi, Is code good to go, should I start with commit process pls suggest On 2014/02/26 05:18:19, h.joshi wrote: > In new patch, underline position is made negative of the value we get from font > similar to Ascent is stored in Skia. Comments in header is also updated with > above changes. > > On 2014/02/25 18:11:40, h.joshi wrote: > > New Patch added with suggested changes, pls review. > > > > On 2014/02/25 18:11:10, h.joshi wrote: > > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h > > > File include/core/SkPaint.h (right): > > > > > > > > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... > > > include/core/SkPaint.h:756: /** Underline Position - position of the top of > > the > > > Underline stroke relative to the baseline. > > > Okey, will make require changes. > > > > > > On 2014/02/25 13:16:51, reed1 wrote: > > > > nit: exceeds 100 cols > > > > > > > > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... > > > include/core/SkPaint.h:758: - Positive - means underline should be drawn > above > > > baseline. > > > Okey, will make require changes same as done for ascent. > > > > > > On 2014/02/25 13:16:51, reed1 wrote: > > > > Our other fontmetrics always follow's Skia coordinate system, which is < 0 > > > above > > > > the baseline > > > > > > > > > https://codereview.chromium.org/152073003/diff/880001/include/core/SkPaint.h#... > > > include/core/SkPaint.h:770: { > > > Okey, will make require changes. > > > > > > On 2014/02/25 13:16:51, reed1 wrote: > > > > Skia style places the { on the same line as the "if"
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/h.joshi@samsung.com/152073003/900001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 152073003-900001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** The email h.joshi@samsung.com is not in Skia's AUTHORS file. Issue owner, this CL must include an addition to the Skia AUTHORS file. Googler reviewers, please check that the AUTHORS entry corresponds to an email address in http://goto/cla-signers. If it does not then ask the issue owner to sign the CLA at https://developers.google.com/open-source/cla/individual (individual) or https://developers.google.com/open-source/cla/corporate (corporate).
On 2014/02/28 16:13:59, reed1 wrote: I verified that h.joshi was listed in the Samsung Corporate CLA at go/clasigners. h.joshi, please add an entry for <*@samsung.com> in the Skia AUTHORS file (https://code.google.com/p/skia/source/browse/trunk/AUTHORS) as a part of this CL and it should be good to go..then you'll also be ready for all future submissions.
Updated AUTHORS files On 2014/02/28 16:21:26, hcm wrote: > On 2014/02/28 16:13:59, reed1 wrote: > > I verified that h.joshi was listed in the Samsung Corporate CLA at > go/clasigners. > > h.joshi, please add an entry for <mailto:*@samsung.com> in the Skia AUTHORS file > (https://code.google.com/p/skia/source/browse/trunk/AUTHORS) as a part of this > CL and it should be good to go..then you'll also be ready for all future > submissions.
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/h.joshi@samsung.com/152073003/920001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for AUTHORS: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file AUTHORS Hunk #1 FAILED at 15. 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej Patch: AUTHORS Index: AUTHORS diff --git a/AUTHORS b/AUTHORS index 84f28269054c17ba1ceec16f2ad966bf3adbc507..e190554961c754f8725595af1f8b7870ddaf06ec 100644 --- a/AUTHORS +++ b/AUTHORS @@ -15,5 +15,6 @@ ARM <*@arm.com> Google Inc. <*@google.com> Intel <*@intel.com> NVIDIA <*@nvidia.com> +Samsung <*@samsung.com> Thiago Fransosi Farina <thiago.farina@gmail.com>
Hi, I have updated AUTHORS file, when tried to submit patch got error, pls suggest On 2014/03/01 03:03:01, I haz the power (commit-bot) wrote: > Failed to apply patch for AUTHORS: > While running patch -p1 --forward --force --no-backup-if-mismatch; > patching file AUTHORS > Hunk #1 FAILED at 15. > 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej > > Patch: AUTHORS > Index: AUTHORS > diff --git a/AUTHORS b/AUTHORS > index > 84f28269054c17ba1ceec16f2ad966bf3adbc507..e190554961c754f8725595af1f8b7870ddaf06ec > 100644 > --- a/AUTHORS > +++ b/AUTHORS > @@ -15,5 +15,6 @@ ARM <mailto:*@arm.com> > Google Inc. <mailto:*@google.com> > Intel <mailto:*@intel.com> > NVIDIA <mailto:*@nvidia.com> > +Samsung <mailto:*@samsung.com> > Thiago Fransosi Farina <mailto:thiago.farina@gmail.com>
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/h.joshi@samsung.com/152073003/840002
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for AUTHORS: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file AUTHORS Hunk #1 FAILED at 11. 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej Patch: AUTHORS Index: AUTHORS diff --git a/AUTHORS b/AUTHORS index 84f28269054c17ba1ceec16f2ad966bf3adbc507..4bef9330edae2df81e3ed31b4dad9b483a0f01fa 100644 --- a/AUTHORS +++ b/AUTHORS @@ -11,9 +11,12 @@ # # Please keep the list sorted. +ACCESS CO., LTD. <*@access-company.com> ARM <*@arm.com> +George Wright <george@mozilla.com> Google Inc. <*@google.com> Intel <*@intel.com> NVIDIA <*@nvidia.com> +Samsung <*@samsung.com> +The Chromium Authors <*@chromium.org> Thiago Fransosi Farina <thiago.farina@gmail.com> -
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/h.joshi@samsung.com/152073003/950001
Message was sent while issue was closed.
Change committed as 13635
Message was sent while issue was closed.
Thank you, made changes to AUTHOR file and code is committed. On 2014/03/01 20:12:30, I haz the power (commit-bot) wrote: > Change committed as 13635 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
