Bug 75141 - ProcessingInstruction should not be a ContainerNode
: ProcessingInstruction should not be a ContainerNode
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Adam Klein
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-22 15:42 PST by Adam Klein
Modified: 2012-01-31 17:54 PST (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-12-22 15:42:07 PST
ProcessingInstruction should not be a ContainerNode
Comment 1 Adam Klein 2011-12-22 15:47:00 PST
Created attachment 120397 [details]
Patch
Comment 2 Darin Adler 2011-12-22 16:22:51 PST
Comment on attachment 120397 [details]
Patch

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 Adam Klein 2011-12-22 16:29:05 PST
(In reply to comment #2)
> (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?

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 WebKit Review Bot 2011-12-22 16:37:44 PST
Comment on attachment 120397 [details]
Patch

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 Ojan Vafai 2011-12-23 10:25:08 PST
(In reply to comment #3)
> (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.

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 Ryosuke Niwa 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 Ojan Vafai 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 10 Adam Klein 2012-01-31 16:17:03 PST
Created attachment 124842 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-01-31 17:54:26 PST
Comment on attachment 124842 [details]
Patch for landing

Clearing flags on attachment: 124842

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