Bug 107131 – [GTK] Implement testRunner::setTextDirection

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Manuel Rego Casasnovas]

fast/html/set-text-direction.html is skipped for this reason.

[Manuel Rego Casasnovas]

Created attachment 183193 [details]
Patch

[Philippe Normand]

Pretty nice! When this one is done the WK2 port might need a similar patch as well.

[WebKit Review Bot]

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.

[Martin Robinson]

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.

[Manuel Rego Casasnovas]

Created attachment 183402 [details]
Patch

[Manuel Rego Casasnovas]

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

[Manuel Rego Casasnovas]

Created attachment 183443 [details]
Patch

Remove test from WK2 TestExpectations file as it was already passing before my patch

[Philippe Normand]

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.

[Manuel Rego Casasnovas]

Created attachment 183459 [details]
Patch

Update patch following comments provided by Philippe. Both attributes GtkTextDirection and gpointer are not used in the method.

[Martin Robinson]

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.

[Manuel Rego Casasnovas]

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

[WebKit Review Bot]

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

[Martin Robinson]

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

[Martin Robinson]

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

[Manuel Rego Casasnovas]

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.

[Martin Robinson]

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.

[Manuel Rego Casasnovas]

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

[Martin Robinson]

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

[WebKit Review Bot]

Comment on attachment 183480 [details]
Patch

Rejecting attachment 183480 [details] from commit-queue.

[e-mail] 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.

[Martin Robinson]

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.

[Manuel Rego Casasnovas]

Created attachment 183915 [details]
Patch

Modified resetDefaultsToConsistentValues to reset the direction to the default value.

[WebKit Review Bot]

Comment on attachment 183915 [details]
Patch

Clearing flags on attachment: 183915

Committed r140398: <http://trac.webkit.org/changeset/140398>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.