Bug 147719 - Font feature settings comparisons are order-dependent and case-dependent
Summary: Font feature settings comparisons are order-dependent and case-dependent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks: 147722
  Show dependency treegraph
 
Reported: 2015-08-05 19:22 PDT by Myles C. Maxfield
Modified: 2015-08-06 13:46 PDT (History)
2 users (show)

See Also:


Attachments
Patch (13.61 KB, patch)
2015-08-05 19:24 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (682.25 KB, application/zip)
2015-08-05 19:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-mavericks (627.78 KB, application/zip)
2015-08-05 20:01 PDT, Build Bot
no flags Details
Patch (12.89 KB, patch)
2015-08-05 23:18 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (563.54 KB, application/zip)
2015-08-05 23:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (572.37 KB, application/zip)
2015-08-06 00:26 PDT, Build Bot
no flags Details
Patch (7.04 KB, patch)
2015-08-06 13:29 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (12.80 KB, patch)
2015-08-06 13:30 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (12.62 KB, patch)
2015-08-06 13:32 PDT, Myles C. Maxfield
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2015-08-05 19:22:23 PDT
Font feature settings comparisons are order-dependent and case-dependent
Comment 1 Myles C. Maxfield 2015-08-05 19:24:19 PDT
Created attachment 258339 [details]
Patch
Comment 2 Myles C. Maxfield 2015-08-05 19:29:18 PDT
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
Comment 3 Build Bot 2015-08-05 19:57:20 PDT
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
Comment 4 Build Bot 2015-08-05 19:57:23 PDT
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
Comment 5 Build Bot 2015-08-05 20:01:16 PDT
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
Comment 6 Build Bot 2015-08-05 20:01:19 PDT
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
Comment 7 Benjamin Poulain 2015-08-05 21:39:11 PDT
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?).
Comment 8 Myles C. Maxfield 2015-08-05 23:09:08 PDT
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.
Comment 9 Myles C. Maxfield 2015-08-05 23:18:43 PDT
Created attachment 258352 [details]
Patch
Comment 10 Build Bot 2015-08-05 23:59:14 PDT
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
Comment 11 Build Bot 2015-08-05 23:59:17 PDT
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
Comment 12 Build Bot 2015-08-06 00:26:21 PDT
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
Comment 13 Build Bot 2015-08-06 00:26:23 PDT
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
Comment 14 Myles C. Maxfield 2015-08-06 13:29:35 PDT
Created attachment 258389 [details]
Patch
Comment 15 Myles C. Maxfield 2015-08-06 13:30:55 PDT
Created attachment 258390 [details]
Patch
Comment 16 Myles C. Maxfield 2015-08-06 13:32:33 PDT
Created attachment 258391 [details]
Patch
Comment 17 Myles C. Maxfield 2015-08-06 13:46:09 PDT
Committed r188056: <http://trac.webkit.org/changeset/188056>