Bug 193111 – [Cocoa] Allow the page to add to the platform undo stack via UndoManager.addItem()

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/44807048>

[Wenson Hsieh]

Created attachment 358427 [details]
WIP (with tests)

[Wenson Hsieh]

Created attachment 359498 [details]
Part 1

[Wenson Hsieh]

Created attachment 359590 [details]
Patch

[Wenson Hsieh]

Created attachment 359591 [details]
Part 1

[Wenson Hsieh]

Created attachment 359592 [details]
Part 1 (w/ unified build fix)

[Wenson Hsieh]

Created attachment 359593 [details]
Part 1 (fix typo in title)

[Wenson Hsieh]

Created attachment 359594 [details]
Part 2 (CustomUndoStep)

[Wenson Hsieh]

Created attachment 359631 [details]
Part 3

[Wenson Hsieh]

Created attachment 359632 [details]
Part 3

[Ryosuke Niwa]

Comment on attachment 359593 [details]
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.

[Wenson Hsieh]

Comment on attachment 359593 [details]
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
>> +        *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.

Ah, I see! Fixed.

>> 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.

So in my patch, UndoItem::invalidate() already does null out m_undoManager by using std::exchange(m_undoManager, nullptr). But maybe this was too subtle :/

I'll change this to set m_undoManager to nullptr instead, to make it more obvious...

>> 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.

The reason I used std::exchange here is that UndoItem::invalidate calls back into UndoManager::removeItem to actually remove the UndoItem from the UndoManager, and so iterating over m_items while invalidating each item is going to cause m_items to mutate during iteration.

On second thought, this code is unnecessarily convoluted; I'll refactor this...

[Wenson Hsieh]

Created attachment 359819 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359819 [details]
Patch for landing

Clearing flags on attachment: 359819

Committed r240315: <https://trac.webkit.org/changeset/240315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 359832 [details]
Part 2 (rebase on trunk)

[Wenson Hsieh]

Created attachment 359834 [details]
Part 2 (fix GTK/WPE builds)

[Ryosuke Niwa]

Can we file a separate bug for part 2? It's confusing to post multiple patches per Bugzilla bug like this.

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #18)
> Can we file a separate bug for part 2? It's confusing to post multiple
> patches per Bugzilla bug like this.

Sure thing — filed <https://bugs.webkit.org/show_bug.cgi?id=193704>.