[webkit-reviews] review granted: [Bug 193111] [Cocoa] Allow the page to add to the platform undo stack via UndoManager.addItem() : [Attachment 359593] Part 1 (fix typo in title)

[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 193111: [Cocoa] Allow the page to add to the platform undo stack via
UndoManager.addItem()
https://bugs.webkit.org/show_bug.cgi?id=193111

Attachment 359593: Part 1 (fix typo in title)

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




--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 359593
  --> https://bugs.webkit.org/attachment.cgi?id=359593
Part 1 (fix typo in title)

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

> Source/WebCore/bindings/js/JSUndoItemCustom.cpp:47
> +    if (UNLIKELY(reason))
> +	   *reason = "Document is an opaque root.";

You can always set the reason even if you were to return false.
So the idiomatic way of writing isReachableFromOpaqueRoots is to put this code
at the beginning of the function.

> Source/WebCore/page/UndoItem.cpp:55
> +    return isValid() ? &m_undoManager->document() : nullptr;

I'd prefer clearing m_undoManager in invalidate() instead.
That way, UndoItem would be destructed even after it had been removed from
UndoManager
e.g. via some kind of clear() operation in the future.

> Source/WebCore/page/UndoManager.cpp:59
> +    auto items = std::exchange(m_items, { });

I think you can just iterate over m_items and call clear at the end.
There is no reentrancy concern here.