[webkit-reviews] review denied: [Bug 88793] Implement undoscope attribute. : [Attachment 147552] Updated patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Rakesh <rakesh.kn at motorola.com>'s
request for review:
Bug 88793: Implement undoscope attribute.
https://bugs.webkit.org/show_bug.cgi?id=88793

Attachment 147552: Updated patch
https://bugs.webkit.org/attachment.cgi?id=147552&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=147552&action=review


> Source/WebCore/dom/Element.cpp:2111
> +void Element::createUndoManager()
> +{
> +    if (!ensureRareData()->m_undoManager)
> +	   rareData()->m_undoManager = UndoManager::create(this);
> +}

This function is only called in setUndoScope.
I'm failing to see why we need to have a separate function for this.

> Source/WebCore/dom/Element.cpp:2121
> +    if (!fastHasAttribute(undoscopeAttr)) {

Why does a function called setUndoScope checks if it has undoscope attribute or
not?
I would have merged this function into createUndoManager and checked the
existence of undoscope attribute in restoreDescendentsUndoManagers.

> Source/WebCore/dom/Element.cpp:2124
> +	       undoManager()->disconnect();
> +	       clearUndoManager();

Instead of forcing users to call disconnect before clearUndoManager.
I would have just made clearUndoManager() automatically call disconnect.
In fact, I would have added disconnectUndoManager() instead of
clearUndoManager.

> Source/WebCore/dom/Element.cpp:2137
> +    for (Node* node = firstChild(); node && node->isElementNode(); node =
node->traverseNextNode(this)) {
> +	   Element* element = toElement(node);

This is not right. You can certainly encounter a text node before seeing the
first element. e.g. <div>hello<div undoScope></div></div>
r- because of this bug.

> Source/WebCore/dom/Element.cpp:2148
> +    for (Node* node = firstChild(); node && node->isElementNode(); node =
node->traverseNextNode(this)) {
> +	   Element* element = toElement(node);

Ditto.

> Source/WebCore/editing/UndoManager.cpp:58
> +	   // FIXME Set INVALID_ACCESS_ERR exception.

Missing : after FIXME.

> LayoutTests/editing/undomanager/undoscope-attribute.html:5
> +<meta charset="utf-8">
> +<script src="../../fast/js/resources/js-test-pre.js"></script>

For undo manager tests, we're going to use testharness.js instead of
js-test-pre.js so that we may submit them to W3C later.

> LayoutTests/editing/undomanager/undoscope-attribute.html:37
> +shouldBe('div2.contentEditable = false;
Object.prototype.toString.call(document.getElementById("newlyAdded").undoManage
r)', "'[object UndoManager]'");

The name newlyAdded is inappropriate here.
Also, we need to test a case where we have multiple undo scope hosts, and a
case where we have a bunch of text nodes, and other random nodes around them.

> LayoutTests/editing/undomanager/undoscope-attribute-expected.txt:7
> +PASS div1.undoScope is true
> +PASS Object.prototype.toString.call(div1.undoManager) is '[object
UndoManager]'

This output isn't helpful because I can't see what div1 is. I would have added
a function like createElement and used evalAndLog like this:
evalAndLog("div1 = createElement('div', {'undoscope':null, 'contenteditable:
'true'});");
Also, why do we need to use Object.prototype.toString.call? Can't we just do
div1.undoManager.toString() ?