Work towards <rdar://problem/44807048>
Created attachment 359840 [details] Patch
Comment on attachment 359840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359840&action=review > Source/WebCore/editing/CompositeEditCommand.h:91 > + const String& customUndoLabel() const final { return emptyString(); } I think a better approach is have the conversion from EditAction to label here.
(In reply to Ryosuke Niwa from comment #2) > Comment on attachment 359840 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359840&action=review > > > Source/WebCore/editing/CompositeEditCommand.h:91 > > + const String& customUndoLabel() const final { return emptyString(); } > > I think a better approach is have the conversion from EditAction to label > here. Not saying that we should make such a change in this patch but I think having editAction() & customUndoLabel() is rather confusing so we probably shouldn't add such a method. Also, this label is used for redo as well so saying it's undo label is quite misleading.
(In reply to Ryosuke Niwa from comment #3) > (In reply to Ryosuke Niwa from comment #2) > > Comment on attachment 359840 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=359840&action=review > > > > > Source/WebCore/editing/CompositeEditCommand.h:91 > > > + const String& customUndoLabel() const final { return emptyString(); } > > > > I think a better approach is have the conversion from EditAction to label > > here. > > Not saying that we should make such a change in this patch but I think > having editAction() & customUndoLabel() is rather confusing so we probably > shouldn't add such a method. Also, this label is used for redo as well so > saying it's undo label is quite misleading. That's a good point! This will take a bit of refactoring, but I think it's worth it (this should also allow us to remove some duplicate code between WebKitLegacy and WebKit). I'll land this patch (Part 2) without customUndoLabel() for now, and write up a patch to add an undoRedoLabel() method to UndoStep instead. This refactoring patch will sit between this one (Part 2) and the Part 3 I had planned.
Created attachment 359848 [details] Patch for landing
Created attachment 359849 [details] Patch for landing
Comment on attachment 359849 [details] Patch for landing Clearing flags on attachment: 359849 Committed r240328: <https://trac.webkit.org/changeset/240328>
(In reply to Wenson Hsieh from comment #4) > (In reply to Ryosuke Niwa from comment #3) > > (In reply to Ryosuke Niwa from comment #2) > > > Comment on attachment 359840 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=359840&action=review > > > > > > > Source/WebCore/editing/CompositeEditCommand.h:91 > > > > + const String& customUndoLabel() const final { return emptyString(); } > > > > > > I think a better approach is have the conversion from EditAction to label > > > here. > > > > Not saying that we should make such a change in this patch but I think > > having editAction() & customUndoLabel() is rather confusing so we probably > > shouldn't add such a method. Also, this label is used for redo as well so > > saying it's undo label is quite misleading. > > That's a good point! This will take a bit of refactoring, but I think it's > worth it (this should also allow us to remove some duplicate code between > WebKitLegacy and WebKit). I'll land this patch (Part 2) without > customUndoLabel() for now, and write up a patch to add an undoRedoLabel() > method to UndoStep instead. > > This refactoring patch will sit between this one (Part 2) and the Part 3 I > had planned. I filed https://bugs.webkit.org/show_bug.cgi?id=193706 to track this refactoring.