<rdar://problem/44807048>
Created attachment 358427 [details] WIP (with tests)
Created attachment 359498 [details] Part 1
Created attachment 359590 [details] Patch
Created attachment 359591 [details] Part 1
Created attachment 359592 [details] Part 1 (w/ unified build fix)
Created attachment 359593 [details] Part 1 (fix typo in title)
Created attachment 359594 [details] Part 2 (CustomUndoStep)
Created attachment 359631 [details] Part 3
Created attachment 359632 [details] Part 3
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.
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...
Created attachment 359819 [details] Patch for landing
Comment on attachment 359819 [details] Patch for landing Clearing flags on attachment: 359819 Committed r240315: <https://trac.webkit.org/changeset/240315>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 359832 [details] Part 2 (rebase on trunk)
Created attachment 359834 [details] Part 2 (fix GTK/WPE builds)
Can we file a separate bug for part 2? It's confusing to post multiple patches per Bugzilla bug like this.
(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>.