Bug 18581 - pre-insert and replace both check type of 'node' in too many places
pre-insert and replace both check type of 'node' in too many places
Status: RESOLVED FIXED
Product: WebAppsWG
Classification: Unclassified
Component: DOM
unspecified
PC Linux
: P2 normal
: ---
Assigned To: Anne
public-webapps-bugzilla
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-15 18:09 UTC by Zack Weinberg
Modified: 2012-11-28 16:01 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Zack Weinberg 2012-08-15 18:09:35 UTC
The pre-insert and replace algorithms (http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-algorithms) have this structure:

  if (parent is not a Document, DocumentFragment, or Element)
    throw HierarchyRequestError;

  if (parent is a Document) {
    if (node is not a DocumentFragment, DocumentType, Element,
        ProcessingInstruction, or Comment)
      throw HierarchyRequestError;

    more checks;
  }
  else if (node is not a DocumentFragment, Element, Text,
           ProcessingInstruction, or Comment)
    throw HierarchyRequestError;

  more operations;

It would be clearer if they had this structure instead:

  if (parent is not a Document, DocumentFragment, or Element)
    throw HierarchyRequestError;

  if (node is not a DocumentFragment, DocumentType, Element, Text,
      ProcessingInstruction, or Comment)
    throw HierarchyRequestError;

  if ((node is a Text and parent is a Document) or
      (node is a DocumentType and parent is not a Document))
    throw HierarchyRequestError;

  if (parent is a Document)
    more checks;

  more operations;

Specifically, it would be clearer that most of the node type constraints apply regardless of the type of 'parent', and there are just a couple of special cases.

I would also recommend moving step 3 ("If child is not null...") of the pre-insert algorithm to be step 1 (and pushing the current steps 1 and 2 down); this would provide more parallel structure with the replace algorithm, and would reduce the chance of confusing the reader about which of 'child' and 'node' is the new node.

I am happy to provide a proper patch to the prose if that would be helpful.
Comment 1 Anne 2012-09-21 10:02:42 UTC
If you could propose a patch on Overview.src.html in https://github.com/whatwg/dom that would be great. 

(Sorry for responding so hopelessly late, I've been on vacation for a while and then focused on the URL Standard.)
Comment 2 Anne 2012-11-28 15:50:06 UTC
https://github.com/whatwg/dom/commit/b529cfcd4013c4f85785fa62d784a00b4403d9c8

I did not move step 3 up because that would change what exceptions get thrown.

I now think we should maybe collapse the first bit of pre-insert and replace together so that it's clear it's the same algorithm (there is actually a minor difference as child cannot be null in replace, but we can point that out).
Comment 3 Anne 2012-11-28 16:01:46 UTC
Only step 1-5 could be shared. I guess for now it's not worth it, unless we get other major changes. I'll make note of that in the source.