[webkit-reviews] review granted: [Bug 215302] Update OriginalAdvancesForCharacterTreatedAsSpace to work properly after r265241 : [Attachment 406228] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 215302: Update OriginalAdvancesForCharacterTreatedAsSpace to work properly
after r265241
https://bugs.webkit.org/show_bug.cgi?id=215302

Attachment 406228: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 406228
  --> https://bugs.webkit.org/attachment.cgi?id=406228
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?