On Dec 7, 2012, at 12:28 PM, Elliott Sprehn espr...@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 m...@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
