[webkit-reviews] review denied: [Bug 89722] Implement undoManager's undo, redo, length, position, clearUndo, and clearRedo : [Attachment 151551] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Sukolsak Sakshuwong
<sukolsak at google.com>'s request for review:
Bug 89722: Implement undoManager's undo, redo, length, position, clearUndo, and
clearRedo
https://bugs.webkit.org/show_bug.cgi?id=89722

Attachment 151551: Patch
https://bugs.webkit.org/attachment.cgi?id=151551&action=review

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


> Source/WebCore/ChangeLog:15
> +	   * WebCore.xcodeproj/project.pbxproj:

You also need to add your files to WebCore.vcproj.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:8998
> +		76808B4D159DADFA002B5233 /* JSHTMLDialogElement.cpp */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp;
path = JSHTMLDialogElement.cpp; sourceTree = "<group>"; };
> +		76808B4E159DADFA002B5233 /* JSHTMLDialogElement.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path =
JSHTMLDialogElement.h; sourceTree = "<group>"; };

It seems like this is an unrelated change?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25218
> +				7B517E8515ACF1F500F16B7A /* JSDOMTransaction.h
in Headers */,
> +				7B517E8715ACF21100F16B7A /* DOMTransaction.h in
Headers */,

Please sort these entries lexicologically.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28278
> +				7B0992F515ACF1AD00ED6D20 /*
JSUndoManagerCustom.cpp in Sources */,
> +				7B517E8415ACF1F500F16B7A /*
JSDOMTransaction.cpp in Sources */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28393
> +				SYMROOT = /Volumes/project/webkit/WebKitBuild;

You need to revert this change.

> Source/WebCore/editing/UndoManager.cpp:107
> +    for (size_t i = entry.size(); i > 0; --i)
> +	   entry[i - 1]->reapply();

This is wrong. You need to call reapply in the order they appear (e.g. if you
had t1 and t2 in entry and called
t2.unapply() and t1.unapply() in the respective order, then we should be
calling t1.reapply() and t2.reapply() in the respective order).
r- because of this bug. And please add a test case for this.

> Source/WebCore/editing/UndoManager.cpp:129
> +	   UndoManagerEntry* entry = new UndoManagerEntry;

You should declare OwnPtr here instead and adoptPtr immediately.

> Source/WebCore/editing/UndoManager.cpp:131
> +	   m_undoStack.append(adoptPtr(entry));

And call release() on that OwnPtr here.

> Source/WebCore/editing/UndoManager.cpp:144
> +	   UndoManagerEntry* entry = new UndoManagerEntry;
> +	   entry->append(step);
> +	   m_redoStack.append(adoptPtr(entry));

Ditto.

> Source/WebCore/editing/UndoManager.idl:42
> +	   void undo()
> +	       raises(DOMException);
> +	   void redo()
> +	       raises(DOMException);

You can put the raises on the same line.

> Source/WebCore/editing/UndoManager.idl:50
> +	   void clearUndo()
> +	       raises(DOMException);
> +	   void clearRedo()
> +	       raises(DOMException);

Ditto.