Remove redundant typesetting checks from Font::width()

Remove redundant typesetting checks from Font::width()

Minor cleanup: Font::codePath() already selects ComplexPath whenever
typesettingFeatures are present (our SimplePath doesn't support kerning
and ligatures), so there's no need to duplicate that test in width().

R=schenney@chromium.org,jbroman@chromium.org,eae@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178557

[blink-reviews, 2014-07-21 11:07:52]

LGTM


On Sun, Jul 20, 2014 at 6:24 PM, <fmalita@chromium.org> wrote:

> Reviewers: eae, jbroman, Stephen Chenney,
>
> Description:
> Remove redundant typesetting checks from Font::width()
>
> Minor cleanup: Font::codePath() already selects ComplexPath whenever
> typesettingFeatures are present (our SimplePath doesn't support kerning
> and ligatures), so there's no need to duplicate that test in width().
>
> R=schenney@chromium.org,jbroman@chromium.org,eae@chromium.org
>
> Please review this at https://codereview.chromium.org/401123005/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+3, -4 lines):
>   M Source/platform/fonts/Font.cpp
>
>
> Index: Source/platform/fonts/Font.cpp
> diff --git a/Source/platform/fonts/Font.cpp b/Source/platform/fonts/Font.
> cpp
> index f25a76fd3f12ee925ec1766c8d49a6504d3a8382..
> 4670177678ea1905f4674086d1fb66348e4f4ca6 100644
> --- a/Source/platform/fonts/Font.cpp
> +++ b/Source/platform/fonts/Font.cpp
> @@ -160,9 +160,8 @@ float Font::width(const TextRun& run, HashSet<const
> SimpleFontData*>* fallbackFo
>              glyphOverflow = 0;
>      }
>
> -    bool hasKerningOrLigatures = fontDescription().typesettingFeatures()
> & (Kerning | Ligatures);
>      bool hasWordSpacingOrLetterSpacing = fontDescription().wordSpacing()
> || fontDescription().letterSpacing();
> -    bool isCacheable = (codePathToUse == ComplexPath ||
> hasKerningOrLigatures)
> +    bool isCacheable = codePathToUse == ComplexPath
>          && !hasWordSpacingOrLetterSpacing // Word spacing and letter
> spacing can change the width of a word.
>          && !run.allowTabs(); // If we allow tabs and a tab occurs inside
> a word, the width of the word varies based on its position on the line.
>
> @@ -180,8 +179,8 @@ float Font::width(const TextRun& run, HashSet<const
> SimpleFontData*>* fallbackFo
>      if (codePathToUse == ComplexPath) {
>          result = floatWidthForComplexText(run, fallbackFonts,
> &glyphBounds);
>      } else {
> -        result = floatWidthForSimpleText(run, fallbackFonts,
> -            glyphOverflow || isCacheable ? &glyphBounds : 0);
> +        ASSERT(!isCacheable);
> +        result = floatWidthForSimpleText(run, fallbackFonts,
> glyphOverflow ? &glyphBounds : 0);
>      }
>
>      if (cacheEntry && (!fallbackFonts || fallbackFonts->isEmpty())) {
>
>
>


-- 
Stephen Chenney | Software Engineer | schenney@google.com | 404-314-1809

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Stephen Chennney, 2014-07-21 11:08:14]

LGTM from the right account.


On Mon, Jul 21, 2014 at 7:07 AM, Stephen Chenney <schenney@google.com>
wrote:

> LGTM
>
>
> On Sun, Jul 20, 2014 at 6:24 PM, <fmalita@chromium.org> wrote:
>
>> Reviewers: eae, jbroman, Stephen Chenney,
>>
>> Description:
>> Remove redundant typesetting checks from Font::width()
>>
>> Minor cleanup: Font::codePath() already selects ComplexPath whenever
>> typesettingFeatures are present (our SimplePath doesn't support kerning
>> and ligatures), so there's no need to duplicate that test in width().
>>
>> R=schenney@chromium.org,jbroman@chromium.org,eae@chromium.org
>>
>> Please review this at https://codereview.chromium.org/401123005/
>>
>> SVN Base: svn://svn.chromium.org/blink/trunk
>>
>> Affected files (+3, -4 lines):
>>   M Source/platform/fonts/Font.cpp
>>
>>
>> Index: Source/platform/fonts/Font.cpp
>> diff --git a/Source/platform/fonts/Font.cpp b/Source/platform/fonts/Font.
>> cpp
>> index f25a76fd3f12ee925ec1766c8d49a6504d3a8382..
>> 4670177678ea1905f4674086d1fb66348e4f4ca6 100644
>> --- a/Source/platform/fonts/Font.cpp
>> +++ b/Source/platform/fonts/Font.cpp
>> @@ -160,9 +160,8 @@ float Font::width(const TextRun& run, HashSet<const
>> SimpleFontData*>* fallbackFo
>>              glyphOverflow = 0;
>>      }
>>
>> -    bool hasKerningOrLigatures = fontDescription().typesettingFeatures()
>> & (Kerning | Ligatures);
>>      bool hasWordSpacingOrLetterSpacing = fontDescription().wordSpacing()
>> || fontDescription().letterSpacing();
>> -    bool isCacheable = (codePathToUse == ComplexPath ||
>> hasKerningOrLigatures)
>> +    bool isCacheable = codePathToUse == ComplexPath
>>          && !hasWordSpacingOrLetterSpacing // Word spacing and letter
>> spacing can change the width of a word.
>>          && !run.allowTabs(); // If we allow tabs and a tab occurs inside
>> a word, the width of the word varies based on its position on the line.
>>
>> @@ -180,8 +179,8 @@ float Font::width(const TextRun& run, HashSet<const
>> SimpleFontData*>* fallbackFo
>>      if (codePathToUse == ComplexPath) {
>>          result = floatWidthForComplexText(run, fallbackFonts,
>> &glyphBounds);
>>      } else {
>> -        result = floatWidthForSimpleText(run, fallbackFonts,
>> -            glyphOverflow || isCacheable ? &glyphBounds : 0);
>> +        ASSERT(!isCacheable);
>> +        result = floatWidthForSimpleText(run, fallbackFonts,
>> glyphOverflow ? &glyphBounds : 0);
>>      }
>>
>>      if (cacheEntry && (!fallbackFonts || fallbackFonts->isEmpty())) {
>>
>>
>>
>
>
> --
> Stephen Chenney | Software Engineer | schenney@google.com | 404-314-1809
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[f(malita), 2014-07-21 12:38:53]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-07-21 12:39:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fmalita@chromium.org/401123005/1

[commit-bot: I haz the power, 2014-07-21 12:39:50]

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

[commit-bot: I haz the power, 2014-07-21 12:39:51]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[f(malita), 2014-07-21 12:44:43]

On 2014/07/21 12:39:51, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Huh. Somehow Rietveld didn't catch any of the L G T M S?!

[jbroman, 2014-07-21 12:50:29]

lgtm

[commit-bot: I haz the power, 2014-07-21 12:53:33]

Change committed as 178557