fast/html/set-text-direction.html is skipped for this reason.
Created attachment 183193 [details] Patch
Pretty nice! When this one is done the WK2 port might need a similar patch as well.
Attachment 183193 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebKit/gtk/webkit/webkitwebview.h:467: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
I wonder if it would be better to simply hook into gtk_widget_set_direction here. To be honest, I don't know even about RTL text to say for sure.
Created attachment 183402 [details] Patch
Thanks for your quick review yesterday :-) I've just attached a new patch using gtk_widget_set_direction as proposed by Martin and it works properly. BTW, just a brief comment about the previous patch. The implementation in the previous patch was based in chromium and efl implementations as you can check in: * Source/WebKit/efl/ewk/ewk_view.cpp: ewk_view_text_direction_set * Source/WebKit/chromium/src/WebViewImpl.cpp: WebViewImpl::setTextDirection
Created attachment 183443 [details] Patch Remove test from WK2 TestExpectations file as it was already passing before my patch
Comment on attachment 183443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183443&action=review LGTM, just a couple of nitpicks! > Source/WebKit/gtk/webkit/webkitwebview.cpp:272 > +static void webkit_web_view_direction_changed(WebKitWebView* webView, GtkTextDirection previousDirection, gpointer gpointer); gpointer gpointer? I guess something like data would be better > Source/WebKit/gtk/webkit/webkitwebview.cpp:5367 > +void webkit_web_view_direction_changed(WebKitWebView* webView, GtkTextDirection previousDirection, gpointer gpointer) Well since the gpointer is not used you can omit to name it in the function signature.
Created attachment 183459 [details] Patch Update patch following comments provided by Philippe. Both attributes GtkTextDirection and gpointer are not used in the method.
Comment on attachment 183459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183459&action=review > Source/WebKit/gtk/webkit/webkitwebview.cpp:5367 > +void webkit_web_view_direction_changed(WebKitWebView* webView, GtkTextDirection, gpointer) Okay. One nit here is that this function doesn't follow WebKit naming conventions (we started doing that always at some point). I can fix that and land this. > LayoutTests/platform/gtk-wk2/TestExpectations:-507 > Bug(GTK) fast/events/clear-edit-drag-state.html [ Pass ] > Bug(GTK) fast/events/dont-loose-last-event.html [ Pass ] > Bug(GTK) fast/events/drag-selects-image.html [ Pass ] > Bug(GTK) fast/forms/text-input-event.html [ Pass ] > -Bug(GTK) fast/html/set-text-direction.html [ Pass ] How is the WebKit2 test passing now? Source/WebKit/gtk/webkit/webkitwebview.cpp is only used for WebKit1.
(In reply to comment #10) > (From update of attachment 183459 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183459&action=review > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:5367 > > +void webkit_web_view_direction_changed(WebKitWebView* webView, GtkTextDirection, gpointer) > > Okay. One nit here is that this function doesn't follow WebKit naming conventions (we started doing that always at some point). I can fix that and land this. I can fix it and provide a new patch but I'd like to be sure. Then should I use the following style in the header: static void webkit_web_view_direction_changed(WebKitWebView* webView, GtkTextDirection previousDirection, gpointer data); And in the implementation: void webkit_web_view_direction_changed(WebKitWebView* webView, GtkTextDirection previousDirecton, gpointer data); Am I right? > > LayoutTests/platform/gtk-wk2/TestExpectations:-507 > > Bug(GTK) fast/events/clear-edit-drag-state.html [ Pass ] > > Bug(GTK) fast/events/dont-loose-last-event.html [ Pass ] > > Bug(GTK) fast/events/drag-selects-image.html [ Pass ] > > Bug(GTK) fast/forms/text-input-event.html [ Pass ] > > -Bug(GTK) fast/html/set-text-direction.html [ Pass ] > > How is the WebKit2 test passing now? Source/WebKit/gtk/webkit/webkitwebview.cpp is only used for WebKit1. This patch was already passing in WebKit2, it was in the section "Tests working in WK2 and failing in WK1". That's the reason why I unflagged it in both TestExpection files for WK1 and WK2.
Comment on attachment 183459 [details] Patch Attachment 183459 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15948432 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 183459 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=183459&action=review > > > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:5367 > > > +void webkit_web_view_direction_changed(WebKitWebView* webView, GtkTextDirection, gpointer) > > > > Okay. One nit here is that this function doesn't follow WebKit naming conventions (we started doing that always at some point). I can fix that and land this. > > I can fix it and provide a new patch but I'd like to be sure. > > Then should I use the following style in the header: > static void webkit_web_view_direction_changed(WebKitWebView* webView, GtkTextDirection previousDirection, gpointer data); > > And in the implementation: > void webkit_web_view_direction_changed(WebKitWebView* webView, GtkTextDirection previousDirecton, gpointer data); Essentially if the function is in the public API it would be called: In the c++ file: webkit_web_view_direction_changed(WebkItWebView* webView, GtkTextDirection previousDirection, gpointer data); In the header: webkit_web_view_direction_changed(WebkItWebView* web_view, GtkTextDirection previous_direction, gpointer data); If the function is only used internally it should be called: webkitWebViewDirectionChanged(WebkItWebView* webView, GtkTextDirection previousDirection, gpointer data); The reason the name changes is that we try to follow WebKit function naming style wherever we can. The naming scheme of GObject APIs is not something we can change. The reason that the parameter names change in public headers is to make gtkdoc happy. It's a bit convoluted. :( > > > LayoutTests/platform/gtk-wk2/TestExpectations:-507 > > > Bug(GTK) fast/events/clear-edit-drag-state.html [ Pass ] > > > Bug(GTK) fast/events/dont-loose-last-event.html [ Pass ] > > > Bug(GTK) fast/events/drag-selects-image.html [ Pass ] > > > Bug(GTK) fast/forms/text-input-event.html [ Pass ] > > > -Bug(GTK) fast/html/set-text-direction.html [ Pass ] > > > > How is the WebKit2 test passing now? Source/WebKit/gtk/webkit/webkitwebview.cpp is only used for WebKit1. > > This patch was already passing in WebKit2, it was in the section "Tests working in WK2 and failing in WK1". That's the reason why I unflagged it in both TestExpection files for WK1 and WK2.
(In reply to comment #13) > > This patch was already passing in WebKit2, it was in the section "Tests working in WK2 and failing in WK1". That's the reason why I unflagged it in both TestExpection files for WK1 and WK2. Oh, okay. You should probably wait to change WKTR until you actually implement text direction support in WebKit2.
Created attachment 183480 [details] Patch Fixed style in method. Avoid name for gpointer param as it's not used. Specified name for GtkTextDirection as previousDirection gives interesting information as someone could confuse it with current direction.
Comment on attachment 183480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183480&action=review > Tools/DumpRenderTree/gtk/TestRunnerGtk.cpp:954 > - // FIXME: Implement. > + GOwnPtr<gchar> writingDirection(JSStringCopyUTF8CString(direction)); > + > + WebKitWebView* view = webkit_web_frame_get_web_view(mainFrame); > + ASSERT(view); > + > + if (g_str_equal(writingDirection.get(), "auto")) > + gtk_widget_set_direction(GTK_WIDGET(view), GTK_TEXT_DIR_NONE); > + else if (g_str_equal(writingDirection.get(), "ltr")) > + gtk_widget_set_direction(GTK_WIDGET(view), GTK_TEXT_DIR_LTR); > + else if (g_str_equal(writingDirection.get(), "rtl")) > + gtk_widget_set_direction(GTK_WIDGET(view), GTK_TEXT_DIR_RTL); > + else > + fprintf(stderr, "TestRunner::setTextDirection called with unknown direction: '%s'.\n", writingDirection.get()); > } I think it makes sense to wait on adding this until you make this change in WebKitWebViewBase.cpp as well.
(In reply to comment #16) > (From update of attachment 183480 [details]) > I think it makes sense to wait on adding this until you make this change in WebKitWebViewBase.cpp as well. I think you don't understand me before, the test is already passing in WK2. It's included in the TestExpectations file of WK2 in the section "Tests working in WK2 and failing in WK1". So, this is already working in W2, and I'm just fixing the test in WK1. With my patch the tests passes in both WK1 and WK2, so I've unflagged it in both TestExpectations files (in WK1 it was marked as "Failure" and in WK2 as "Pass"). You can see how it's implemented in WK2 in WebFrame::setTextDirection which is used from TestRunner::setTextDirection (Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp).
(In reply to comment #17) > You can see how it's implemented in WK2 in WebFrame::setTextDirection which is used from TestRunner::setTextDirection (Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp). Sorry, I minterpreted the change to TestRunnerGtk.cpp as a change to WebKitTestRunner. This is passing in WebKit2 likely because WKTR is using the C API to change the text direction. If we want compatibility with WebKit1, you'll also need to change the non-C API version of WebKit2.
Comment on attachment 183480 [details] Patch Rejecting attachment 183480 [details] from commit-queue. rego@igalia.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Oh, I actually realized there might be an issue with this patch. There's a function in DRT called something like resetValuesToConsistentState (probably not the real name). This function ensures that once a test is finished the WebView is in a "clean" state for a new test. You likely need to set the text direction to LTR there.
Created attachment 183915 [details] Patch Modified resetDefaultsToConsistentValues to reset the direction to the default value.
Comment on attachment 183915 [details] Patch Clearing flags on attachment: 183915 Committed r140398: <http://trac.webkit.org/changeset/140398>
All reviewed patches have been landed. Closing bug.