Comment on attachment 392872[details]
Part 1: Share AlternativeTextUIController
View in context: https://bugs.webkit.org/attachment.cgi?id=392872&action=review> Source/WebCore/ChangeLog:14
> + Note that I haven't enable USE_DICTATION_ALTERNATIVES on iOS, yet. So, this code isn't being compile on it.
Nit - "being compiled"
> Source/WebKit/UIProcess/ios/PageClientImplIOS.h:37
> +OBJC_CLASS NSTextAlternatives;
Nit - alphabetic order.
Created attachment 392890[details]
Part 5: Enable on iOS <- easy review patch
This patch will fail to build on EWS since it depends on the other 4 parts having been applied.
Comment on attachment 392876[details]
Part 2: Move insertDictatedTextAsync() from the Mac-specific files to Cocoa-specific files
View in context: https://bugs.webkit.org/attachment.cgi?id=392876&action=review> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:259
> + options.registerUndoGroup = registerUndoGroup;
(Not related to this patch)
At some point, we should consider changing InsertTextOptions’s bool members into enum classes.
(In reply to Brent Fulgham from comment #23)
>
> > Source/WebCore/ChangeLog:13
> > + Note that I haven't enable USE_DICTATION_ALTERNATIVES on iOS. So, this code isn't being
>
> enabled
Oops! I forgot to do this
Comment on attachment 392904[details]
Part 1: Share AlternativeTextUIController – To Land
View in context: https://bugs.webkit.org/attachment.cgi?id=392904&action=review> Source/WebCore/ChangeLog:15
> + Note that I haven't enable USE_DICTATION_ALTERNATIVES on iOS. So, this code isn't being
enabled
> Source/WebCore/editing/cocoa/AlternativeTextContextController.h:47
> + NSTextAlternatives *alternativesForContext(uint64_t context);
Can we 'uses' this uint64_t so we can tell it from all the other uint64_ts? or use ObjectIdentifier<>
> Source/WebCore/editing/cocoa/AlternativeTextContextController.mm:47
> + uint64_t context = reinterpret_cast<uint64_t>(alternatives.get());
Ugh, the pointer address Is the id?
Pointer addresses can get recycled.
> Source/WebCore/editing/cocoa/AlternativeTextUIController.hSource/WebCore/editing/mac/AlternativeTextUIController.h:45
> + WEBCORE_EXPORT uint64_t addAlternatives(const RetainPtr<NSTextAlternatives>&); // Returns a context ID.
If you used a named type you wouldn't need the comment.
Comment on attachment 392905[details]
Part 2: Move insertDictatedTextAsync() from the Mac-specific files to Cocoa-specific files – To Land
View in context: https://bugs.webkit.org/attachment.cgi?id=392905&action=review> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:195
> + auto& frame = m_page->focusController().focusedOrMainFrame();
> + Ref<Frame> protector { frame };
Why not just Ref<Frame> = m_page->focusController().focusedOrMainFrame();
or fix focusedOrMainFrame() to return a ref
2020-03-06 09:43 PST, Daniel Bates
2020-03-06 09:54 PST, Daniel Bates
2020-03-06 09:55 PST, Daniel Bates
2020-03-07 00:23 PST, Daniel Bates
2020-03-07 10:40 PST, Daniel Bates
2020-03-07 10:56 PST, Daniel Bates
2020-03-07 10:59 PST, Daniel Bates
2020-03-07 11:06 PST, Daniel Bates
2020-03-07 11:14 PST, Daniel Bates
2020-03-07 11:17 PST, Daniel Bates
2020-03-07 11:37 PST, Daniel Bates
2020-03-07 12:35 PST, Daniel Bates
2020-03-07 12:47 PST, Daniel Bates
2020-03-07 12:52 PST, Daniel Bates
2020-03-07 13:25 PST, Daniel Bates
2020-03-07 13:40 PST, Daniel Bates
2020-03-07 13:41 PST, Daniel Bates
2020-03-07 13:46 PST, Daniel Bates
2020-03-07 13:48 PST, Daniel Bates
2020-03-07 16:17 PST, Daniel Bates
2020-03-07 16:18 PST, Daniel Bates
2020-03-07 16:18 PST, Daniel Bates
2020-03-07 16:19 PST, Daniel Bates
2020-03-07 16:19 PST, Daniel Bates