Bug 193706

Summary: Introduce UndoStep::label() and adopt it in WebKitLegacy and WebKit
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bdakin, commit-queue, megan_gardner, rniwa, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
rniwa: review+
Review feedback & fix GTK build none

Description Wenson Hsieh 2019-01-22 20:16:32 PST 
More work towards <rdar://problem/44807048>.
Comment 1 Ryosuke Niwa 2019-01-22 22:30:42 PST 
I think we can just call it label() since the class is called UndoStep.
Comment 2 Wenson Hsieh 2019-01-22 22:34:42 PST 
Created attachment 359856 [details]
Patch
Comment 3 Wenson Hsieh 2019-01-22 22:35:28 PST 
(In reply to Ryosuke Niwa from comment #1)
> I think we can just call it label() since the class is called UndoStep.

Oops, I just saw this now :/
Comment 4 Wenson Hsieh 2019-01-22 22:49:56 PST 
Created attachment 359857 [details]
Patch
Comment 5 Ryosuke Niwa 2019-01-23 00:31:43 PST 
Comment on attachment 359857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359857&action=review

> Source/WebKit/ChangeLog:45
> +        Adjust this to take Ref<UndoStep>&&.

Why are we making this change?

> Source/WebKit/UIProcess/WebEditCommandProxy.h:42
> -    static Ref<WebEditCommandProxy> create(WebUndoStepID commandID, WebCore::EditAction editAction, WebPageProxy* page)
> +    static Ref<WebEditCommandProxy> create(WebUndoStepID commandID, const String& label, WebPageProxy& page)

Hm... I would have kept WebCore::EditAction for this one but okay.
Comment 6 Wenson Hsieh 2019-01-23 07:21:35 PST 
Comment on attachment 359857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359857&action=review

>> Source/WebKit/ChangeLog:45
>> +        Adjust this to take Ref<UndoStep>&&.
> 
> Why are we making this change?

I thought it would be preferable to UndoStep&, but I can revert this part.

>> Source/WebKit/UIProcess/WebEditCommandProxy.h:42
>> +    static Ref<WebEditCommandProxy> create(WebUndoStepID commandID, const String& label, WebPageProxy& page)
> 
> Hm... I would have kept WebCore::EditAction for this one but okay.

I think it should be okay to remove editAction from WebEditCommandProxy, since the only thing we used it for was the undo/redo label.
Comment 7 Wenson Hsieh 2019-01-23 07:48:35 PST 
Created attachment 359882 [details]
Review feedback & fix GTK build
Comment 8 WebKit Commit Bot 2019-01-23 09:30:11 PST 
Comment on attachment 359882 [details]
Review feedback & fix GTK build

Clearing flags on attachment: 359882

Committed r240342: <https://trac.webkit.org/changeset/240342>
Comment 9 Simon Fraser (smfr) 2019-01-23 20:44:07 PST 
Is the undo label controllable by the web content?
Comment 10 Wenson Hsieh 2019-01-23 20:48:39 PST 
(In reply to Simon Fraser (smfr) from comment #9)
> Is the undo label controllable by the web content?

Yes. That being said, the UndoManager API isn't web-exposed (at least, not currently).
Comment 11 Ryosuke Niwa 2019-01-24 19:03:18 PST 
(In reply to Wenson Hsieh from comment #10)
> (In reply to Simon Fraser (smfr) from comment #9)
> > Is the undo label controllable by the web content?
> 
> Yes. That being said, the UndoManager API isn't web-exposed (at least, not
> currently).

We probably need to do some sanitization if we decide to expose this API to the Web.