Introduce UndoStep::label() and adopt it in WebKitLegacy and WebKit
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 Jan 2019 17:30:05 +0000 (17:30 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 Jan 2019 17:30:05 +0000 (17:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193706
<rdar://problem/44807048>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Refactors some existing logic when registering undoable actions, such that we propagate the undoable action's
label string instead of the EditAction to the client layer. This will help make handling of CustomUndoStep's
undo/redo label simpler, as the client layer won't need to worry about managing an EditAction and undo/redo
label simultaneously. There should be no change in behavior.

* editing/CompositeEditCommand.cpp:
(WebCore::EditCommandComposition::label const):
* editing/CompositeEditCommand.h:
* editing/CustomUndoStep.cpp:
(WebCore::CustomUndoStep::label const):
* editing/CustomUndoStep.h:
* editing/EditAction.cpp:
(WebCore::undoRedoLabel):
(WebCore::nameForUndoRedo): Deleted.
* editing/EditAction.h:

Rename nameForUndoRedo to undoRedoLabel, and remove the WEBCORE_EXPORT since it's no longer needed in WebKit or
WebKitLegacy.

* editing/UndoStep.h:

Add UndoStep::label(). While EditCommandComposition implements this by mapping its EditAction to a
localized string, CustomUndoStep implements this by returning the undoable action label provided by script.

Source/WebKit:

* UIProcess/Cocoa/WebViewImpl.mm:
(WebKit::WebViewImpl::registerEditCommand):
* UIProcess/WebEditCommandProxy.cpp:
(WebKit::WebEditCommandProxy::WebEditCommandProxy):
* UIProcess/WebEditCommandProxy.h:

Drive-by tweak: make WebEditCommandProxy's backpointer to its WebPageProxy a WeakPtr instead of a raw pointer.
Additionally, call clear() instead of setting m_page to 0 upon invalidation. Also, turn the WebPageProxy*
argument into a WebPageProxy&, since the WebPageProxy must exist when it creates a new WebEditCommandProxy.

(WebKit::WebEditCommandProxy::create):
(WebKit::WebEditCommandProxy::label const):
(WebKit::WebEditCommandProxy::invalidate):
(WebKit::WebEditCommandProxy::editAction const): Deleted.

Adjust UI-side logic to just handle the undo/redo label, rather than map the edit action to a localized string.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::registerEditCommandForUndo):
(WebKit::WebPageProxy::resetState):

Tweak this to use std::exchange instead of copying all the WebEditCommandProxy RefPtrs into a separate Vector
and then iterating over the Vector.

* UIProcess/WebPageProxy.h:
* UIProcess/WebPageProxy.messages.in:

Adjust this so that we only send the undo/redo label over IPC, rather than the edit action type.

* UIProcess/ios/PageClientImplIOS.mm:
(WebKit::PageClientImpl::registerEditCommand):
* WebProcess/WebCoreSupport/WebEditorClient.cpp:
(WebKit::WebEditorClient::registerUndoStep):

Source/WebKitLegacy/mac:

Use UndoStep::label().

* WebCoreSupport/WebEditorClient.h:
* WebCoreSupport/WebEditorClient.mm:
(-[WebUndoStep initWithUndoStep:]):
(+[WebUndoStep stepWithUndoStep:]):
(WebEditorClient::registerUndoOrRedoStep):
(WebEditorClient::registerUndoStep):
(WebEditorClient::registerRedoStep):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240342 268f45cc-cd09-0410-ab3c-d52691b4dbfc

19 files changed:
Source/WebCore/ChangeLog
Source/WebCore/editing/CompositeEditCommand.cpp
Source/WebCore/editing/CompositeEditCommand.h
Source/WebCore/editing/CustomUndoStep.cpp
Source/WebCore/editing/CustomUndoStep.h
Source/WebCore/editing/EditAction.cpp
Source/WebCore/editing/EditAction.h
Source/WebCore/editing/UndoStep.h
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm
Source/WebKit/UIProcess/WebEditCommandProxy.cpp
Source/WebKit/UIProcess/WebEditCommandProxy.h
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/UIProcess/WebPageProxy.messages.in
Source/WebKit/UIProcess/ios/PageClientImplIOS.mm
Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.cpp
Source/WebKitLegacy/mac/ChangeLog
Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm

index 5b93a77..6c5b00f 100644 (file)
@@ -1,3 +1,35 @@
+2019-01-23  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Introduce UndoStep::label() and adopt it in WebKitLegacy and WebKit
+        https://bugs.webkit.org/show_bug.cgi?id=193706
+        <rdar://problem/44807048>
+
+        Reviewed by Ryosuke Niwa.
+
+        Refactors some existing logic when registering undoable actions, such that we propagate the undoable action's
+        label string instead of the EditAction to the client layer. This will help make handling of CustomUndoStep's
+        undo/redo label simpler, as the client layer won't need to worry about managing an EditAction and undo/redo
+        label simultaneously. There should be no change in behavior.
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::EditCommandComposition::label const):
+        * editing/CompositeEditCommand.h:
+        * editing/CustomUndoStep.cpp:
+        (WebCore::CustomUndoStep::label const):
+        * editing/CustomUndoStep.h:
+        * editing/EditAction.cpp:
+        (WebCore::undoRedoLabel):
+        (WebCore::nameForUndoRedo): Deleted.
+        * editing/EditAction.h:
+
+        Rename nameForUndoRedo to undoRedoLabel, and remove the WEBCORE_EXPORT since it's no longer needed in WebKit or
+        WebKitLegacy.
+
+        * editing/UndoStep.h:
+
+        Add UndoStep::label(). While EditCommandComposition implements this by mapping its EditAction to a
+        localized string, CustomUndoStep implements this by returning the undoable action label provided by script.
+
 2019-01-23  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         [SOUP] Clean up NetworkStorageSession
index 242bd98..91d8af1 100644 (file)
@@ -302,6 +302,11 @@ void EditCommandComposition::getNodesInCommand(HashSet<Node*>& nodes)
 }
 #endif
 
+String EditCommandComposition::label() const
+{
+    return undoRedoLabel(m_editAction);
+}
+
 CompositeEditCommand::CompositeEditCommand(Document& document, EditAction editingAction)
     : EditCommand(document, editingAction)
 {
index 2da520f..ff1e61c 100644 (file)
@@ -88,6 +88,8 @@ public:
 private:
     EditCommandComposition(Document&, const VisibleSelection& startingSelection, const VisibleSelection& endingSelection, EditAction);
 
+    String label() const final;
+
     RefPtr<Document> m_document;
     VisibleSelection m_startingSelection;
     VisibleSelection m_endingSelection;
index 917f2aa..0680752 100644 (file)
@@ -65,4 +65,13 @@ bool CustomUndoStep::isValid() const
     return m_undoItem && m_undoItem->isValid();
 }
 
+String CustomUndoStep::label() const
+{
+    if (!isValid()) {
+        ASSERT_NOT_REACHED();
+        return emptyString();
+    }
+    return m_undoItem->label();
+}
+
 } // namespace WebCore
index fbce454..d9ead13 100644 (file)
@@ -47,6 +47,7 @@ private:
     void unapply() final;
     void reapply() final;
     EditAction editingAction() const final { return EditAction::Unspecified; }
+    String label() const final;
 
     bool isValid() const;
 
index 4125e35..31d216f 100644 (file)
@@ -30,7 +30,7 @@
 
 namespace WebCore {
 
-String nameForUndoRedo(EditAction editAction)
+String undoRedoLabel(EditAction editAction)
 {
     switch (editAction) {
     case EditAction::Unspecified:
index b6c45da..17e8be9 100644 (file)
@@ -95,6 +95,6 @@ enum class EditAction : uint8_t {
     InsertEditableImage
 };
 
-WEBCORE_EXPORT WTF::String nameForUndoRedo(EditAction);
+WTF::String undoRedoLabel(EditAction);
 
 } // namespace WebCore
index 00c1700..05009cb 100644 (file)
@@ -32,6 +32,7 @@
 
 #include "EditAction.h"
 #include <wtf/RefCounted.h>
+#include <wtf/text/WTFString.h>
 
 namespace WebCore {
 
@@ -42,6 +43,7 @@ public:
     virtual void unapply() = 0;
     virtual void reapply() = 0;
     virtual EditAction editingAction() const = 0;
+    virtual String label() const = 0;
 };
 
 } // namespace WebCore
index 34d58ca..ea380fb 100644 (file)
@@ -1,3 +1,45 @@
+2019-01-23  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Introduce UndoStep::label() and adopt it in WebKitLegacy and WebKit
+        https://bugs.webkit.org/show_bug.cgi?id=193706
+        <rdar://problem/44807048>
+
+        Reviewed by Ryosuke Niwa.
+
+        * UIProcess/Cocoa/WebViewImpl.mm:
+        (WebKit::WebViewImpl::registerEditCommand):
+        * UIProcess/WebEditCommandProxy.cpp:
+        (WebKit::WebEditCommandProxy::WebEditCommandProxy):
+        * UIProcess/WebEditCommandProxy.h:
+
+        Drive-by tweak: make WebEditCommandProxy's backpointer to its WebPageProxy a WeakPtr instead of a raw pointer.
+        Additionally, call clear() instead of setting m_page to 0 upon invalidation. Also, turn the WebPageProxy*
+        argument into a WebPageProxy&, since the WebPageProxy must exist when it creates a new WebEditCommandProxy.
+
+        (WebKit::WebEditCommandProxy::create):
+        (WebKit::WebEditCommandProxy::label const):
+        (WebKit::WebEditCommandProxy::invalidate):
+        (WebKit::WebEditCommandProxy::editAction const): Deleted.
+
+        Adjust UI-side logic to just handle the undo/redo label, rather than map the edit action to a localized string.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::registerEditCommandForUndo):
+        (WebKit::WebPageProxy::resetState):
+
+        Tweak this to use std::exchange instead of copying all the WebEditCommandProxy RefPtrs into a separate Vector
+        and then iterating over the Vector.
+
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebPageProxy.messages.in:
+
+        Adjust this so that we only send the undo/redo label over IPC, rather than the edit action type.
+
+        * UIProcess/ios/PageClientImplIOS.mm:
+        (WebKit::PageClientImpl::registerEditCommand):
+        * WebProcess/WebCoreSupport/WebEditorClient.cpp:
+        (WebKit::WebEditorClient::registerUndoStep):
+
 2019-01-23  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         [SOUP] Remove libsoup cruft from WebProcess
index 064e0df..d8fcc8c 100644 (file)
@@ -2692,7 +2692,7 @@ void WebViewImpl::executeEditCommandForSelector(SEL selector, const String& argu
 
 void WebViewImpl::registerEditCommand(Ref<WebEditCommandProxy>&& command, UndoOrRedo undoOrRedo)
 {
-    auto actionName = WebCore::nameForUndoRedo(command->editAction());
+    auto actionName = command->label();
     auto commandObjC = adoptNS([[WKEditCommand alloc] initWithWebEditCommandProxy:WTFMove(command)]);
 
     NSUndoManager *undoManager = [m_view undoManager];
index 9f83e3c..0e64bd7 100644 (file)
 namespace WebKit {
 using namespace WebCore;
 
-WebEditCommandProxy::WebEditCommandProxy(WebUndoStepID commandID, WebCore::EditAction editAction, WebPageProxy* page)
+WebEditCommandProxy::WebEditCommandProxy(WebUndoStepID commandID, const String& label, WebPageProxy& page)
     : m_commandID(commandID)
-    , m_editAction(editAction)
-    , m_page(page)
+    , m_label(label)
+    , m_page(makeWeakPtr(page))
 {
     m_page->addEditCommand(*this);
 }
index 716cb58..d1a14a9 100644 (file)
@@ -31,6 +31,8 @@
 #include <wtf/Forward.h>
 #include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
+#include <wtf/WeakPtr.h>
+#include <wtf/text/WTFString.h>
 
 namespace WebKit {
 
@@ -38,26 +40,26 @@ class WebPageProxy;
 
 class WebEditCommandProxy : public API::ObjectImpl<API::Object::Type::EditCommandProxy> {
 public:
-    static Ref<WebEditCommandProxy> create(WebUndoStepID commandID, WebCore::EditAction editAction, WebPageProxy* page)
+    static Ref<WebEditCommandProxy> create(WebUndoStepID commandID, const String& label, WebPageProxy& page)
     {
-        return adoptRef(*new WebEditCommandProxy(commandID, editAction, page));
+        return adoptRef(*new WebEditCommandProxy(commandID, label, page));
     }
     ~WebEditCommandProxy();
 
     WebUndoStepID commandID() const { return m_commandID; }
-    WebCore::EditAction editAction() const { return m_editAction; }
+    String label() const { return m_label; }
 
-    void invalidate() { m_page = 0; }
+    void invalidate() { m_page.clear(); }
 
     void unapply();
     void reapply();
 
 private:
-    WebEditCommandProxy(WebUndoStepID commandID, WebCore::EditAction, WebPageProxy*);
+    WebEditCommandProxy(WebUndoStepID commandID, const String& label, WebPageProxy&);
 
     WebUndoStepID m_commandID;
-    WebCore::EditAction m_editAction;
-    WebPageProxy* m_page;
+    String m_label;
+    WeakPtr<WebPageProxy> m_page;
 };
 
 } // namespace WebKit
index e74f38d..e4449f8 100644 (file)
@@ -5439,9 +5439,9 @@ void WebPageProxy::compositionWasCanceled()
 
 // Undo management
 
-void WebPageProxy::registerEditCommandForUndo(WebUndoStepID commandID, uint32_t editAction)
+void WebPageProxy::registerEditCommandForUndo(WebUndoStepID commandID, const String& label)
 {
-    registerEditCommand(WebEditCommandProxy::create(commandID, static_cast<EditAction>(editAction), this), UndoOrRedo::Undo);
+    registerEditCommand(WebEditCommandProxy::create(commandID, label, *this), UndoOrRedo::Undo);
 }
     
 void WebPageProxy::registerInsertionUndoGrouping()
@@ -6652,10 +6652,7 @@ void WebPageProxy::resetState(ResetStateReason resetStateReason)
     m_callbacks.invalidate(error);
     m_loadDependentStringCallbackIDs.clear();
 
-    auto editCommandVector = copyToVector(m_editCommandSet);
-    m_editCommandSet.clear();
-
-    for (auto& editCommand : editCommandVector)
+    for (auto& editCommand : std::exchange(m_editCommandSet, { }))
         editCommand->invalidate();
 
     m_activePopupMenu = nullptr;
index ac5600f..ad7d7dc 100644 (file)
@@ -1644,7 +1644,7 @@ private:
     void backForwardClear();
 
     // Undo management
-    void registerEditCommandForUndo(WebUndoStepID commandID, uint32_t editAction);
+    void registerEditCommandForUndo(WebUndoStepID commandID, const String& label);
     void registerInsertionUndoGrouping();
     void clearAllEditCommands();
     void canUndoRedo(UndoOrRedo, bool& result);
index 389a32b..6c35cb4 100644 (file)
@@ -233,7 +233,7 @@ messages -> WebPageProxy {
     WillGoToBackForwardListItem(struct WebCore::BackForwardItemIdentifier itemID, bool inPageCache)
 
     # Undo/Redo messages
-    RegisterEditCommandForUndo(uint64_t commandID, uint32_t editAction)
+    RegisterEditCommandForUndo(uint64_t commandID, String label)
     ClearAllEditCommands()
     RegisterInsertionUndoGrouping()
     CanUndoRedo(enum:bool WebKit::UndoOrRedo undoOrRedo) -> (bool result) LegacySync
index 71ea6b8..f2304ac 100644 (file)
@@ -267,7 +267,7 @@ void PageClientImpl::didChangeViewportProperties(const ViewportAttributes&)
 
 void PageClientImpl::registerEditCommand(Ref<WebEditCommandProxy>&& command, UndoOrRedo undoOrRedo)
 {
-    auto actionName = WebCore::nameForUndoRedo(command->editAction());
+    auto actionName = command->label();
     auto commandObjC = adoptNS([[WKEditCommand alloc] initWithWebEditCommandProxy:WTFMove(command)]);
     
     NSUndoManager *undoManager = [m_contentView undoManager];
index 0094e35..8f2181b 100644 (file)
@@ -311,10 +311,9 @@ void WebEditorClient::registerUndoStep(UndoStep& step)
 
     auto webStep = WebUndoStep::create(step);
     auto stepID = webStep->stepID();
-    auto editAction = static_cast<uint32_t>(webStep->step().editingAction());
 
     m_page->addWebUndoStep(stepID, WTFMove(webStep));
-    m_page->send(Messages::WebPageProxy::RegisterEditCommandForUndo(stepID, editAction), m_page->pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
+    m_page->send(Messages::WebPageProxy::RegisterEditCommandForUndo(stepID, step.label()), m_page->pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
 }
 
 void WebEditorClient::registerRedoStep(UndoStep&)
index c07f40f..00717e0 100644 (file)
@@ -1,3 +1,21 @@
+2019-01-23  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Introduce UndoStep::label() and adopt it in WebKitLegacy and WebKit
+        https://bugs.webkit.org/show_bug.cgi?id=193706
+        <rdar://problem/44807048>
+
+        Reviewed by Ryosuke Niwa.
+
+        Use UndoStep::label().
+
+        * WebCoreSupport/WebEditorClient.h:
+        * WebCoreSupport/WebEditorClient.mm:
+        (-[WebUndoStep initWithUndoStep:]):
+        (+[WebUndoStep stepWithUndoStep:]):
+        (WebEditorClient::registerUndoOrRedoStep):
+        (WebEditorClient::registerUndoStep):
+        (WebEditorClient::registerRedoStep):
+
 2019-01-22  Alex Christensen  <achristensen@webkit.org>
 
         Move NetworkStorageSession ownership to NetworkProcess
index ccc33f9..c35233e 100644 (file)
@@ -125,7 +125,7 @@ static WebViewInsertAction kit(EditorInsertAction action)
     RefPtr<UndoStep> m_step;   
 }
 
-+ (WebUndoStep *)stepWithUndoStep:(UndoStep&)step;
++ (WebUndoStep *)stepWithUndoStep:(Ref<UndoStep>&&)step;
 - (UndoStep&)step;
 
 @end
@@ -141,12 +141,12 @@ static WebViewInsertAction kit(EditorInsertAction action)
 #endif
 }
 
-- (id)initWithUndoStep:(UndoStep&)step
+- (id)initWithUndoStep:(Ref<UndoStep>&&)step
 {
     self = [super init];
     if (!self)
         return nil;
-    m_step = &step;
+    m_step = WTFMove(step);
     return self;
 }
 
@@ -158,9 +158,9 @@ static WebViewInsertAction kit(EditorInsertAction action)
     [super dealloc];
 }
 
-+ (WebUndoStep *)stepWithUndoStep:(UndoStep&)step
++ (WebUndoStep *)stepWithUndoStep:(Ref<UndoStep>&&)step
 {
-    return [[[WebUndoStep alloc] initWithUndoStep:step] autorelease];
+    return [[[WebUndoStep alloc] initWithUndoStep:WTFMove(step)] autorelease];
 }
 
 - (UndoStep&)step
@@ -601,9 +601,8 @@ void WebEditorClient::registerUndoOrRedoStep(UndoStep& step, bool isRedo)
         return;
 #endif
 
-    NSString *actionName = WebCore::nameForUndoRedo(step.editingAction());
-    WebUndoStep *webEntry = [WebUndoStep stepWithUndoStep:step];
-    [undoManager registerUndoWithTarget:m_undoTarget.get() selector:(isRedo ? @selector(redoEditing:) : @selector(undoEditing:)) object:webEntry];
+    NSString *actionName = step.label();
+    [undoManager registerUndoWithTarget:m_undoTarget.get() selector:(isRedo ? @selector(redoEditing:) : @selector(undoEditing:)) object:[WebUndoStep stepWithUndoStep:step]];
     if (actionName)
         [undoManager setActionName:actionName];
     m_haveUndoRedoOperations = YES;
@@ -629,14 +628,14 @@ void WebEditorClient::updateEditorStateAfterLayoutIfEditabilityChanged()
         [m_webView updateTouchBar];
 }
 
-void WebEditorClient::registerUndoStep(UndoStep& cmd)
+void WebEditorClient::registerUndoStep(UndoStep& command)
 {
-    registerUndoOrRedoStep(cmd, false);
+    registerUndoOrRedoStep(command, false);
 }
 
-void WebEditorClient::registerRedoStep(UndoStep& cmd)
+void WebEditorClient::registerRedoStep(UndoStep& command)
 {
-    registerUndoOrRedoStep(cmd, true);
+    registerUndoOrRedoStep(command, true);
 }
 
 void WebEditorClient::clearUndoRedoOperations()