[webkit-dev] Prefix naming scheme for DOM-exposed functions

[Darin Adler]

On Dec 7, 2012, at 12:28 PM, Elliott Sprehn <esprehn at chromium.org> wrote:

> This seems like it would introduce bugs and make maintaining the DOM harder since we'd need to duplicate logic.

We should not needlessly duplicate logic. If the change causes us to copy and paste code, we’re doing it wrong.

> Right now we have appendChild() and parserAppendChild(), and using parserAppendChild() for anything not in the parser introduces web observable bugs and changes in behavior.

That seems like an overgeneralization.

The fact that parser-suitable behavior is obtained by calling a separate function rather than by passing some special argument to appendChild doesn’t seem to reflect on this proposal one way or another. Using the wrong function is going to give the wrong result, in both directions. Using appendChild instead of parserAppendChild in the parser would also introduce web observable bugs and change behavior.

A problem with the web-exposed functions is that their names are dictated by the standards and existing code on the web, and so can easily to be misleading for people writing code within WebKit. We have had problems in the past because code doing internal things calls temptingly named web-exposed functions and gets unwanted behavior. One recent example is that Eric Seidel and I suspect, but have not yet proven, that the change to use appendChild instead of parserAppendChild in https://bugs.webkit.org/show_bug.cgi?id=104040 introduced web observable problems.

> We also only have tests for the web exposed methods, unless you're suggesting we add C++ unit tests too? :)

A fair point.

I think we’d still have sufficient test coverage without being able to unit test each function. Currently we have thousands if not tens of thousands of non-web-exposed functions and we test them indirectly through the web-exposed functions, or expose them via internals. The same would be true of these new functions.

I believe we do have C++ unit tests, although we also test as much as possible with the JavaScript ones.

> Can you give an example of what you want to change in the non web exposed appendChild() ?

For internal use, we’d have an appendChild-like function on ContainerNode, not Node, and that takes an Element* rather than a Node*. The web-exposed appendChild needs logic that the internal function would not, to handle failed attempts to append a child to non-container nodes and to handle text nodes. We’d have a separate function for adding text content to a node that should be used instead of creating a text node and then calling appendChild. The bindings-exposed function would call through to the internal functions.

I’m not sure, but I also suspect that our internal appendChild-like function would probably be a single function that encompasses both insertBefore and appendChild.

There are many common web-exposed functions and attributes we don’t ever want to be called internally. Examples from Node.idl are nodeType, considerably slower than the internal techniques to ask the same questions, childNodes, an inefficient way to iterate the children of a node, attributes, an inefficient way to work with attributes, and ownerDocument, a slower version of document that has to do an extra branch because it needs to return 0 when called on a document. Don’t even get me started on isSameNode.

On Dec 7, 2012, at 1:36 PM, Maciej Stachowiak <mjs at apple.com> wrote:

> Would this involve creating a bindingsFoo() for every method foo() that is exposed to bindings? For example, would we have to add XMLHttpRequest::bindingsSend(), even though there's no real need for a special internal XMLHttpRequest::send()?

We’d rename to the prefixed name, rather than adding a new second function. There is no internal WebKit use of XMLHttpRequest::send except to implement other overloads of XMLHttpRequest::send.

> Would getters and setters that map to JavaScript properties (but which do not reflect markup attributes) also need a bindings... version? For example, would we need HTMLMediaElement::bindingsMuted() and HTMLMediaElement::bindingsSetMuted() to wrap the regular muted() and setMuted()?

Yes, we would have to either rename or have two functions. I can see that on the media elements, with a broad interface, that could be a problem. It does seem that setMuted is called in two or three places internally in WebKit code.

> I think this is too much complexity tax on all exposed methods and properties to make up for the benefit. It's likely only a minority of methods where it's highly desirable to have a specialized version for internal use.

Perhaps. On a class like Node it looks like it’s a majority of methods, but on a class like HTMLMediaElement it looks like a minority.

> How does this help WebCore developers know that they should use the internal firstElementChild() instead of the internal firstChild()? I expect both have to exist, because there really are cases where you need to traverse the whole DOM, not just elements, and even if we were to convert, firstElementChild() is not a drop-in replacement.

No, we would not have an internal firstChild function. The idiom for iterating the entire DOM would take some other form where you’d not always have a Node* for a text node.

> It also seems to me that internal methods that do the exact same thing as a bindings...() version but lack an ExceptionCode parameter, we'll still want to avoid excess code duplication, in some cases of tricky algorithms. I would not want a second copy of ContainerNode::insertBefore() and its helper methods that replaces exception checking with preflight checks that return false without setting an exeption code (I don't think you can just skip the checks entirely or you'd make a method that is extremely dangerous to call if you have not met very complex preconditions).

I agree that we don’t want to make second copies of functions.

There are many functions where exception codes are used only for the most trivial kinds of problems, problems that we’d handle with assertions or non-exception defined-behavior in C++ code. For example, Range::startContainer uses its exception code to indicate the range is invalid. In C++ code we’d just return 0. In cases like that, the exception checking can be trivially separated from the real work of the function.

The Range class is a perfect example of the overuse of exceptions in DOM APIs. Few of the exceptions convey any important information and the interface is no good for internal use.

> Possible alternatives:
> - Use something in the IDL to call a bindings... variant only in cases where we know there is a materially better internal method.

I’ll think about it.

You both raised some good points, as well as some points I don’t agree with or find compelling. I’m not as enthusiastic about this idea now as I was before.

The refactoring of CSS and separation between how we manipulate CSS internally from the DOM API to CSS was a huge success, and I’d like to be able to get some of the same wins in the HTML DOM. The way we currently do bindings makes it tricky to do this.

-- Darin