[Webkit-unassigned] [Bug 183448] [GTK][WPE] Add API to convert between DOM and JSCValue

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=183448

--- Comment #11 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180326/914a0772/attachment.html>