Bug 55603 – [GTK] Implement the next bit of the TextInputController (hasMarkedText and markedRange)

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

The TextInputController is still missing hasMarkedText and markedRange. This bug tracks implementing those methods and cleaning up the code that's already there.

[Martin Robinson]

Created attachment 84452 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 84452 [details]
Patch

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

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:499
>      Frame* frame = core(webView)->focusController()->focusedOrMainFrame();
> -    if (!frame)
> -        return;
> +    g_return_if_fail(frame);
>  
>      Editor* editor = frame->editor();
> -    if (!editor)
> -        return;
> -    if (!editor->canEdit() && !editor->hasComposition())
> -        return;
> +    g_return_if_fail(editor);
> +    g_return_if_fail(editor->canEdit() || editor->hasComposition());

I don't think using g_return_* macros everywhere is a good idea. Those macros are not just early returns, are more like an assert that doesn't abort, they log a critical warning, if you have ciriticals=fatal it will crash on an assertion. g_return_* macros are used to protect public methods checking the input and logging a critical message when something is not expected, to easily catch bugs. Since they are inefficient you can build glib with G_DISABLE_CHECKS (it's usually done in production builds for performance reasons) which makes those macros do nothing (#define g_return_if_fail(expr) (void)0;)

[Martin Robinson]

Created attachment 84586 [details]
Patch with Carlos' suggestions

[Martin Robinson]

(In reply to comment #2)
> I don't think using g_return_* macros everywhere is a good idea. Those macros are not just early returns, are more like an assert that doesn't abort, they log a critical warning, if you have ciriticals=fatal it will crash on an assertion. g_return_* macros are used to protect public methods checking the input and logging a critical message when something is not expected, to easily catch bugs. Since they are inefficient you can build glib with G_DISABLE_CHECKS (it's usually done in production builds for performance reasons) which makes those macros do nothing (#define g_return_if_fail(expr) (void)0;)

Excellent point. I've uploaded a new patch that conforms to the following policy:

1. When there is an API that accepts a GObject, arguments are verified with GLib style early returns (g_return_ macros).

2. Assertions are used for non-API arguments and values computed at runtime.

[Martin Robinson]

Created attachment 84590 [details]
Patch which better conforms to the policy stated above

[Carlos Garcia Campos]

Comment on attachment 84590 [details]
Patch which better conforms to the policy stated above

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

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:527
> +    *start = *length = 0;
> +
> +    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), false);
> +    g_return_val_if_fail(start && length, false);
> +

You should move the line *start = *length = 0; after the g_return_val_if_fail(start && length, false);

[Martin Robinson]

Comment on attachment 84590 [details]
Patch which better conforms to the policy stated above

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

>> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:527
>> +
> 
> You should move the line *start = *length = 0; after the g_return_val_if_fail(start && length, false);

I feel that the method should return conssitent values when it fails. Is there a particular reason it's better to move it down? Externally the user of the method couldn't know if it returned false because of a bad argument or one of the following runtime situations.

[Carlos Garcia Campos]

(In reply to comment #7)
> (From update of attachment 84590 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84590&action=review
> 
> >> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:527
> >> +
> > 
> > You should move the line *start = *length = 0; after the g_return_val_if_fail(start && length, false);
> 
> I feel that the method should return conssitent values when it fails. Is there a particular reason it's better to move it down? Externally the user of the method couldn't know if it returned false because of a bad argument or one of the following runtime situations.

The macro checks that valid pointers have been passed, so you shouldn't use the pointers before it.

[Martin Robinson]

Comment on attachment 84590 [details]
Patch which better conforms to the policy stated above

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

>>>> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:527
>>>> +
>>> 
>>> You should move the line *start = *length = 0; after the g_return_val_if_fail(start && length, false);
>> 
>> I feel that the method should return conssitent values when it fails. Is there a particular reason it's better to move it down? Externally the user of the method couldn't know if it returned false because of a bad argument or one of the following runtime situations.
> 
> The macro checks that valid pointers have been passed, so you shouldn't use the pointers before it.

Ah! Quite right. I'll reorder the statement and upload a new patch.

[Martin Robinson]

Created attachment 91964 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 91964 [details]
Patch

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

> Tools/DumpRenderTree/gtk/TextInputController.cpp:96
> -        return JSValueMakeUndefined(context);
> +        JSValueMakeUndefined(context);

Why did you remove the return?

[Martin Robinson]

Comment on attachment 91964 [details]
Patch

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

>> Tools/DumpRenderTree/gtk/TextInputController.cpp:96
>> +        JSValueMakeUndefined(context);
> 
> Why did you remove the return?

I believe this was a mistake. Uploading a new patch now.

[Martin Robinson]

Created attachment 92100 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 92100 [details]
Patch

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

I'm willing to rubber stamp this if that's what you're looking for.  If you need a more detailed review from a gtk person, you should set r? again.

> LayoutTests/platform/gtk/Skipped:666
> +# These tests also use firstRectForCharacterRange, but they generate slightly
> +# different values. (Chromium also fails here; see their test_expectations.txt file)

Do we match chromium?

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:539
> +    *start = *length = 0;

Um.  Why not pass these by reference?

[Martin Robinson]

(In reply to comment #14)
> (From update of attachment 92100 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92100&action=review
> 
> I'm willing to rubber stamp this if that's what you're looking for.  If you need a more detailed review from a gtk person, you should set r? again.
> 
> > LayoutTests/platform/gtk/Skipped:666
> > +# These tests also use firstRectForCharacterRange, but they generate slightly
> > +# different values. (Chromium also fails here; see their test_expectations.txt file)
> 
> Do we match chromium?

I'm not sure, since results are not checked in for these test. The same set of tests fail though, so I assumed it was the same issue.

> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:539
> > +    *start = *length = 0;
> 
> Um.  Why not pass these by reference?

This is more typical in GTK+-land since it also supports C -- though DumpRenderTreeSupport is C++ obviously. The existing methods used this approach, so I followed it. If you prefer these to be references, hopefully it's okay to fix them it in a followup patch.

[Martin Robinson]

Committed r88498: <http://trac.webkit.org/changeset/88498>