[webkit-reviews] review denied: [Bug 88793] Implement undoscope attribute. : [Attachment 148329] patch3

[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 148329: patch3
https://bugs.webkit.org/attachment.cgi?id=148329&action=review

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


> Source/WebCore/dom/Element.idl:146
> +	   // Undo Manager

This comment is a pure noise. Please remove.

> Source/WebCore/dom/Element.cpp:2111
> +    return fastHasAttribute(HTMLNames::undoscopeAttr) && undoManager();

The call to fastHasAttribute(HTMLNames::undoscopeAttr) is redundant here
because undoManager() returns false when undoscope is not set.

> Source/WebCore/dom/Element.cpp:2118
> +    if (hasRareData())
> +	   return rareData()->m_undoManager;
> +    return 0;

Why don't we use ternary operator here?

> Source/WebCore/dom/Element.cpp:2121
> +void Element::createUndoManager()

createUndoManager is a misnomer here it doesn't always create undo manager.
Maybe createUndoManagerIfPossible?

I think it would be cleaner if the check for isContentEditable() &&
!isRootEditableElement() was in the caller instead.

> Source/WebCore/dom/Element.cpp:2143
> +	   if (!node->isElementNode() || !toElement(node)->undoManager())
> +	       continue;
> +	   toElement(node)->disconnectUndoManager();

Here, you're obtaining the node rare data twice by calling undoManager() before
calling disconnectUndoManager().
Since disconnectUndoManager bails out when there isn't undo manager, it's
probably better to always call disconnectUndoManager instead.

Also, it would have been cleaner if the call to disconnectUndoManager as in the
if statement as in:
if (node->isElementNode())
    toElement(node)->disconnectUndoManager();

> Source/WebCore/html/HTMLElement.cpp:226
> +	   if (equalIgnoringCase(attribute.value(), "false"))
> +	       return restoreDescendentsUndoManagers();
> +	   return disconnectDescendentsUndoManagers();

I don't think this works. contenteditable could also be set to "inherent".
Also, value() could be null when the attribute had been removed.
See
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#conten
teditable

r- because of this.

> LayoutTests/editing/undomanager/undoscope-attribute.html:22
> +var div1 = document.getElementById("divid1");
> +var div2 = document.getElementById("divid2");
> +var div3 = document.getElementById("divid3");

Instead of 1-3, use some descriptive name instead.