Summary: | Font feature settings comparisons are order-dependent and case-dependent | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, rniwa | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 147722 | ||||||||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2015-08-05 19:22:23 PDT
Created attachment 258339 [details]
Patch
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 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 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 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 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 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 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. Created attachment 258352 [details]
Patch
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 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 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 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
Created attachment 258389 [details]
Patch
Created attachment 258390 [details]
Patch
Created attachment 258391 [details]
Patch
Committed r188056: <http://trac.webkit.org/changeset/188056> |