RESOLVED FIXED 55603
[GTK] Implement the next bit of the TextInputController (hasMarkedText and markedRange)
https://bugs.webkit.org/show_bug.cgi?id=55603
Summary [GTK] Implement the next bit of the TextInputController (hasMarkedText and ma...
Martin Robinson
Reported 2011-03-02 12:50:11 PST
The TextInputController is still missing hasMarkedText and markedRange. This bug tracks implementing those methods and cleaning up the code that's already there.
Attachments
Patch (18.58 KB, patch)
2011-03-02 13:02 PST, Martin Robinson
no flags
Patch with Carlos' suggestions (18.47 KB, patch)
2011-03-03 10:53 PST, Martin Robinson
no flags
Patch which better conforms to the policy stated above (18.50 KB, patch)
2011-03-03 11:08 PST, Martin Robinson
no flags
Patch (18.64 KB, patch)
2011-05-02 13:26 PDT, Martin Robinson
no flags
Patch (18.60 KB, patch)
2011-05-03 12:15 PDT, Martin Robinson
eric: review+
Martin Robinson
Comment 1 2011-03-02 13:02:06 PST
Carlos Garcia Campos
Comment 2 2011-03-03 00:09:41 PST
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
Comment 3 2011-03-03 10:53:51 PST
Created attachment 84586 [details] Patch with Carlos' suggestions
Martin Robinson
Comment 4 2011-03-03 10:57:59 PST
(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
Comment 5 2011-03-03 11:08:27 PST
Created attachment 84590 [details] Patch which better conforms to the policy stated above
Carlos Garcia Campos
Comment 6 2011-03-04 00:55:01 PST
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 7 2011-03-04 08:20:19 PST
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
Comment 8 2011-03-04 08:51:25 PST
(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 9 2011-03-04 09:09:26 PST
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
Comment 10 2011-05-02 13:26:48 PDT
Carlos Garcia Campos
Comment 11 2011-05-02 23:58:41 PDT
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 12 2011-05-03 10:54:39 PDT
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
Comment 13 2011-05-03 12:15:03 PDT
Eric Seidel (no email)
Comment 14 2011-06-02 08:01:22 PDT
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
Comment 15 2011-06-09 15:53:50 PDT
(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
Comment 16 2011-06-09 16:34:00 PDT
Note You need to log in before you can comment on or make changes to this bug.