Bug 183149 - Use ICU's ubidi facilities for canvas text
Summary: Use ICU's ubidi facilities for canvas text
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-26 14:42 PST by Myles C. Maxfield
Modified: 2019-06-16 21:11 PDT (History)
7 users (show)

See Also:


Attachments
WIP (9.04 KB, patch)
2018-02-26 18:54 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (9.85 KB, patch)
2018-02-26 22:11 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.45 MB, application/zip)
2018-02-26 23:45 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews201 for win-future (11.60 MB, application/zip)
2018-02-26 23:53 PST, EWS Watchlist
no flags Details
Patch (11.96 KB, patch)
2018-02-27 19:34 PST, Myles C. Maxfield
no flags 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 2018-02-26 14:42:06 PST
Use ICU's ubidi facilities for canvas text
Comment 1 Myles C. Maxfield 2018-02-26 18:54:12 PST
Created attachment 334663 [details]
WIP
Comment 2 Myles C. Maxfield 2018-02-26 22:11:27 PST
Created attachment 334670 [details]
Patch
Comment 3 Myles C. Maxfield 2018-02-26 22:27:22 PST
Comment on attachment 334670 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:631
> +    UBiDi* line = ubidi_open();

Forgot to close.
Comment 4 Myles C. Maxfield 2018-02-26 23:40:49 PST
Comment on attachment 334670 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:642
> +    ubidi_setPara(line, text, run.length(), run.direction() == LTR ? 0 : 1, nullptr, &errorCode);

We may want to use UBIDI_DEFAULT_RTL or UBIDI_DEFAULT_LTR instead.
Comment 5 EWS Watchlist 2018-02-26 23:45:54 PST
Comment on attachment 334670 [details]
Patch

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

New failing tests:
fast/canvas/bidi.html
Comment 6 EWS Watchlist 2018-02-26 23:45:55 PST
Created attachment 334676 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 7 EWS Watchlist 2018-02-26 23:53:29 PST
Comment on attachment 334670 [details]
Patch

Attachment 334670 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6686696

New failing tests:
fast/canvas/bidi.html
Comment 8 EWS Watchlist 2018-02-26 23:53:39 PST
Created attachment 334677 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 9 Myles C. Maxfield 2018-02-27 19:34:52 PST
Created attachment 334725 [details]
Patch
Comment 10 Sam Weinig 2018-03-21 14:44:43 PDT
Comment on attachment 334725 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This is part of https://bugs.webkit.org/show_bug.cgi?id=183149 for using ICU's bidi facilities
> +        instead of our custom BidiResolver.

I don't think this sentence adds much to to the introductory statement above. I think you should add a "why" clause here. For instance, why is using ICU's bidi facilities rather than our own a desirable thing?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:630
> +    BidiCloser()
> +    {
> +        m_bidi = ubidi_open();

This can be in an initializer list.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:636
> +        if (m_bidi)
> +            ubidi_close(m_bidi);

In what cases is m_bidi null here?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:654
> +    BidiCloser line;
> +    if (!line.bidi())

Can line.bidi() ever be null here?
Comment 11 Sam Weinig 2018-03-21 14:46:18 PDT
Comment on attachment 334725 [details]
Patch

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

> LayoutTests/platform/ios/TestExpectations:3283
> +# This test requires specific metrics for the Times font.
> +webkit.org/b/183149 fast/canvas/bidi.html [ ImageOnlyFailure ]

Is there a way to use something like Ahem for this to avoid this issue?