Bug 143389 – Migrate FontCascade's expansion functions to use StringView iterators

• Status: NEW
• Component: New Bugs

[Myles C. Maxfield]

Migrate FontCascade's expansion functions to use StringView iterators

[Myles C. Maxfield]

Created attachment 250102 [details]
Patch

[Darin Adler]

Comment on attachment 250102 [details]
Patch

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

Not sure why compilation is failing. Please don’t land until you resolve that!

> Source/WTF/wtf/text/StringView.h:74
> +    // Note that these four iterators do not support double increments or double dereferences.

These four things that follow are not iterators. These are sort of “collections” that you can then call begin/end on to get iterators.

I don’t think the terms “double increments” and “double dereferences” are clear. What you are saying is that these are a certain type of iterator. I believe that what you are saying is that these are input iterators http://en.cppreference.com/w/cpp/concept/InputIterator but further they are even more restricted because you must always dereference and then increment before dereferencing again. I don’t think the phrase “do not support double increments” says that to me.

> Source/WTF/wtf/text/StringView.h:515
> +// Iterators

I don’t think this is a helpful comment, especially since the classes below are not all iterators.

> Source/WTF/wtf/text/StringView.h:516
> +class StringView::CodeUnits {

I suggest putting all the class definitions first, and then all the inline functions later.

[Myles C. Maxfield]

Comment on attachment 250102 [details]
Patch

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

>> Source/WTF/wtf/text/StringView.h:74
>> +    // Note that these four iterators do not support double increments or double dereferences.
> 
> These four things that follow are not iterators. These are sort of “collections” that you can then call begin/end on to get iterators.
> 
> I don’t think the terms “double increments” and “double dereferences” are clear. What you are saying is that these are a certain type of iterator. I believe that what you are saying is that these are input iterators http://en.cppreference.com/w/cpp/concept/InputIterator but further they are even more restricted because you must always dereference and then increment before dereferencing again. I don’t think the phrase “do not support double increments” says that to me.

Done.

>> Source/WTF/wtf/text/StringView.h:515
>> +// Iterators
> 
> I don’t think this is a helpful comment, especially since the classes below are not all iterators.

Done.

>> Source/WTF/wtf/text/StringView.h:516
>> +class StringView::CodeUnits {
> 
> I suggest putting all the class definitions first, and then all the inline functions later.

Done.

Out of curiosity, why is this preferred?

[Myles C. Maxfield]

Created attachment 250111 [details]
Patch for landing

[Myles C. Maxfield]

Looks like this patch regresses Layout/line-layout-simple.html about 0.5%