Bug 95303 – [WK2] Text direction API for WK2

• Status: NEW
• Component: WebKit2

[Mikhail Pozdnyakov]

A new type representing WebCore::StringWithDirection is to be added to WK2 API.
The reason:
1) Have a few places where text direction should be considered (for example setting of web page title)
2) Necessary for bug93470 fix

[Mikhail Pozdnyakov]

Created attachment 161167 [details]
preliminary patch.

A new WKStringWithDirection type wrapping WebCore::StringWithDirection was 
added to WK2 C API. The new type was used in WebFrameLoaderClient::dispatchDidReceiveTitle()
to provide both Bundle client and UI process with the text direction info (this is also needed 
for bug93470).

[Build Bot]

Comment on attachment 161167 [details]
preliminary patch.

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

[Gyuyoung Kim]

Comment on attachment 161167 [details]
preliminary patch.

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

> Source/WebKit2/Shared/API/c/WKStringWithDirection.h:38
> +};

Needs a new line ?

> Source/WebKit2/Shared/WebStringWithDirection.h:31
> +// #include <wtf/OwnPtr.h>

Unneeded code.

> Source/WebKit2/Shared/WebStringWithDirection.h:32
> +// #include <wtf/PassOwnPtr.h>

ditto.

> Source/WebKit2/Shared/WebStringWithDirection.h:56
> +    WebStringWithDirection(const WebCore::StringWithDirection& string)

Missing explicit.

[Build Bot]

Comment on attachment 161167 [details]
preliminary patch.

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

[Chris Dumez]

Comment on attachment 161167 [details]
preliminary patch.

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

> Source/WebKit2/Shared/API/c/WKStringWithDirection.cpp:48
> +WKTextDirection WKStringWithDirectionTextDirection(WKStringWithDirectionRef string)

WKStringWithDirectionTextDirection -> WKStringWithDirectionTextGetDirection ?

[Mikhail Pozdnyakov]

Created attachment 161202 [details]
patch

Fixed win build. Improved ChangeLog record. Took comments from Gyuyoung and Chris into consideration.

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 161167 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161167&action=review
> 
> > Source/WebKit2/Shared/API/c/WKStringWithDirection.h:38
> > +};
> 
> Needs a new line ?
Other enum declarations do not have it (WKFindOptions for example).
> 
> > Source/WebKit2/Shared/WebStringWithDirection.h:56
> > +    WebStringWithDirection(const WebCore::StringWithDirection& string)
> 
> Missing explicit.
OK, however think it is not crucial for a private constructor.

[Build Bot]

Comment on attachment 161202 [details]
patch

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

Comment on attachment 161202 [details]
patch

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

> Source/WebKit2/Shared/API/c/WKStringWithDirection.cpp:35
> +    return toAPI(WebURL::APIType);

This is not the right type

[Sam Weinig]

Comment on attachment 161202 [details]
patch

I don't think there is a good reason to expose this as API.

[Sam Weinig]

This is not API I think we should expose.

[Mikhail Pozdnyakov]

(In reply to comment #11)
> This is not API I think we should expose

Despite all the arguments I still think we need to expose something to provide the client with the info about title direction.. another approach could be exposing TextDirection enum only.

[Mikhail Pozdnyakov]

Created attachment 161446 [details]
patch v2

Exposing only TextDirection enum, passing it as an extra parameter.

[Mikhail Pozdnyakov]

Created attachment 161449 [details]
patch v2

fixed mess with ChangeLog.

[Build Bot]

Comment on attachment 161449 [details]
patch v2

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

[Mikhail Pozdnyakov]

(In reply to comment #15)
> (From update of attachment 161449 [details])
> Attachment 161449 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13684698

error: WebKit2/WKTextDirection.h: No such file or directory
WKTextDirection.h is to be added to mac proj file but it is pretty hard to do without a machine with mac..

[Alexey Proskuryakov]

Comment on attachment 161449 [details]
patch v2

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:94
> -typedef void (*WKBundlePageDidReceiveTitleForFrameCallback)(WKBundlePageRef page, WKStringRef title, WKBundleFrameRef frame, WKTypeRef* userData, const void *clientInfo);
> +typedef void (*WKBundlePageDidReceiveTitleForFrameCallback)(WKBundlePageRef page, WKStringRef title, WKTextDirection titleDirection, WKBundleFrameRef frame, WKTypeRef* userData, const void *clientInfo);

If you change existing API, WebKit nightlies will no longer work with Safari, which is unacceptable.

[Darin Adler]

Comment on attachment 161449 [details]
patch v2

The backward compatible way to add this is to add a new callback but keep the old one working as well.

[Mikhail Pozdnyakov]

(In reply to comment #18)
> (From update of attachment 161449 [details])
> The backward compatible way to add this is to add a new callback but keep the old one working as well.
Thanks for the hint! Could I maybe leave it for another patch? I would keep the client API unchanged yet and provide the title direction info for the Injected Bundle only. Is it acceptable?

[Mikhail Pozdnyakov]

Found quite good explanation of why the information about text direction is needed and how it can be used by browsers here: http://www.i18nguy.com/markup/right-to-left.html

The HTML DIR (http://www.w3.org/TR/html401/struct/dirlang.html) attribute specifies the base direction (LTR, RTL) of text, or sections of text. The base direction can influence the ordering of the display of runs of text of different directions, and the display of directionally neutral text (i.e., characters or sequences of characters that do not have inherent directionality, as defined in the Unicode Character Standard).

And this is the example of rendering by browser the same text with different directions http://www.i18nguy.com/markup/bidi.png .

So think we need to expose text direction API to the client eventually.

[Mikhail Pozdnyakov]

Created attachment 161530 [details]
patch v3

Removed changes from the client API.

[Darin Adler]

(In reply to comment #19)
> I would keep the client API unchanged yet and provide the title direction info for the Injected Bundle only. Is it acceptable?

No. The injected bundle is also part of the API and would also break compatibility with Safari, which has an injected bundle.

[Darin Adler]

Comment on attachment 161530 [details]
patch v3

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:94
> -typedef void (*WKBundlePageDidReceiveTitleForFrameCallback)(WKBundlePageRef page, WKStringRef title, WKBundleFrameRef frame, WKTypeRef* userData, const void *clientInfo);
> +typedef void (*WKBundlePageDidReceiveTitleForFrameCallback)(WKBundlePageRef page, WKStringRef title, WKTextDirection titleDirection, WKBundleFrameRef frame, WKTypeRef* userData, const void *clientInfo);

Same problem here as with the last patch, albeit in a different place. Changing the arguments here breaks the API. That’s why this header is in a directory named API.

[Sam Weinig]

(In reply to comment #11)
> This is not API I think we should expose.

I talked to my local bidi expert, and he disagrees with me, and we should indeed expose this as API.  So, please add new callbacks.

[Mikhail Pozdnyakov]

Created attachment 161717 [details]
patch v4

Added new callback to WKBundlePage.

[Darin Adler]

Comment on attachment 161717 [details]
patch v4

I don’t understand why we are adding this for the injected bundle only and not the UI process.

[Mikhail Pozdnyakov]

Created attachment 161782 [details]
patch v5

Added also new WKPageDidReceiveTitleWithDirectionForFrameCallback to WKPage

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Build Bot]

Comment on attachment 161782 [details]
patch v5

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

[Mikhail Pozdnyakov]

(In reply to comment #29)
> (From update of attachment 161782 [details])
> Attachment 161782 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13720422

error: WebKit2/WKTextDirection.h: No such file or directory
would need a mac to include it to project file.

[Mikhail Pozdnyakov]

Created attachment 162504 [details]
patch v6

Added new file to mac project. Removed trailing spaces in ChangeLog.

[Gyuyoung Kim]

Comment on attachment 162504 [details]
patch v6

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

> Source/WebKit2/UIProcess/API/C/WKPage.h:134
> +    WKPageDidReceiveTitleWithDirectionForFrameCallback                               didReceiveTitleWithDirectionForFrame;

Nit : It looks you need to follow existing indentation between WKPageDidReceiveTitleWithDirectionForFrameCallback and didReceiveTitleWithDirectionForFrame;

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2121
> +

It looks this line is not needed. 

By the way, it seems to me that you send title to calling function though this comment says that we don't need to send title because it is already done. Is my understanding right ?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:961
> +    // FIXME : use direction of title.

Nit : FIXME : use -> FIXME: Use ?

[Mikhail Pozdnyakov]

(In reply to comment #32)
 
> By the way, it seems to me that you send title to calling function though this comment says that we don't need to send title because it is already done. Is my understanding right ?
> 
First of all, thank you for review!

Did you mean this comment:
 "Do not set title for frame as it is done already in didReceiveTitleForFrame()." ?

this is about setting title which is done in didReceiveTitleForFrame() so there is no need to set it again in the added callback. 
And sending of the title has to be performed for both callbacks.

[Mikhail Pozdnyakov]

Created attachment 162747 [details]
patch v7

Took comment from Gyuyoung into consideration.

[Gyuyoung Kim]

Comment on attachment 162747 [details]
patch v7

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:961
> +    // FIXME : Use direction of title.

It looks this function doesn't test a title direction though this patch adds new functionality for title direction. This patch just changes function name, right ?  IMO, you have to support the title direction in order to land this patch. BTW, don't you need to keep didReceiveTitleForFrame for existing legacy system ?

[Mikhail Pozdnyakov]

(In reply to comment #35)
> (From update of attachment 162747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162747&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:961
> > +    // FIXME : Use direction of title.
> 
> It looks this function doesn't test a title direction though this patch adds new functionality for title direction. This patch just changes function name, right ?  IMO, you have to support the title direction in order to land this patch.
This is client WTR code. The dependent bug93470 is targeted to use the text direction info so that layout tests can be unskipped.

> BTW, don't you need to keep didReceiveTitleForFrame for existing legacy system ?
All the callbacks are kept in both UI process and bundle client API for legacy reasons, thing is that practically only one callback is needed for the client (we're not going to react on the same notification twice), that is why I substituted the old callback with the new one in WTR.

[Gyuyoung Kim]

Comment on attachment 162747 [details]
patch v7

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

>>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:961
>>> +    // FIXME : Use direction of title.
>> 
>> It looks this function doesn't test a title direction though this patch adds new functionality for title direction. This patch just changes function name, right ?  IMO, you have to support the title direction in order to land this patch. BTW, don't you need to keep didReceiveTitleForFrame for existing legacy system ?
> 
> This is client WTR code. The dependent bug93470 is targeted to use the text direction info so that layout tests can be unskipped.

I think new functionality have to be added with test cases at a time. I wonder how do other reviewers think about this. Looks fine except for test cases support.

[Mikhail Pozdnyakov]

Sam, Anders, could you please take a look?

[Mikhail Pozdnyakov]

Created attachment 172403 [details]
rebased

[Gyuyoung Kim]

Comment on attachment 172403 [details]
rebased

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

> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:860
> +    case WebCore::RTL:

It would be good if you place this according to enum order.

enum TextDirection { RTL, LTR };

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:333
> +        didReceiveTitleWithDirectionForFrame

IMO, it would be better if *didReceiveTitleWithDirectionForFrame* is placed next to *didReceiveTitleForFrame*.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:988
> +    // FIXME : use direction of title.

use -> Use ?

[Mikhail Pozdnyakov]

(In reply to comment #40)
> (From update of attachment 172403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172403&action=review
> 
> > Source/WebKit2/Shared/API/c/WKSharedAPICast.h:860
> > +    case WebCore::RTL:
> 
> It would be good if you place this according to enum order.
> 
> enum TextDirection { RTL, LTR };
ok

> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:333
> > +        didReceiveTitleWithDirectionForFrame
> 
> IMO, it would be better if *didReceiveTitleWithDirectionForFrame* is placed next to *didReceiveTitleForFrame*.
think, it would break ABI

> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:988
> > +    // FIXME : use direction of title.
> 
> use -> Use ?
yep

[Mikhail Pozdnyakov]

Created attachment 172543 [details]
patch v9

Took Gyuyoung's comments into consideration (including layout tests unskipping).

[Mikhail Pozdnyakov]

Could please anyone review?

[Anders Carlsson]

Comment on attachment 172543 [details]
patch v9

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.