This is causing several tests to timeout in the bots: fast/forms/validation-bubble-disappears-when-input-detached.html fast/forms/validation-bubble-disappears-when-input-moved.html fast/forms/validation-bubble-disappears-when-input-no-longer-visible.html fast/forms/validation-custom-message.html fast/forms/validation-message-detached-iframe.html fast/forms/validation-message-detached-iframe2.html fast/forms/validation-messages.html I don't know if we really want to implement them or not, I guess it looks better using a GtkPopover instead of the Shadow DOM based UI, and it's easy to implement. For now I'll just skip these tests.
Skipped in r211359
Four new tests have started timing out since r238038 fast/forms/validation-message-on-checkbox.html [ Timeout ] fast/forms/validation-message-on-listbox.html [ Timeout ] fast/forms/validation-message-on-menulist.html [ Timeout ] fast/forms/validation-message-on-radio.html [ Timeout ]
Even after closing bug 234629, tests related to the validation bubble would keep failing due to missing JS APIs for the test runner (e.g., UIController.contentsOfUserInterfaceItem). We may want to add those interfaces here.
Created attachment 449488 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Hrm... Why do mac/ios ports complain about a changeset for the GTK port? -_-a
Comment on attachment 449488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449488&action=review > Source/JavaScriptCore/ChangeLog:11 > + This change enables form-validation-related tests. To this end, two APIs called > + overridePreference and contentsOfUserInterfaceItem are added to UIScriptController. > + The two APIs need some private interface of JavaScriptCore so we suppress > + compiler complains that happen when accessing JSC headers directly in the test runner. I think we can just use the C API from WTR instead of the glib one. > Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:70 > +void WKViewSetMinimumFontSize(WKViewRef viewRef, double fontSize) > +{ > + webkitWebViewBaseSetMinimumFontSize(toImpl(viewRef), fontSize); > +} WKPreferences is exposed in the C API, I could just add WKPreferencesSetMinimumFontSize() instead > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1649 > + if (!g_strcmp0(userInterfaceItem, "validationBubble")) { I would add an early return instead.
Comment on attachment 449488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449488&action=review Thanks for your review. I am not sure if I effectively addressed your comments. Please have a look at the new patch and let me know if it needs more improvement. We can take another cycle. >> Source/JavaScriptCore/ChangeLog:11 >> + compiler complains that happen when accessing JSC headers directly in the test runner. > > I think we can just use the C API from WTR instead of the glib one. Sorry, I am not sure if I understand correctly. Are you talking about the dictionary implementation in WebKitWebViewBase.cpp (i.e., webkitWebViewBaseContentsOfUserInterfaceItem)? >> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:70 >> +} > > WKPreferences is exposed in the C API, I could just add WKPreferencesSetMinimumFontSize() instead O.K. I get the preference by using these APIs, WKPageGroupGetPreferences(WKPageGetPageGroup(WKViewGetPage(viewRef))). Do you think this is the most efficient way to get the WKPreferences here? If you know a better way, please let me know. >> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1649 >> + if (!g_strcmp0(userInterfaceItem, "validationBubble")) { > > I would add an early return instead. Done.
Created attachment 449635 [details] Patch
(In reply to ChangSeok Oh from comment #8) > Comment on attachment 449488 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449488&action=review > > Thanks for your review. I am not sure if I effectively addressed your > comments. Please have a look at the new patch and let me know if it needs > more improvement. We can take another cycle. > > >> Source/JavaScriptCore/ChangeLog:11 > >> + compiler complains that happen when accessing JSC headers directly in the test runner. > > > > I think we can just use the C API from WTR instead of the glib one. > > Sorry, I am not sure if I understand correctly. Are you talking about the > dictionary implementation in WebKitWebViewBase.cpp (i.e., > webkitWebViewBaseContentsOfUserInterfaceItem)? No, I mean we can use the JSC C API, that is already used by WTR, instead of the JSC glib API and then we don't need the changes in jsc headers. > >> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:70 > >> +} > > > > WKPreferences is exposed in the C API, I could just add WKPreferencesSetMinimumFontSize() instead > > O.K. I get the preference by using these APIs, > WKPageGroupGetPreferences(WKPageGetPageGroup(WKViewGetPage(viewRef))). Do > you think this is the most efficient way to get the WKPreferences here? If > you know a better way, please let me know. That's fine, you can also use TestController::platformPreferences(). > >> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1649 > >> + if (!g_strcmp0(userInterfaceItem, "validationBubble")) { > > > > I would add an early return instead. > > Done.
Comment on attachment 449635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449635&action=review > Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:72 > +void WKViewSetMinimumFontSize(WKViewRef viewRef, double fontSize) > +{ > + auto preferences = WKPageGroupGetPreferences(WKPageGetPageGroup(WKViewGetPage(viewRef))); > + WKPreferencesSetMinimumFontSize(preferences, static_cast<uint32_t>(fontSize)); > +} We don't need to expose WKViewSetMinimumFontSize, WTR can use WKPreferencesSetMinimumFontSize directly.
Created attachment 449703 [details] Patch
Comment on attachment 449635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449635&action=review >> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:72 >> +} > > We don't need to expose WKViewSetMinimumFontSize, WTR can use WKPreferencesSetMinimumFontSize directly. O.K.
Comment on attachment 449488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449488&action=review >>>> Source/JavaScriptCore/ChangeLog:11 >>>> + compiler complains that happen when accessing JSC headers directly in the test runner. >>> >>> I think we can just use the C API from WTR instead of the glib one. >> >> Sorry, I am not sure if I understand correctly. Are you talking about the dictionary implementation in WebKitWebViewBase.cpp (i.e., webkitWebViewBaseContentsOfUserInterfaceItem)? > > No, I mean we can use the JSC C API, that is already used by WTR, instead of the JSC glib API and then we don't need the changes in jsc headers. I understood. >>>> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:70 >>>> +} >>> >>> WKPreferences is exposed in the C API, I could just add WKPreferencesSetMinimumFontSize() instead >> >> O.K. I get the preference by using these APIs, WKPageGroupGetPreferences(WKPageGetPageGroup(WKViewGetPage(viewRef))). Do you think this is the most efficient way to get the WKPreferences here? If you know a better way, please let me know. > > That's fine, you can also use TestController::platformPreferences(). platformPreferences is private so I had to make it public.
@KaL, would you have a look at the new patch once again? If possible, I'd like to close this bug this week and move on to another.
Comment on attachment 449703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449703&action=review > Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:177 > +static Ref<JSON::Object> toJSONObject(GVariant* variant) Since we always want the string representation, this could return a String instead of the JSON object. > Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:201 > +static JSStringRef toJSStringRef(GVariant* contentDictionary) > +{ > + auto jsonObject = toJSONObject(contentDictionary); > + return JSStringCreateWithUTF8CString(jsonObject->toJSONString().utf8().data()); > +} And then we don't need this, because we can use WTR::createString()
Created attachment 450084 [details] Patch
Comment on attachment 449703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449703&action=review >> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:177 >> +static Ref<JSON::Object> toJSONObject(GVariant* variant) > > Since we always want the string representation, this could return a String instead of the JSON object. This function is recursive so it needs to return JSON::Object. Please see line 188. >> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:201 >> +} > > And then we don't need this, because we can use WTR::createString() Maybe WTR::createJSString() you mean? This function is removed anyway.
Committed r288666 (246472@main): <https://commits.webkit.org/246472@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450084 [details].
*** Bug 202232 has been marked as a duplicate of this bug. ***