GNOME Bugzilla – Bug 794395
Use the new JavaScriptCore GLib API instead of DOM API
Last modified: 2018-04-19 08:12:26 UTC
We are working on a new glib based JSC API that will deprecate most of the DOM bindings API. The idea is to use JavaScript and this new API to do the same we are currently doing with the DOM bindings. The new API is still under development, but we should migrate to use it as soon as it's available in a new unstable WebKit release to have enough time during the development cycle to test it.
Created attachment 369774 [details] [review] Initial patch This is the initial patch to port the extension to use the new API. I've tried to port things 1 to 1, without changing it too much to avoid (or at least minimize) regressions, but there's room for improvement now. The overview part is not done yet, that deserves a different patch.
Review of attachment 369774 [details] [review]: Very, very nice, r=me. My main concern are the parts I say are fragile. What happens when JavaScript is disabled? Ideally our own JS code would run irrespective of that setting, or it will be very hard to ever implement NoScript, which we really need to do. ::: embed/ephy-web-view.c @@ -3104,3 @@ - cancellable, - (GAsyncReadyCallback)has_modified_forms_cb, - g_object_ref (task)); You removed this ref, but left the corresponding unref in has_modified_forms_cb. Same for get_best_web_app_icon and get_web_app_title. Was this leaking before? @@ +3231,3 @@ + if (js_result) { + JSCValue *js_value; + char *retval = NULL; Nit: don't indent like this, it's not Epiphany style ::: embed/web-extension/ephy-web-extension.c @@ +302,3 @@ +static char * +save_auth_requester (uint64_t page_id, I'd rather stick with guint64, here and throughout this file, since it's guaranteed to be identical and is more idiomatic in GNOME. @@ +327,3 @@ + g_hash_table_insert (ephy_web_extension_get_form_auth_data_save_requests (ephy_web_extension_get ()), + GINT_TO_POINTER (request_id), save_auth_request_new (origin, target_origin, username, + password, username_field_name, Nit: the indentation is not right here @@ +427,3 @@ + G_TYPE_UINT64, G_TYPE_BOOLEAN); + remember_passwords = extension->password_manager && + g_settings_get_boolean (EPHY_SETTINGS_WEB, EPHY_PREFS_WEB_REMEMBER_PASSWORDS); Another indentation nit: g_settings_get_boolean should be aligned with extension->password_manager on the line above. @@ +469,2 @@ + frame = webkit_web_page_get_main_frame (web_page); + js_context = webkit_frame_get_js_context (frame); Does the return value of webkit_frame_get_js_context() really need to be transfer full? @@ +852,3 @@ +static void +js_log (const char* message) const char *message @@ +869,3 @@ + jsc_class_add_method (js_class, + "permission", + G_CALLBACK (ephy_permissions_manager_get_permission), NULL, NULL, This seems really risky. It's going to break next time we edit the declaration of ephy_permissions_manager_get_permission. I think we need another layer of indirection here: add a wrapper function that just calls the EphyPermissionsManager function, that way we'll get a nice compiler error if it ever changes. @@ +877,3 @@ + g_object_unref (js_permissions_manager); + + js_enum = jsc_value_new_object (js_context, NULL, NULL); Hm, this looks safer... but perhaps we should move this entire block of code to EphyPermissionsManager itself. ephy_permissions_manager_get_js_permission_enum() or something like that? We should avoid having a big distance between where the C types are declared and the code for the conversion to the JS type. @@ +968,3 @@ + JSCContext *js_context = jsc_context_get_current (); + JSCValue *executor, *promise, *retval; + PasswordManagerPromiseData data = { password_manager, origin, target_origin, username, username_field, password_field }; I don't think this is safe; surely you need to g_strdup() all the strings, right? Could they be garbage collected before the callback executes? If not, then what are the lifetime semantics here? Seems really weird that the strings might remain valid after the function call completes. Maybe it would be useful to be able to use G_SIGNAL_TYPE_STATIC_SCOPE (renamed to something else) in jsc_class_add_method() to get the string itself, and get a string copy otherwise, that way you would not have to manually g_strdup() everything. This seems useful to have even if we don't use it in Epiphany. @@ +1011,3 @@ + element = WEBKIT_DOM_HTML_INPUT_ELEMENT (node); + webkit_dom_html_input_element_set_auto_filled (element, TRUE); + webkit_dom_html_input_element_set_editing_value (element, value); That's not good. This should be done with JavaScript instead, because WebKitDOMHTMLInputElement is going to be deprecated, right? Is that hard? @@ +1032,3 @@ + jsc_class_add_method (js_class, + "save", + G_CALLBACK (ephy_password_manager_save), NULL, NULL, Fragile (as discussed above) @@ +1064,3 @@ + + if (WEBKIT_DOM_IS_HTML_TEXT_AREA_ELEMENT (node)) + return webkit_dom_html_text_area_element_is_edited (WEBKIT_DOM_HTML_TEXT_AREA_ELEMENT (node)); What's the plan to eliminate the DOM API use? @@ +1073,3 @@ + +static void +js_exception_handler (JSCContext *context, Could we use this to print a g_warning() when there is a JS exception? That seems useful (as long as it's an exception in our own code), right? @@ +1122,3 @@ + js_function = jsc_value_new_function (js_context, + "isWebApplication", + G_CALLBACK (ephy_dot_dir_is_web_application), NULL, NULL, Fragile @@ +1159,3 @@ + g_signal_connect (webkit_script_world_get_default(), + "window-object-cleared", + G_CALLBACK(window_object_cleared_cb), G_CALLBACK ( ::: embed/web-extension/resources/js/ephy.js @@ +2,3 @@ + +Ephy.formControlsAssociated = function(pageID, forms, serializer, rememberPasswords) +{ Personal preference: I would use GNOME's JS code style here and put the open brace at the close of the previous line. @@ +12,3 @@ + if (rememberPasswords) + formManager.preFillForms(); + Ephy.formManagers.push(formManager); It grows without bound... there should be a way to remove formManagers when the page is closed. @@ +52,3 @@ + return true; + + // A small heuristic here. If there's only one input element This comment belongs above the previous block. @@ +80,3 @@ +} + +Ephy.webAppIcon = function(baseURL) Let's call it Ephy.getWebAppIcon. That's clearer; I don't think it makes sense to follow WebKit coding style here. @@ +165,3 @@ +Ephy.PreFillUserMenu = class PreFillUserMenu +{ + constructor(manager, userElement, users, passwordElement) I'm really not sure how idiomatic this is... I would run this past Dan or another JS person. @@ +371,3 @@ + let users = Ephy.passwordManager.cachedUsers(this._formAuth.url.origin); + if (users.length > 1) { + Ephy.log('More than 1 password saved, hooking menu for choosing which on focus'); "1" -> "one"
Review of attachment 369774 [details] [review]: ::: embed/web-extension/resources/js/ephy.js @@ +3,3 @@ +Ephy.formControlsAssociated = function(pageID, forms, serializer, rememberPasswords) +{ + Ephy.formManagers = []; Doesn't this delete existing form managers if the page has multiple forms (and formControlsAssociated is called multiple times)? @@ +12,3 @@ + if (rememberPasswords) + formManager.preFillForms(); + Ephy.formManagers.push(formManager); Ah, I suppose this JS is run for a page, so when the page gets closed all the objects become eligible for GC, so no need to remove form managers manually. (Right?)
Maybe we should create and use a non-default WebKitScriptWorld here?
(In reply to Michael Catanzaro from comment #2) > Review of attachment 369774 [details] [review] [review]: > > Very, very nice, r=me. Thanks. > My main concern are the parts I say are fragile. > > What happens when JavaScript is disabled? Ideally our own JS code would run > irrespective of that setting, or it will be very hard to ever implement > NoScript, which we really need to do. Good point, I don't know. > ::: embed/ephy-web-view.c > @@ -3104,3 @@ > - cancellable, > - > (GAsyncReadyCallback)has_modified_forms_cb, > - g_object_ref > (task)); > > You removed this ref, but left the corresponding unref in > has_modified_forms_cb. Same for get_best_web_app_icon and get_web_app_title. > Was this leaking before? I also remove the unref in this function. The previous pattern was: task = new Task; if (proxy) complete_task_async (ref task); else complete_task_here (task); unref task. Now we are always handling the task asynchronously, so we don't need to ref it nor unref it, just transfer it. > @@ +3231,3 @@ > + if (js_result) { > + JSCValue *js_value; > + char *retval = NULL; > > Nit: don't indent like this, it's not Epiphany style Sure. > ::: embed/web-extension/ephy-web-extension.c > @@ +302,3 @@ > > +static char * > +save_auth_requester (uint64_t page_id, > > I'd rather stick with guint64, here and throughout this file, since it's > guaranteed to be identical and is more idiomatic in GNOME. Indeed. > @@ +327,3 @@ > + g_hash_table_insert (ephy_web_extension_get_form_auth_data_save_requests > (ephy_web_extension_get ()), > + GINT_TO_POINTER (request_id), save_auth_request_new > (origin, target_origin, username, > + > password, username_field_name, > > Nit: the indentation is not right here Yes, I don't know what happened there. > @@ +427,3 @@ > + G_TYPE_UINT64, G_TYPE_BOOLEAN); > + remember_passwords = extension->password_manager && > + g_settings_get_boolean (EPHY_SETTINGS_WEB, > EPHY_PREFS_WEB_REMEMBER_PASSWORDS); > > Another indentation nit: g_settings_get_boolean should be aligned with > extension->password_manager on the line above. Ok. > @@ +469,2 @@ > + frame = webkit_web_page_get_main_frame (web_page); > + js_context = webkit_frame_get_js_context (frame); > > Does the return value of webkit_frame_get_js_context() really need to be > transfer full? Well, this is something we can still decide. JSCContext and JSValue are actually wrappers than can be think as temporary to handle the context, they don't represent the context or values. You can unref a JSCvalue and its wrapped value will stay alive, if you create another wrapper for the same value, you will get the same result. That's why in the end I decided to make everything transfer full, but it's open to discussion for the entire jsc glib api. In this case, making the function transfer none would force us to keep a reference of the context in the frame. Note that JSCContext retains the context and JSCValue protects the js value from being garbage collected. The best way to ensure the garbage collector will work as expected is to release our references as soon as we don't need those values anymore. > @@ +852,3 @@ > > +static void > +js_log (const char* message) > > const char *message Oops :-) > @@ +869,3 @@ > + jsc_class_add_method (js_class, > + "permission", > + G_CALLBACK > (ephy_permissions_manager_get_permission), NULL, NULL, > > This seems really risky. It's going to break next time we edit the > declaration of ephy_permissions_manager_get_permission. I think we need > another layer of indirection here: add a wrapper function that just calls > the EphyPermissionsManager function, that way we'll get a nice compiler > error if it ever changes. that's a good point. I designed the jsc glib API with the idea of being able to expose methods directly this way, but I see your point. > @@ +877,3 @@ > + g_object_unref (js_permissions_manager); > + > + js_enum = jsc_value_new_object (js_context, NULL, NULL); > > Hm, this looks safer... but perhaps we should move this entire block of code > to EphyPermissionsManager itself. > ephy_permissions_manager_get_js_permission_enum() or something like that? We > should avoid having a big distance between where the C types are declared > and the code for the conversion to the JS type. This is a good point too. I thought it was better that exported objects don't need to know anything about javascript, but again I see your point of having the code closer to its "bindings". > @@ +968,3 @@ > + JSCContext *js_context = jsc_context_get_current (); > + JSCValue *executor, *promise, *retval; > + PasswordManagerPromiseData data = { password_manager, origin, > target_origin, username, username_field, password_field }; > > I don't think this is safe; surely you need to g_strdup() all the strings, > right? Surely no :-) > Could they be garbage collected before the callback executes? No. > If not, > then what are the lifetime semantics here? Seems really weird that the > strings might remain valid after the function call completes. This is indeed tricky, you need to understand how promises work in JavaScript. I was thing about adding convenience API to handle promises in jsc glib API. The executor function is a construct parameter of Promise and the Promise constructor always calls the executor. Note that this is all synchronous, so jsc_value_constructor_call (promise, JSC_TYPE_VALUE, executor, G_TYPE_NONE); will call js_password_manager_query_resolve() always. When js_password_manager_query finishes the excutor has always been called, so we don't need to duplicate anything. Nothing can be garbage collected because the executor is protected until g_object_unref is called after jsc_value_constructor_call(). > Maybe it would be useful to be able to use G_SIGNAL_TYPE_STATIC_SCOPE > (renamed to something else) in jsc_class_add_method() to get the string > itself, and get a string copy otherwise, that way you would not have to > manually g_strdup() everything. This seems useful to have even if we don't > use it in Epiphany. This has nothing to do with JSCClass, you can use this with any js objects not only the ones exposed with JSCClass. > @@ +1011,3 @@ > + element = WEBKIT_DOM_HTML_INPUT_ELEMENT (node); > + webkit_dom_html_input_element_set_auto_filled (element, TRUE); > + webkit_dom_html_input_element_set_editing_value (element, value); > > That's not good. This should be done with JavaScript instead, because > WebKitDOMHTMLInputElement is going to be deprecated, right? Is that hard? Not exactly. Those methods don't exist in JavaScript, they are internal methods of WebKit, that's also why we need to keep a minimal DOM API. But WebKitDOMHTMLInputElement will be deprecated, so these methods will be moved to WebKitDOMElement with runtime checks to ensure they are HTMLInputElement, doing nothing otherwise (or showing a warning, I don't know). This is based on WebKit WIP patches, I haven't done that part in WebKit yet. > @@ +1032,3 @@ > + jsc_class_add_method (js_class, > + "save", > + G_CALLBACK (ephy_password_manager_save), NULL, NULL, > > Fragile (as discussed above) > > @@ +1064,3 @@ > + > + if (WEBKIT_DOM_IS_HTML_TEXT_AREA_ELEMENT (node)) > + return webkit_dom_html_text_area_element_is_edited > (WEBKIT_DOM_HTML_TEXT_AREA_ELEMENT (node)); > > What's the plan to eliminate the DOM API use? These will also be moved to WebKitDOMElement, because these are also internal WebKit API, not part of the JS DOM API. > @@ +1073,3 @@ > + > +static void > +js_exception_handler (JSCContext *context, > > Could we use this to print a g_warning() when there is a JS exception? That > seems useful (as long as it's an exception in our own code), right? What I do is enable the setting to send console messages to stdout. We could indeed a g_warning, yes. > @@ +1122,3 @@ > + js_function = jsc_value_new_function (js_context, > + "isWebApplication", > + G_CALLBACK > (ephy_dot_dir_is_web_application), NULL, NULL, > > Fragile > > @@ +1159,3 @@ > + g_signal_connect (webkit_script_world_get_default(), > + "window-object-cleared", > + G_CALLBACK(window_object_cleared_cb), > > G_CALLBACK ( > > ::: embed/web-extension/resources/js/ephy.js > @@ +2,3 @@ > + > +Ephy.formControlsAssociated = function(pageID, forms, serializer, > rememberPasswords) > +{ > > Personal preference: I would use GNOME's JS code style here and put the open > brace at the close of the previous line. Good thing about using this style is that we can easily keep it consistent, since I'm using check-webkit-style :-) > @@ +12,3 @@ > + if (rememberPasswords) > + formManager.preFillForms(); > + Ephy.formManagers.push(formManager); > > It grows without bound... there should be a way to remove formManagers when > the page is closed. That's what the garbage collector does :-) > @@ +52,3 @@ > + return true; > + > + // A small heuristic here. If there's only one input element > > This comment belongs above the previous block. > > @@ +80,3 @@ > +} > + > +Ephy.webAppIcon = function(baseURL) > > Let's call it Ephy.getWebAppIcon. That's clearer; I don't think it makes > sense to follow WebKit coding style here. > > @@ +165,3 @@ > +Ephy.PreFillUserMenu = class PreFillUserMenu > +{ > + constructor(manager, userElement, users, passwordElement) > > I'm really not sure how idiomatic this is... I would run this past Dan or > another JS person. what's wrong? > @@ +371,3 @@ > + let users = > Ephy.passwordManager.cachedUsers(this._formAuth.url.origin); > + if (users.length > 1) { > + Ephy.log('More than 1 password saved, hooking menu for > choosing which on focus'); > > "1" -> "one" All log messages are literally copied.
> Now we are always handling the task asynchronously, so we don't need to ref > it nor unref it, just transfer it. Ah, I missed that there was a matching unref in the same function. > > @@ +165,3 @@ > > +Ephy.PreFillUserMenu = class PreFillUserMenu > > +{ > > + constructor(manager, userElement, users, passwordElement) > > > > I'm really not sure how idiomatic this is... I would run this past Dan or > > another JS person. > > what's wrong? Uhh, I don't know, it just looked unfamiliar to me... but now I see it's an ES6 class, and I don't know anything about those. > > @@ +371,3 @@ > > + let users = > > Ephy.passwordManager.cachedUsers(this._formAuth.url.origin); > > + if (users.length > 1) { > > + Ephy.log('More than 1 password saved, hooking menu for > > choosing which on focus'); > > > > "1" -> "one" > > All log messages are literally copied. Seems like a good opportunity to improve the log message.
Created attachment 370053 [details] [review] web-extension: Use the new JavaScriptCore GLib API instead of DOM API Updated patch. The overview is still pending, but I will add it in a different patch because it requires new jsc glib api.
Created attachment 370201 [details] [review] web-extension: Use the new JavaScriptCore GLib API instead of DOM API
Created attachment 370202 [details] [review] overview: Use the new JavaScriptCore GLib API instead of DOM API
Review of attachment 370201 [details] [review]: OK, this is good to commit after 2.21.1 is released, just don't forget to require it in the toplevel meson.build. ::: lib/ephy-permissions-manager.c @@ +420,3 @@ + +static EphyPermission +js_permissions_manager_get_permission (EphyPermissionsManager *manager, Yes, good stuff!
Review of attachment 370202 [details] [review]: OK, let's do it. Bonus points if you make it work when JavaScript is disabled, or we have no chance of ever bringing back the enable-javascript setting or writing a NoScript. ::: embed/ephy-about-handler.c @@ +438,3 @@ " <meta name=\"viewport\" content=\"width=device-width\">" " <link href=\""EPHY_PAGE_TEMPLATE_ABOUT_CSS "\" rel=\"stylesheet\" type=\"text/css\">\n" + " <script> </script>\n" You really have to leave an empty script tag? Surely not? @@ +484,3 @@ g_string_append_printf (data_str, "<a class=\"overview-item\" title=\"%s\" href=\"%s\">" + " <div class=\"overview-close-button\" title=\"%s\">✖</div>" That's unreadable, it's 2018 so just use Unicode: ✖
(In reply to Michael Catanzaro from comment #11) > That's unreadable, it's 2018 so just use Unicode: ✖ The HTML document is declared to use charset=utf-8, so it should be fine.
(In reply to Michael Catanzaro from comment #11) > Review of attachment 370202 [details] [review] [review]: > > OK, let's do it. > > Bonus points if you make it work when JavaScript is disabled, or we have no > chance of ever bringing back the enable-javascript setting or writing a > NoScript. I need to investigate that. > ::: embed/ephy-about-handler.c > @@ +438,3 @@ > " <meta name=\"viewport\" > content=\"width=device-width\">" > " <link href=\""EPHY_PAGE_TEMPLATE_ABOUT_CSS "\" > rel=\"stylesheet\" type=\"text/css\">\n" > + " <script> </script>\n" > > You really have to leave an empty script tag? Surely not? Yes, need to investigate it too. I don't know id it's a bug or the expected behavior, but window object cleared is not emitted if I remove that script. > @@ +484,3 @@ > g_string_append_printf (data_str, > "<a class=\"overview-item\" title=\"%s\" > href=\"%s\">" > + " <div class=\"overview-close-button\" > title=\"%s\">✖</div>" > > That's unreadable, it's 2018 so just use Unicode: ✖ Sure!
Remember to land it