[webkit-reviews] review granted: [Bug 183448] [GTK][WPE] Add API to convert between DOM and JSCValue : [Attachment 336280] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 183448: [GTK][WPE] Add API to convert between DOM and JSCValue
https://bugs.webkit.org/show_bug.cgi?id=183448

Attachment 336280: Patch

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




--- Comment #8 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 336280
  --> https://bugs.webkit.org/attachment.cgi?id=336280
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.