|
Description
Daniel Bates
2020-03-06 09:42:10 PST
Created attachment 392732 [details]
Part 1: For the bots
Created attachment 392735 [details]
Part 1: For the bots
Created attachment 392737 [details]
Part 2: For the bots
Created attachment 392854 [details]
All in one - for the bots
Created attachment 392866 [details]
Part 1: Share AlternativeTextUIController
Created attachment 392868 [details]
Part 1: Share AlternativeTextUIController
Created attachment 392869 [details]
Part 1: Share AlternativeTextUIController
Expose some more UIKit SPI
Created attachment 392870 [details]
Part 2: Move insertDictatedTextAsync() from the Mac-specific files to Cocoa-specific files
Created attachment 392872 [details]
Part 1: Share AlternativeTextUIController
Created attachment 392873 [details]
Part 2: Move insertDictatedTextAsync() from the Mac-specific files to Cocoa-specific files
Created attachment 392874 [details]
Part 2: Move insertDictatedTextAsync() from the Mac-specific files to Cocoa-specific files
Created attachment 392876 [details]
Part 2: Move insertDictatedTextAsync() from the Mac-specific files to Cocoa-specific files
Created attachment 392878 [details]
Part 3: Share more AlternativeTextController code and WebPageProxy code
Created attachment 392879 [details]
Part 3: Share more AlternativeTextController code and WebPageProxy code
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 392883 [details]
Part 3: Share more AlternativeTextController code and WebPageProxy code
Created attachment 392886 [details]
Part 3: Share more AlternativeTextController code and WebPageProxy code
Created attachment 392887 [details]
Part 4: Implement -insertText:alternatives:style:
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.
Created attachment 392891 [details]
[Just for bots] All in one patch
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. Comment on attachment 392886 [details] Part 3: Share more AlternativeTextController code and WebPageProxy code View in context: https://bugs.webkit.org/attachment.cgi?id=392886&action=review r=me > Source/WebCore/ChangeLog:13 > + Note that I haven't enable USE_DICTATION_ALTERNATIVES on iOS. So, this code isn't being enabled Created attachment 392904 [details]
Part 1: Share AlternativeTextUIController – To Land
Created attachment 392905 [details]
Part 2: Move insertDictatedTextAsync() from the Mac-specific files to Cocoa-specific files – To Land
Created attachment 392906 [details]
Part 3: Share more AlternativeTextController code and WebPageProxy code – To Land
Created attachment 392907 [details]
Part 4: Implement -insertText:alternatives:style: – To Land
Created attachment 392908 [details]
Part 5: Enable on iOS – To Land
Committed part 1 in r258085: <https://trac.webkit.org/changeset/258085> Committed part 2 in r258086: <https://trac.webkit.org/changeset/258086> Committed part 3 in r258087: <https://trac.webkit.org/changeset/258087> Committed part 4 in r258088: <https://trac.webkit.org/changeset/258088> Committed part 5 in r258089: <https://trac.webkit.org/changeset/258089> (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 |