Bug 95303

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).

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

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.

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

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 ?

Created attachment 161202 [details] patch Fixed win build. Improved ChangeLog record. Took comments from Gyuyoung and Chris into consideration.

(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.

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

Comment on attachment 161202 [details] patch I don't think there is a good reason to expose this as API.

This is not API I think we should expose.

(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.

Created attachment 161446 [details] patch v2 Exposing only TextDirection enum, passing it as an extra parameter.

Created attachment 161449 [details] patch v2 fixed mess with ChangeLog.

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

(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..

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.

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.

(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?

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.

Created attachment 161530 [details] patch v3 Removed changes from the client API.

(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.

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.

(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.

Created attachment 161717 [details] patch v4 Added new callback to WKBundlePage.

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.

Created attachment 161782 [details] patch v5 Added also new WKPageDidReceiveTitleWithDirectionForFrameCallback to WKPage

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

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

(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.

Created attachment 162504 [details] patch v6 Added new file to mac project. Removed trailing spaces in ChangeLog.

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 ?

(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.

Created attachment 162747 [details] patch v7 Took comment from Gyuyoung into consideration.

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 ?

(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.

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.

Sam, Anders, could you please take a look?

Created attachment 172403 [details] rebased

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 ?

(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

Created attachment 172543 [details] patch v9 Took Gyuyoung's comments into consideration (including layout tests unskipping).

Could please anyone review?

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.