Improve 'ContainerNode' exception messages.

Improve 'ContainerNode' exception messages.

BUG=270033
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158367

[Mike West, 2013-09-24 09:28:20]

Jochen, one more in the alphabetical series of core/dom modifications. :)

-mike

[jochen (gone - plz use gerrit), 2013-09-24 09:30:20]

lgtm

[commit-bot: I haz the power, 2013-09-24 09:48:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/23889039/1

[commit-bot: I haz the power, 2013-09-24 11:57:10]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-09-25 08:26:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/23889039/17001

[commit-bot: I haz the power, 2013-09-25 09:34:25]

Failed to apply patch for Source/core/dom/ContainerNode.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/ContainerNode.cpp
  Hunk #2 FAILED at 141.
  Hunk #3 FAILED at 154.
  Hunk #4 FAILED at 163.
  Hunk #5 FAILED at 222.
  Hunk #7 FAILED at 335.
  Hunk #10 FAILED at 452.
  Hunk #11 FAILED at 466.
  Hunk #12 FAILED at 474.
  8 out of 14 hunks FAILED -- saving rejects to file
Source/core/dom/ContainerNode.cpp.rej

Patch:       Source/core/dom/ContainerNode.cpp
Index: Source/core/dom/ContainerNode.cpp
diff --git a/Source/core/dom/ContainerNode.cpp
b/Source/core/dom/ContainerNode.cpp
index
90240809e8c6a21176a52c3d228d33db50a65b2a..62e29ffee2d1cea0caf797efdb01f0748f10398c
100644
--- a/Source/core/dom/ContainerNode.cpp
+++ b/Source/core/dom/ContainerNode.cpp
@@ -23,6 +23,7 @@
 #include "config.h"
 #include "core/dom/ContainerNode.h"
 
+#include "bindings/v8/ExceptionMessages.h"
 #include "bindings/v8/ExceptionState.h"
 #include "bindings/v8/ExceptionStatePlaceholder.h"
 #include "core/dom/ChildListMutationScope.h"
@@ -140,11 +141,11 @@ static inline bool containsConsideringHostElements(const
Node* newChild, const N
         : newChild->contains(newParent);
 }
 
-static inline bool checkAcceptChild(ContainerNode* newParent, Node* newChild,
Node* oldChild, ExceptionState& es)
+static inline bool checkAcceptChild(ContainerNode* newParent, Node* newChild,
Node* oldChild, const String& method, ExceptionState& es)
 {
     // Not mentioned in spec: throw NotFoundError if newChild is null
     if (!newChild) {
-        es.throwDOMException(NotFoundError);
+        es.throwDOMException(NotFoundError,
ExceptionMessages::failedToExecute(method, "ContainerNode", "The new child
element is null."));
         return false;
     }
 
@@ -153,7 +154,7 @@ static inline bool checkAcceptChild(ContainerNode*
newParent, Node* newChild, No
         ASSERT(!newParent->isDocumentTypeNode());
         ASSERT(isChildTypeAllowed(newParent, newChild));
         if (containsConsideringHostElements(newChild, newParent)) {
-            es.throwDOMException(HierarchyRequestError);
+            es.throwDOMException(HierarchyRequestError,
ExceptionMessages::failedToExecute(method, "ContainerNode", "The new child
element contains the parent."));
             return false;
         }
         return true;
@@ -162,48 +163,49 @@ static inline bool checkAcceptChild(ContainerNode*
newParent, Node* newChild, No
     // This should never happen, but also protect release builds from tree
corruption.
     ASSERT(!newChild->isPseudoElement());
     if (newChild->isPseudoElement()) {
-        es.throwDOMException(HierarchyRequestError);
+        es.throwDOMException(HierarchyRequestError,
ExceptionMessages::failedToExecute(method, "ContainerNode", "The new child
element is a pseudo-element."));
         return false;
     }
 
     if (containsConsideringHostElements(newChild, newParent)) {
-        es.throwDOMException(HierarchyRequestError);
+        es.throwDOMException(HierarchyRequestError,
ExceptionMessages::failedToExecute(method, "ContainerNode", "The new child
element contains the parent."));
         return false;
     }
 
     if (oldChild && newParent->isDocumentNode()) {
         if (!toDocument(newParent)->canReplaceChild(newChild, oldChild)) {
-            es.throwDOMException(HierarchyRequestError);
+            // FIXME: Adjust 'Document::canReplaceChild' to return some
additional detail (or an error message).
+            es.throwDOMException(HierarchyRequestError,
ExceptionMessages::failedToExecute(method, "ContainerNode"));
             return false;
         }
     } else if (!isChildTypeAllowed(newParent, newChild)) {
-        es.throwDOMException(HierarchyRequestError);
+        es.throwDOMException(HierarchyRequestError,
ExceptionMessages::failedToExecute(method, "ContainerNode", "Nodes of type '" +
newChild->nodeName() + "' may not be inserted inside nodes of type '" +
newParent->nodeName() + "'."));
         return false;
     }
 
     return true;
 }
 
-static inline bool checkAcceptChildGuaranteedNodeTypes(ContainerNode*
newParent, Node* newChild, ExceptionState& es)
+static inline bool checkAcceptChildGuaranteedNodeTypes(ContainerNode*
newParent, Node* newChild, const String& method, ExceptionState& es)
 {
     ASSERT(!newParent->isDocumentTypeNode());
     ASSERT(isChildTypeAllowed(newParent, newChild));
     if (newChild->contains(newParent)) {
-        es.throwDOMException(HierarchyRequestError);
+        es.throwDOMException(HierarchyRequestError,
ExceptionMessages::failedToExecute(method, "ContainerNode", "The new child
element contains the parent."));
         return false;
     }
 
     return true;
 }
 
-static inline bool checkAddChild(ContainerNode* newParent, Node* newChild,
ExceptionState& es)
+static inline bool checkAddChild(ContainerNode* newParent, Node* newChild,
const String& method, ExceptionState& es)
 {
-    return checkAcceptChild(newParent, newChild, 0, es);
+    return checkAcceptChild(newParent, newChild, 0, method, es);
 }
 
-static inline bool checkReplaceChild(ContainerNode* newParent, Node* newChild,
Node* oldChild, ExceptionState& es)
+static inline bool checkReplaceChild(ContainerNode* newParent, Node* newChild,
Node* oldChild, const String& method, ExceptionState& es)
 {
-    return checkAcceptChild(newParent, newChild, oldChild, es);
+    return checkAcceptChild(newParent, newChild, oldChild, method, es);
 }
 
 void ContainerNode::insertBefore(PassRefPtr<Node> newChild, Node* refChild,
ExceptionState& es)
@@ -221,12 +223,12 @@ void ContainerNode::insertBefore(PassRefPtr<Node>
newChild, Node* refChild, Exce
     }
 
     // Make sure adding the new child is OK.
-    if (!checkAddChild(this, newChild.get(), es))
+    if (!checkAddChild(this, newChild.get(), "insertBefore", es))
         return;
 
     // NotFoundError: Raised if refChild is not a child of this node
     if (refChild->parentNode() != this) {
-        es.throwDOMException(NotFoundError);
+        es.throwDOMException(NotFoundError,
ExceptionMessages::failedToExecute("insertBefore", "ContainerNode", "The node
before which the new node is to be inserted is not a child of this node."));
         return;
     }
 
@@ -243,7 +245,7 @@ void ContainerNode::insertBefore(PassRefPtr<Node> newChild,
Node* refChild, Exce
         return;
 
     // We need this extra check because collectChildrenAndRemoveFromOldParent()
can fire mutation events.
-    if (!checkAcceptChildGuaranteedNodeTypes(this, newChild.get(), es))
+    if (!checkAcceptChildGuaranteedNodeTypes(this, newChild.get(),
"insertBefore", es))
         return;
 
     InspectorInstrumentation::willInsertDOMNode(&document(), this);
@@ -334,17 +336,17 @@ void ContainerNode::replaceChild(PassRefPtr<Node>
newChild, Node* oldChild, Exce
         return;
 
     if (!oldChild) {
-        es.throwDOMException(NotFoundError);
+        es.throwDOMException(NotFoundError,
ExceptionMessages::failedToExecute("replaceChild", "ContainerNode", "The node to
be replaced is null."));
         return;
     }
 
     // Make sure replacing the old child with the new is ok
-    if (!checkReplaceChild(this, newChild.get(), oldChild, es))
+    if (!checkReplaceChild(this, newChild.get(), oldChild, "replaceChild", es))
         return;
 
     // NotFoundError: Raised if oldChild is not a child of this node.
     if (oldChild->parentNode() != this) {
-        es.throwDOMException(NotFoundError);
+        es.throwDOMException(NotFoundError,
ExceptionMessages::failedToExecute("replaceChild", "ContainerNode", "The node to
be replaced is not a child of this node."));
         return;
     }
 
@@ -362,7 +364,7 @@ void ContainerNode::replaceChild(PassRefPtr<Node> newChild,
Node* oldChild, Exce
         return;
 
     // Does this one more time because removeChild() fires a MutationEvent.
-    if (!checkReplaceChild(this, newChild.get(), oldChild, es))
+    if (!checkReplaceChild(this, newChild.get(), oldChild, "replaceChild", es))
         return;
 
     NodeVector targets;
@@ -371,7 +373,7 @@ void ContainerNode::replaceChild(PassRefPtr<Node> newChild,
Node* oldChild, Exce
         return;
 
     // Does this yet another check because
collectChildrenAndRemoveFromOldParent() fires a MutationEvent.
-    if (!checkReplaceChild(this, newChild.get(), oldChild, es))
+    if (!checkReplaceChild(this, newChild.get(), oldChild, "replaceChild", es))
         return;
 
     InspectorInstrumentation::willInsertDOMNode(&document(), this);
@@ -451,7 +453,7 @@ void ContainerNode::removeChild(Node* oldChild,
ExceptionState& es)
 
     // NotFoundError: Raised if oldChild is not a child of this node.
     if (!oldChild || oldChild->parentNode() != this) {
-        es.throwDOMException(NotFoundError);
+        es.throwDOMException(NotFoundError,
ExceptionMessages::failedToExecute("removeChild", "ContainerNode", "The node to
be removed is not a child of this node."));
         return;
     }
 
@@ -465,7 +467,7 @@ void ContainerNode::removeChild(Node* oldChild,
ExceptionState& es)
     // Events fired when blurring currently focused node might have moved this
     // child into a different parent.
     if (child->parentNode() != this) {
-        es.throwDOMException(NotFoundError);
+        es.throwDOMException(NotFoundError,
ExceptionMessages::failedToExecute("removeChild", "ContainerNode", "The node to
be removed is no longer a child of this node. Perhaps it was moved in a 'blur'
event handler?"));
         return;
     }
 
@@ -473,7 +475,7 @@ void ContainerNode::removeChild(Node* oldChild,
ExceptionState& es)
 
     // Mutation events might have moved this child into a different parent.
     if (child->parentNode() != this) {
-        es.throwDOMException(NotFoundError);
+        es.throwDOMException(NotFoundError,
ExceptionMessages::failedToExecute("removeChild", "ContainerNode", "The node to
be removed is no longer a child of this node. Perhaps it was moved in r…
(message too large)

[commit-bot: I haz the power, 2013-09-26 06:57:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/23889039/29001

[commit-bot: I haz the power, 2013-09-26 08:28:17]

Change committed as 158367

[arv (Not doing code reviews), 2013-09-26 15:23:09]

https://chromiumcodereview.appspot.com/23889039/diff/29001/Source/core/dom/Co...
File Source/core/dom/ContainerNode.cpp (right):

https://chromiumcodereview.appspot.com/23889039/diff/29001/Source/core/dom/Co...
Source/core/dom/ContainerNode.cpp:148: es.throwDOMException(NotFoundError,
ExceptionMessages::failedToExecute(method, "ContainerNode", "The new child
element is null."));
Let's not use ContainerNode in any user visible messages. It is an
implementation detail. It should just be Node.

[Mike West, 2013-09-27 06:58:29]

On 2013/09/26 15:23:09, arv wrote:
>
https://chromiumcodereview.appspot.com/23889039/diff/29001/Source/core/dom/Co...
> File Source/core/dom/ContainerNode.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/23889039/diff/29001/Source/core/dom/Co...
> Source/core/dom/ContainerNode.cpp:148: es.throwDOMException(NotFoundError,
> ExceptionMessages::failedToExecute(method, "ContainerNode", "The new child
> element is null."));
> Let's not use ContainerNode in any user visible messages. It is an
> implementation detail. It should just be Node.

Ooh, yes. You're right, that was a bad choice. I'll clean that up.