[webkit-reviews] review granted: [Bug 190009] Need a way for JavaScript (or bundle) code to participate in undo : [Attachment 359905] Fix WPE/GTK/Win

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 190009: Need a way for JavaScript (or bundle) code to participate in undo
https://bugs.webkit.org/show_bug.cgi?id=190009

Attachment 359905: Fix WPE/GTK/Win

https://bugs.webkit.org/attachment.cgi?id=359905&action=review




--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 359905
  --> https://bugs.webkit.org/attachment.cgi?id=359905
Fix WPE/GTK/Win

View in context: https://bugs.webkit.org/attachment.cgi?id=359905&action=review

> Source/WebCore/editing/CompositeEditCommand.h:92
> +    void invalidate() final { }

I don't like "invalidate". It doesn't tell us what it does, nor what we're
invalidating, or what invalidating edit command means.
I think we should say something more specific like didRemoveFromUndoManager.

> Source/WebCore/page/UndoManager.cpp:48
> +	   // FIXME: Support adding the same UndoItem to an UndoManager
multiple times.

We probably want to throw in this case.

> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:299
> +    readonly attribute DOMString undoActionName;
> +    readonly attribute DOMString redoActionName;

action name is Cocoa terminology. It's also somewhat surprising that there is
only one name.
We should probably call these something like lastUndoLabel and firstRedoLabel.