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.
Created attachment 358121 [details] Patch
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.
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.
Created attachment 358130 [details] Patch for EWS
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 ->.
Created attachment 358131 [details] Patch for EWS
Comment on attachment 358131 [details] Patch for EWS Clearing flags on attachment: 358131 Committed r239558: <https://trac.webkit.org/changeset/239558>
<rdar://problem/46975343>