Summary: | Move WKEditCommandObjC and WKEditorUndoTargetObjC into a separate file | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||
Component: | HTML Editing | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bdakin, commit-queue, megan_gardner, rniwa, sam, thorton, webkit-bug-importer, wenson_hsieh | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Wenson Hsieh
2018-12-28 12:12:08 PST
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> |