Bug 75141 - ProcessingInstruction should not be a ContainerNode
: ProcessingInstruction should not be a ContainerNode
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-12-22 15:42 PST by
Modified: 2012-01-31 17:54 PST (History)


Attachments
Patch (8.01 KB, patch)
2011-12-22 15:47 PST, Adam Klein
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (10.71 KB, patch)
2012-01-31 16:17 PST, Adam Klein
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-12-22 15:42:07 PST
ProcessingInstruction should not be a ContainerNode
------- Comment #1 From 2011-12-22 15:47:00 PST -------
Created an attachment (id=120397) [details]
Patch
------- Comment #2 From 2011-12-22 16:22:51 PST -------
(From update of attachment 120397 [details])
This changes behavior. When you call appendChild on a ProcessingInstruction and pass null for the child, you will get HIERARCHY_REQUEST_ERR now, and before you would get NOT_FOUND_ERR. While unimportant, it would be nice to get this edge case right. Could we add tests covering that behavior?
------- Comment #3 From 2011-12-22 16:29:05 PST -------
(In reply to comment #2)
> (From update of attachment 120397 [details] [details])
> This changes behavior. When you call appendChild on a ProcessingInstruction and pass null for the child, you will get HIERARCHY_REQUEST_ERR now, and before you would get NOT_FOUND_ERR. While unimportant, it would be nice to get this edge case right. Could we add tests covering that behavior?

An interesting point. Perhaps Node::appendChild ought to throw NOT_FOUND_ERR if its argument is null, or HIERARCHY_REQUEST_ERR if non-null.  Agreed that it'd be nice to have test coverage in either case for all the various node types.
------- Comment #4 From 2011-12-22 16:37:44 PST -------
(From update of attachment 120397 [details])
Attachment 120397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10939086

New failing tests:
media/W3C/video/networkState/networkState_during_progress.html
http/tests/inspector/resource-tree/resource-tree-document-url.html
------- Comment #5 From 2011-12-23 10:25:08 PST -------
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 120397 [details] [details] [details])
> > This changes behavior. When you call appendChild on a ProcessingInstruction and pass null for the child, you will get HIERARCHY_REQUEST_ERR now, and before you would get NOT_FOUND_ERR. While unimportant, it would be nice to get this edge case right. Could we add tests covering that behavior?
> 
> An interesting point. Perhaps Node::appendChild ought to throw NOT_FOUND_ERR if its argument is null, or HIERARCHY_REQUEST_ERR if non-null.  Agreed that it'd be nice to have test coverage in either case for all the various node types.

I don't think it's a big deal to change the error type if we're not violating the spec. I highly doubt there is code that depends on throwing a NOT_FOUND_ERR instead of a HIERARCHY_REQUEST_ERR.

We should do what we can to be inline with the new spec or get the spec changed to make the code more simple: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-node-pre-insert.
------- Comment #6 From 2011-12-23 11:53:15 PST -------
According to http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-node-pre-insert:

Step 3: If parent is not a Document, DocumentFragment, or Element node, throw a "HierarchyRequestError" and terminate these steps.
...
Step 4.1: If node is not a DocumentFragment, DocumentType, Element, ProcessingInstruction, or Comment node, throw a "HierarchyRequestError" and terminate these steps.
...
Step 5: Otherwise if parent is a DocumentFragment or Element node and node is not a DocumentFragment, Element, Text, ProcessingInstruction, or Comment node, throw a "HierarchyRequestError" and terminate these steps.

So we should be throwing HierarchyRequestError in both of those cases.
------- Comment #7 From 2011-12-23 12:53:08 PST -------
Yup, HIERARCHY_REQUEST_ERR seems right. I was a little confused by step 1:
"If child is not null and its parent is not parent, throw a "NotFoundError" exception and terminate these steps."

I now understand that to be talking about parent.insertBefore(newChild, oldChild) and the case where oldChild is not actually a child of parent. For that we should throw NOT_FOUND_ERR, otherwise, ProcessingInstruction should always throw HIERARCHY_REQUEST_ERR for all the DOM insertion methods. 

That said, it might make sense to change the spec here to reorder items 1-3 to the following:
1. If parent is not a Document, DocumentFragment, or Element node, throw a "HierarchyRequestError" and terminate these steps.
2. If node is parent or an ancestor of parent, throw a "HierarchyRequestError" and terminate these steps.
3. If child is not null and its parent is not parent, throw a "NotFoundError" exception and terminate these steps.

It's not a big deal, but it would somewhat simplify implementation.

In either case, I'm fine with this patch going in as long as Adam adds a test verifying the behavior (even in a followup patch if he prefers).
------- Comment #8 From 2011-12-23 13:00:42 PST -------
http://lists.w3.org/Archives/Public/www-dom/2011OctDec/0271.html
------- Comment #10 From 2012-01-31 16:17:03 PST -------
Created an attachment (id=124842) [details]
Patch for landing
------- Comment #11 From 2012-01-31 17:54:26 PST -------
(From update of attachment 124842 [details])
Clearing flags on attachment: 124842

Committed r106418: <http://trac.webkit.org/changeset/106418>
------- Comment #12 From 2012-01-31 17:54:31 PST -------
All reviewed patches have been landed.  Closing bug.