Bug 215302

Created attachment 406228 [details]
Patch

Comment on attachment 406228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406228&action=review

> Source/WebCore/ChangeLog:3
> +        Update OriginalAdvancesForCharacterTreatedAsSpace to work properly after r265241

"work properly" seems ambiguous

Do you mean "work without depending on something that’s been true in practice, but not guaranteed", "work more efficiently", "work in a way that’s more maintainable", or "work correctly"?

I’m assuming it’s the first. Can we find a slightly more precise way of saying that in the title, not just the change log comment?

> Source/WebCore/ChangeLog:11
> +        However, this is wrong, becuase shaping can insert or delete glyphs. Instead, now that we have

"becuase"

> Source/WebCore/platform/graphics/WidthIterator.cpp:61
>  public:

struct doesn’t need "public"

> Source/WebCore/platform/graphics/WidthIterator.cpp:73
> +    bool characterIsSpace = false;
> +    float advanceBeforeCharacter = 0;
> +    float advanceAtCharacter = 0;

In WebKit we’ve been using { false } instead of = false in cases like this.

> Source/WebCore/platform/graphics/WidthIterator.h:38
> +typedef HashMap<unsigned, OriginalAdvancesForCharacterTreatedAsSpace, DefaultHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned>> CharactersTreatedAsSpace;

I think a sorted vector and std::binary_search might be better than a HashMap for this.

New code should use "using" instead of "typedef". Switch this since you are touching it, please.

What guarantees we never use 0xFFFFFFFE or 0xFFFFFFFF? If we need to stick with a HasMap, can we make this even more efficient by using 16 bits?

Comment on attachment 406228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406228&action=review

>> Source/WebCore/platform/graphics/WidthIterator.h:38
>> +typedef HashMap<unsigned, OriginalAdvancesForCharacterTreatedAsSpace, DefaultHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned>> CharactersTreatedAsSpace;
> 
> I think a sorted vector and std::binary_search might be better than a HashMap for this.
> 
> New code should use "using" instead of "typedef". Switch this since you are touching it, please.
> 
> What guarantees we never use 0xFFFFFFFE or 0xFFFFFFFF? If we need to stick with a HasMap, can we make this even more efficient by using 16 bits?

I think I mean std::lower_bound.

Committed r265415: <https://trac.webkit.org/changeset/265415>

<rdar://problem/66742090>