Bug 193049 – Move WKEditCommandObjC and WKEditorUndoTargetObjC into a separate file

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

The interfaces and implementations for these two Objective-C objects are currently in both PageClientIOS and WebViewImpl, with nearly identical code. We should de-duplicate this logic.

[Wenson Hsieh]

Created attachment 358121 [details]
Patch

[Sam Weinig]

Comment on attachment 358121 [details]
Patch

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

> Source/WebKit/ChangeLog:15
> +        Rename WKEditCommandObjC to WKEditCommand, and WKEditorUndoTarget to WKEditorUndoTargetObjC. The ObjC suffix in

Looks like you have this phrase backwards: "WKEditorUndoTarget to WKEditorUndoTargetObjC".

> Source/WebKit/UIProcess/Cocoa/WKEditCommand.h:36
> +    RefPtr<WebKit::WebEditCommandProxy> m_command;

This member should go in the implementation file.

> Source/WebKit/UIProcess/Cocoa/WKEditCommand.h:38
> +- (instancetype)initWithWebEditCommandProxy:(Ref<WebKit::WebEditCommandProxy>&&)command;

I think you probably also want something like:
- (instancetype)init NS_UNAVAILABLE;

> Source/WebKit/UIProcess/Cocoa/WKEditCommand.h:39
> +- (WebKit::WebEditCommandProxy*)command;

Can this ever be null? If not, return a reference?

> Source/WebKit/UIProcess/Cocoa/WKEditCommand.h:45
> +@interface WKEditorUndoTarget : NSObject
> +- (void)undoEditing:(id)sender;
> +- (void)redoEditing:(id)sender;
> +@end

My instinct is that this should have it's own file, but it is very closely tied to WKEditCommand. Perhaps a comment explaining why it's here would help.

[Wenson Hsieh]

Comment on attachment 358121 [details]
Patch

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

>> Source/WebKit/ChangeLog:15
>> +        Rename WKEditCommandObjC to WKEditCommand, and WKEditorUndoTarget to WKEditorUndoTargetObjC. The ObjC suffix in
> 
> Looks like you have this phrase backwards: "WKEditorUndoTarget to WKEditorUndoTargetObjC".

Oops, that's right. Fixed!

>> Source/WebKit/UIProcess/Cocoa/WKEditCommand.h:36
>> +    RefPtr<WebKit::WebEditCommandProxy> m_command;
> 
> This member should go in the implementation file.

Moved to the @implementation.

>> Source/WebKit/UIProcess/Cocoa/WKEditCommand.h:38
>> +- (instancetype)initWithWebEditCommandProxy:(Ref<WebKit::WebEditCommandProxy>&&)command;
> 
> I think you probably also want something like:
> - (instancetype)init NS_UNAVAILABLE;

Marked -init as unavailable.

>> Source/WebKit/UIProcess/Cocoa/WKEditCommand.h:39
>> +- (WebKit::WebEditCommandProxy*)command;
> 
> Can this ever be null? If not, return a reference?

Good catch, this should always be nonnull. Made this a WebEditCommandProxy&, and updated the call sites.

>> Source/WebKit/UIProcess/Cocoa/WKEditCommand.h:45
>> +@end
> 
> My instinct is that this should have it's own file, but it is very closely tied to WKEditCommand. Perhaps a comment explaining why it's here would help.

Sounds good, I'll clarify this with a comment.

[Wenson Hsieh]

Created attachment 358130 [details]
Patch for EWS

[Wenson Hsieh]

Comment on attachment 358121 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/WKEditCommand.h:36
>>> +    RefPtr<WebKit::WebEditCommandProxy> m_command;
>> 
>> This member should go in the implementation file.
> 
> Moved to the @implementation.

On second thought, this seems to break the 32-bit build :(

I'll move it back to the @interface { } for now, but at least added a @private so that nothing can access the ivar directly using ->.

[Wenson Hsieh]

Created attachment 358131 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 358131 [details]
Patch for EWS

Clearing flags on attachment: 358131

Committed r239558: <https://trac.webkit.org/changeset/239558>

[Radar WebKit Bug Importer]

<rdar://problem/46975343>