Bug 186007 – [Cocoa] Delete unnecessary WebCascadeList in ComplexTextController

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

[Myles C. Maxfield]

[Cocoa] Delete unnecessary WebCascadeList in ComplexTextController

[Myles C. Maxfield]

Created attachment 341374 [details]
Patch

[EWS Watchlist]

Attachment 341374 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 341374 [details]
Patch

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

New failing tests:
fast/text/combining-mark-paint.html
fast/text/combining-enclosing-keycap.html

[EWS Watchlist]

Created attachment 341378 [details]
Archive of layout-test-results from ews103 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 341374 [details]
Patch

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

New failing tests:
fast/text/combining-mark-paint.html
fast/text/combining-enclosing-keycap.html

[EWS Watchlist]

Created attachment 341381 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 341374 [details]
Patch

Attachment 341374 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7807906

New failing tests:
fast/text/combining-enclosing-keycap.html

[EWS Watchlist]

Created attachment 341382 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 341374 [details]
Patch

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

New failing tests:
fast/text/combining-mark-paint.html
fast/text/combining-enclosing-keycap.html

[EWS Watchlist]

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

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

[Myles C. Maxfield]

Created attachment 341384 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 341384 [details]
Patch

Clearing flags on attachment: 341384

Committed r232223: <https://trac.webkit.org/changeset/232223>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40574677>

Comment on attachment 341384 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        No new tests because there is no behavior change.

I am quite concerned about this change given the lack of good test coverage. The argument made for the change in bug 185933 turned out to be incorrect, and I worry that the argument being made here is similarly incorrect and we just don’t have a way to tell.

I think we should fix bug 186010 before making any further changes here.

Comment on attachment 341384 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        Except there's no point, because we just determined that no font in the list can be used to render the
> +        cluster. CoreText isn't magic; it isn't going to somehow disagree with us. WebCascadeList is just
> +        useless code.

I think you’re right that Core Text won’t return any of the fonts in the list, but the cascade list affects the fallback path; that’s why the test case specified font-family values that had multiple families. Because of bug 186010, it may seem like this code isn’t doing anything now, but in Mountain Lion (and again after bug 186010 is fixed), if you were to remove it, those test cases would fall back to different fonts.

[Myles C. Maxfield]

This patch requires https://bugs.webkit.org/show_bug.cgi?id=185976