Bug 147719 – Font feature settings comparisons are order-dependent and case-dependent

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Font feature settings comparisons are order-dependent and case-dependent

[Myles C. Maxfield]

Created attachment 258339 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 258339 [details]
Patch

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

> Source/WebCore/platform/graphics/FontFeatureSettings.h:59
> +        if (m_value < other.m_value)
> +            return true;
> +        return false;

This can just be return m_value < other.m_value

[Build Bot]

Comment on attachment 258339 [details]
Patch

Attachment 258339 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/21541

New failing tests:
css3/font-feature-settings-parsing.html

[Build Bot]

Created attachment 258341 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 258339 [details]
Patch

Attachment 258339 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/21563

New failing tests:
css3/font-feature-settings-parsing.html

[Build Bot]

Created attachment 258343 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Benjamin Poulain]

Comment on attachment 258339 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        No tests yet, because font-feature-settings is not implemented.

No way to test this from CSSOM?

> Source/WebCore/css/CSSParser.cpp:10381
> -    String tag = value->string;
> +    String tag = String(value->string).convertToASCIILowercase();

It's a bit sad to allocate memory for the first string, just to convert it.

Ideally this should be a feature of StringView and the CSSParserString could produce lowercase ASCII String.

> Source/WebCore/platform/graphics/FontFeatureSettings.h:57
> +        if (m_tag.impl() > other.m_tag.impl())
> +            return false;
> +        if (m_value < other.m_value)

If (m_tag.impl() == other.m_tag.impl() && m_value < other.m_value)
    return true;
return false;

> Source/WebCore/platform/graphics/FontFeatureSettings.h:74
>          return adoptRef(*new FontFeatureSettings);

This should be out-of-line.

It is fine to inline the constructor of Vector if your create() is out-of-line. Here you just inline everything, we have no data showing the inlining is beneficial.

> Source/WebCore/platform/graphics/FontFeatureSettings.h:79
> +        m_list.insert(std::lower_bound(m_list.begin(), m_list.end(), feature) - m_list.begin(), WTF::move(feature));

Clever use of lower_bound().

I am not a big fan of this code though. It assumes that the iterator supports operator-().
Ideally insert could should an iterator to avoid that problem (and invalidate it afterwards?).

[Myles C. Maxfield]

Comment on attachment 258339 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontFeatureSettings.h:79
>> +        m_list.insert(std::lower_bound(m_list.begin(), m_list.end(), feature) - m_list.begin(), WTF::move(feature));
> 
> Clever use of lower_bound().
> 
> I am not a big fan of this code though. It assumes that the iterator supports operator-().
> Ideally insert could should an iterator to avoid that problem (and invalidate it afterwards?).

I think Anders would object to more usage of Vector::iterators. It seems to me that it's a design goal to use unsigned everywhere in Vectors instead of iterators.

Given that this vector will almost always hold 0 - 1 items, an O(n) search is probably best here.

[Myles C. Maxfield]

Created attachment 258352 [details]
Patch

[Build Bot]

Comment on attachment 258352 [details]
Patch

Attachment 258352 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/22140

New failing tests:
css3/font-feature-settings-parsing.html

[Build Bot]

Created attachment 258354 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 258352 [details]
Patch

Attachment 258352 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/22201

New failing tests:
css3/font-feature-settings-parsing.html

[Build Bot]

Created attachment 258356 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Myles C. Maxfield]

Created attachment 258389 [details]
Patch

[Myles C. Maxfield]

Created attachment 258390 [details]
Patch

[Myles C. Maxfield]

Created attachment 258391 [details]
Patch

[Myles C. Maxfield]

Committed r188056: <http://trac.webkit.org/changeset/188056>