Bug 193704 – Introduce CustomUndoStep.h and CustomUndoStep.cpp

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

Work towards <rdar://problem/44807048>

[Wenson Hsieh]

Created attachment 359840 [details]
Patch

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Wenson Hsieh]

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

[Wenson Hsieh]

Created attachment 359848 [details]
Patch for landing

[Wenson Hsieh]

Created attachment 359849 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359849 [details]
Patch for landing

Clearing flags on attachment: 359849

Committed r240328: <https://trac.webkit.org/changeset/240328>

[Wenson Hsieh]

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