Bug 183448 – [GTK][WPE] Add API to convert between DOM and JSCValue

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

For GTK+ port we will deprecate most of the DOM bindings API, we will leave only a few classes Node, Element, Document, etc. and even for those most of their methods and properties will be deprecated. We will only leave API for things that can't be done with JavaScript.
For WPE we will add a minimum DOM API, that will allow us to expose the web extensions API we currently disable because it uses DOM bindings.

In both cases we need a way to get a JSCValue from a WebKitDOMObject and every DOM object will also provide a factory method to create an instance from a JSCValue.

[Carlos Garcia Campos]

Created attachment 335294 [details]
WIP patch

[Carlos Garcia Campos]

Created attachment 335931 [details]
WIP patch

[Michael Catanzaro]

Comment on attachment 335931 [details]
WIP patch

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

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitFrame.cpp:27
>  #include <jsc/JSCContextPrivate.h>

Alphabetize jsc below WebCore and above wtf: all the uppercase headers come first.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitFrame.cpp:154
> +JSCValue* webkit_frame_get_js_value_for_dom_object(WebKitFrame* frame, WebKitDOMObject* domObject)
> +{
> +    return webkit_frame_get_js_value_for_dom_object_in_script_world(frame, domObject, webkit_script_world_get_default());
> +}

Would it make sense to force users to explicitly pass a WebKitScriptWorld? Could prevent mistakes?

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitFrame.cpp:168
> +        JSC::JSLockHolder lock(exec);

So why is a lock needed here, but not in webkit_dom_note_for_js_value()... because the JSC::ExecState is used?

> Tools/TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.cpp:47
> +    g_print("Leaked objects in WebProcess:");
> +    for (const auto object : s_watchedObjects)
> +        g_print(" %s(%p)", g_type_name_from_instance(reinterpret_cast<GTypeInstance*>(object)), object);
> +    g_print("\n");

This should use g_fprintf and stderr, so that it causes the test to fail.

> Tools/TestWebKitAPI/Tests/WebKitGtk/EditorTest.cpp:34
> +    bool testSelectionchanged(WebKitWebPage* page)

testSelectionChanged

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 335931 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335931&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitFrame.cpp:27
> >  #include <jsc/JSCContextPrivate.h>
> 
> Alphabetize jsc below WebCore and above wtf: all the uppercase headers come
> first.
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitFrame.cpp:154
> > +JSCValue* webkit_frame_get_js_value_for_dom_object(WebKitFrame* frame, WebKitDOMObject* domObject)
> > +{
> > +    return webkit_frame_get_js_value_for_dom_object_in_script_world(frame, domObject, webkit_script_world_get_default());
> > +}
> 
> Would it make sense to force users to explicitly pass a WebKitScriptWorld?
> Could prevent mistakes?

99% of the cases you want to use the default, so this is a convenience function to get the value using the default world and will be documented as such. It's equivalent to webkit_frame_get_js_context and get_js_context_for_scripot_world.

> > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitFrame.cpp:168
> > +        JSC::JSLockHolder lock(exec);
> 
> So why is a lock needed here, but not in webkit_dom_note_for_js_value()...
> because the JSC::ExecState is used?

We are creating a new value in the context here to wrap a node, while when getting the wrapped node, we are just querying an existing value. toWrapped is just a jsDynamicCast in the end.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.cpp:47
> > +    g_print("Leaked objects in WebProcess:");
> > +    for (const auto object : s_watchedObjects)
> > +        g_print(" %s(%p)", g_type_name_from_instance(reinterpret_cast<GTypeInstance*>(object)), object);
> > +    g_print("\n");
> 
> This should use g_fprintf and stderr, so that it causes the test to fail.

No, we don't want the test to fail on the first error message, we want to see the whole message, the test will fail anyway because we assert right after this.

> > Tools/TestWebKitAPI/Tests/WebKitGtk/EditorTest.cpp:34
> > +    bool testSelectionchanged(WebKitWebPage* page)
> 
> testSelectionChanged

Right.

[Carlos Garcia Campos]

Created attachment 336279 [details]
Patch

[Carlos Garcia Campos]

Created attachment 336280 [details]
Patch

[Carlos Garcia Campos]

I don't see what's making the build to fail with all the warnings, but it builds fine for me locally with current gtk-doc. The warnings are a gtk-doc issue, IMO, see https://bugzilla.gnome.org/show_bug.cgi?id=794590

[Michael Catanzaro]

Comment on attachment 336280 [details]
Patch

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

> Source/WebKit/PlatformWPE.cmake:169
> +    ${WEBKIT_DIR}/WebProcess/InjectedBundle/API/wpe/DOM/WebKitDOMDocument.h
> +    ${WEBKIT_DIR}/WebProcess/InjectedBundle/API/wpe/DOM/WebKitDOMElement.h
> +    ${WEBKIT_DIR}/WebProcess/InjectedBundle/API/wpe/DOM/WebKitDOMNode.h
> +    ${WEBKIT_DIR}/WebProcess/InjectedBundle/API/wpe/DOM/WebKitDOMObject.h

So these are the DOM APIs that will live on undeprecated. I think it worked out OK for WebKitDOMDocument, WebKitDOMNode, and WebKitDOMObject, but I'm quite uncertain about WebKitDOMElement. The former WebKitDOMHTMLInputElement functions that you are providing there seem like hacked-on afterthoughts. Maybe it would make sense to keep WebKitDOMHTMLInputElement undeprecated as well? Are there any other ways we could provide the required functionality?

> Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:55
> +#if PLATFORM(GTK)
> +    return WEBKIT_DOM_ELEMENT(g_object_new(WEBKIT_DOM_TYPE_ELEMENT, "core-object", coreObject, nullptr));
> +#else
> +    auto* element = WEBKIT_DOM_ELEMENT(g_object_new(WEBKIT_DOM_TYPE_ELEMENT, nullptr));
> +    webkitDOMNodeSetCoreObject(WEBKIT_DOM_NODE(element), static_cast<WebCore::Node*>(coreObject));
> +    return element;
> +#endif

My main concern with this patch is that removing the deprecated stuff from the GTK+ 4 API will be quite difficult. It will be much easier to prepare for that now, rather than later. I would change all the #if PLATFORM(GTK) conditions to #if PLATFORM(GTK) && GTK_CHECK_VERSION(3, 90, 0). That way, our GTK+ 4 API will automatically get the WPE version of the code, which will save us the trouble of updating all these conditions in the future.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:66
> +#if PLATFORM(GTK)
> +G_GNUC_BEGIN_IGNORE_DEPRECATIONS;
> +G_DEFINE_TYPE_WITH_CODE(WebKitDOMElement, webkit_dom_element, WEBKIT_DOM_TYPE_NODE, G_IMPLEMENT_INTERFACE(WEBKIT_DOM_TYPE_EVENT_TARGET, webkitDOMElementDOMEventTargetInit))
> +G_GNUC_END_IGNORE_DEPRECATIONS;
> +#else
> +G_DEFINE_TYPE(WebKitDOMElement, webkit_dom_element, WEBKIT_DOM_TYPE_NODE)
> +#endif

Ditto.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:73
> +#if PLATFORM(GTK)
> +    GObjectClass* gobjectClass = G_OBJECT_CLASS(elementClass);
> +    webkitDOMElementInstallProperties(gobjectClass);
> +#endif

Ditto.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:99
> +    if (is<WebCore::HTMLInputElement>(node))
> +        return downcast<WebCore::HTMLInputElement>(*node).lastChangeWasUserEdit();
> +
> +    if (is<WebCore::HTMLTextAreaElement>(node))
> +        return downcast<WebCore::HTMLTextAreaElement>(*node).lastChangeWasUserEdit();

I'd use g_return_val_if_fail(is<WebCore::HTMLInputElement>(node) || is<WebCore::HTMLTextAreaElement(node)). That would make debugging easier. It's programmer error, right?

> Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:120
> +    if (!is<WebCore::HTMLInputElement>(node))
> +        return false;

Ditto. etc.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #8)
> Maybe it would make sense to keep WebKitDOMHTMLInputElement undeprecated as
> well?

I'm not confident that would be a good idea, it's more of a strawman suggestion. I see the value in getting rid of all the specific DOM classes. But moving its methods up to WebKitDOMElement just doesn't seem terribly clean.

[Michael Catanzaro]

Comment on attachment 336280 [details]
Patch

You also need to add WebKitWebProcessEnumTypes.h to WPE's webkit-web-extension.h to avoid introducing bug #183998 for WPE.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 336280 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336280&action=review
> 
> > Source/WebKit/PlatformWPE.cmake:169
> > +    ${WEBKIT_DIR}/WebProcess/InjectedBundle/API/wpe/DOM/WebKitDOMDocument.h
> > +    ${WEBKIT_DIR}/WebProcess/InjectedBundle/API/wpe/DOM/WebKitDOMElement.h
> > +    ${WEBKIT_DIR}/WebProcess/InjectedBundle/API/wpe/DOM/WebKitDOMNode.h
> > +    ${WEBKIT_DIR}/WebProcess/InjectedBundle/API/wpe/DOM/WebKitDOMObject.h
> 
> So these are the DOM APIs that will live on undeprecated.

For now, there might be more, but not much more, only the ones that don't inherit from Node and have API not available in JS. They will probably show up when trying to migrate evolution.

> I think it worked
> out OK for WebKitDOMDocument, WebKitDOMNode, and WebKitDOMObject, but I'm
> quite uncertain about WebKitDOMElement. The former WebKitDOMHTMLInputElement
> functions that you are providing there seem like hacked-on afterthoughts.
> Maybe it would make sense to keep WebKitDOMHTMLInputElement undeprecated as
> well?

No, that would require to keep the intermediate classes. I don't think we should expose any HTML class at all.

> Are there any other ways we could provide the required functionality?

We could move them to the Node class, like mac does, but I think they make more sense in Element.

> > Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:55
> > +#if PLATFORM(GTK)
> > +    return WEBKIT_DOM_ELEMENT(g_object_new(WEBKIT_DOM_TYPE_ELEMENT, "core-object", coreObject, nullptr));
> > +#else
> > +    auto* element = WEBKIT_DOM_ELEMENT(g_object_new(WEBKIT_DOM_TYPE_ELEMENT, nullptr));
> > +    webkitDOMNodeSetCoreObject(WEBKIT_DOM_NODE(element), static_cast<WebCore::Node*>(coreObject));
> > +    return element;
> > +#endif
> 
> My main concern with this patch is that removing the deprecated stuff from
> the GTK+ 4 API will be quite difficult. It will be much easier to prepare
> for that now, rather than later. I would change all the #if PLATFORM(GTK)
> conditions to #if PLATFORM(GTK) && GTK_CHECK_VERSION(3, 90, 0). That way,
> our GTK+ 4 API will automatically get the WPE version of the code, which
> will save us the trouble of updating all these conditions in the future.

What does GTK+ have to do with this? It's very confusing to make this conditional to a GTK+ version when it has nothing to do with GTK+. If we ever bump the ABI version we will simply remove all deprecated code.

> > Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:66
> > +#if PLATFORM(GTK)
> > +G_GNUC_BEGIN_IGNORE_DEPRECATIONS;
> > +G_DEFINE_TYPE_WITH_CODE(WebKitDOMElement, webkit_dom_element, WEBKIT_DOM_TYPE_NODE, G_IMPLEMENT_INTERFACE(WEBKIT_DOM_TYPE_EVENT_TARGET, webkitDOMElementDOMEventTargetInit))
> > +G_GNUC_END_IGNORE_DEPRECATIONS;
> > +#else
> > +G_DEFINE_TYPE(WebKitDOMElement, webkit_dom_element, WEBKIT_DOM_TYPE_NODE)
> > +#endif
> 
> Ditto.
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:73
> > +#if PLATFORM(GTK)
> > +    GObjectClass* gobjectClass = G_OBJECT_CLASS(elementClass);
> > +    webkitDOMElementInstallProperties(gobjectClass);
> > +#endif
> 
> Ditto.
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:99
> > +    if (is<WebCore::HTMLInputElement>(node))
> > +        return downcast<WebCore::HTMLInputElement>(*node).lastChangeWasUserEdit();
> > +
> > +    if (is<WebCore::HTMLTextAreaElement>(node))
> > +        return downcast<WebCore::HTMLTextAreaElement>(*node).lastChangeWasUserEdit();
> 
> I'd use g_return_val_if_fail(is<WebCore::HTMLInputElement>(node) ||
> is<WebCore::HTMLTextAreaElement(node)). That would make debugging easier.
> It's programmer error, right?

Not necessarily. I think it's ok to call it with any element and assume it returns FALSE. I even add a test case for this.

> > Source/WebKit/WebProcess/InjectedBundle/API/glib/DOM/WebKitDOMElement.cpp:120
> > +    if (!is<WebCore::HTMLInputElement>(node))
> > +        return false;
> 
> Ditto. etc.

[Carlos Garcia Campos]

Created attachment 336509 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r229973: <https://trac.webkit.org/changeset/229973>

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #11)
> For now, there might be more, but not much more, only the ones that don't
> inherit from Node and have API not available in JS. They will probably show
> up when trying to migrate evolution.

They all inherit from WebKitDOMNode, because WebKitDOMElement itself inherits from WebKitDOMNode.

> No, that would require to keep the intermediate classes. I don't think we
> should expose any HTML class at all.

OK.

> What does GTK+ have to do with this? It's very confusing to make this
> conditional to a GTK+ version when it has nothing to do with GTK+. If we
> ever bump the ABI version we will simply remove all deprecated code.

Surely we are going to want to remove this code from the GTK+ 4 API, but we are not going to want to drop support for the GTK+ 3 API at the same time. So we are going to have to go in and add all these conditionals later anyway.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #14)
> (In reply to Carlos Garcia Campos from comment #11)
> > For now, there might be more, but not much more, only the ones that don't
> > inherit from Node and have API not available in JS. They will probably show
> > up when trying to migrate evolution.
> 
> They all inherit from WebKitDOMNode, because WebKitDOMElement itself
> inherits from WebKitDOMNode.

Not at all, WebKitDOMBlob, WebKitDOMCSSRule, WebKitDOMCSSStyleDeclaration, WebKitDOMStyleSheet, WebKitDOMEvent, WebKitDOMRange, and a few other I don't remember now aren't nodes, they inherit from WebKitDOMObject.

> > No, that would require to keep the intermediate classes. I don't think we
> > should expose any HTML class at all.
> 
> OK.
> 
> > What does GTK+ have to do with this? It's very confusing to make this
> > conditional to a GTK+ version when it has nothing to do with GTK+. If we
> > ever bump the ABI version we will simply remove all deprecated code.
> 
> Surely we are going to want to remove this code from the GTK+ 4 API, but we
> are not going to want to drop support for the GTK+ 3 API at the same time.
> So we are going to have to go in and add all these conditionals later anyway.

We will see how to do it, but checking GTK version in glib code shared by GTK and WPE ports is not the right thing.

[Ross Kirsling]

Reopening to attach new patch.

[Ross Kirsling]

Created attachment 336539 [details]
Patch for landing

[Ross Kirsling]

I'm sorry, I don't understand how "webkit-patch land-safely" attached this patch to this ticket! It should have been attached to https://bugs.webkit.org/show_bug.cgi?id=183655.

[Ross Kirsling]

cq-'d the misfired patch. I apologize for the commotion.