WK2: Request completion candidates when needed rdar://problem/24155631
Created attachment 268818 [details] Patch
Attachment 268818 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:2109: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:2129: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 268818 [details] Patch Attachment 268818 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/684120 New failing tests: fast/dom/focus-contenteditable.html
Created attachment 268826 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 268909 [details] Patch
Attachment 268909 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:2109: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:2129: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 268909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268909&action=review Looks good to me > Source/WebCore/editing/Editor.cpp:3553 > + RefPtr<Range> rangeForCurrentlyTypedString = candidateRangeForSelection(selection, m_frame); I think you can have only one parameter (m_frame) to this function, since you compute the VisibleSelectionFrom it > Source/WebCore/editing/Editor.cpp:3563 > + RefPtr<Range> candidateRange = candidateRangeForSelection(selection, m_frame); same here. > Source/WebKit2/ChangeLog:10 > + Mac need to support postLayoutData in order to have some layout-related typo needs
Comment on attachment 268909 [details] Patch Attachment 268909 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/688106 New failing tests: fast/dom/focus-contenteditable.html
Created attachment 268916 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268909 [details] Patch Attachment 268909 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/688104 New failing tests: fast/dom/focus-contenteditable.html
Created attachment 268921 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Thanks Enrica! Good find. I fixed all three things. http://trac.webkit.org/changeset/195002
Comment on attachment 268909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268909&action=review > Source/WebKit2/Shared/EditorState.cpp:131 > +#if PLATFORM(MAC) > + encoder << candidateRequestStartPosition; > + encoder << paragraphContextForCandidateRequest; > + encoder << stringForCandidateRequest; > +#endif EditorState is a very unfortunate hack which cannot possibly result in reliable behavior. Does the API require synchronous return? Can we request making it asynchronous?
(In reply to comment #13) > Comment on attachment 268909 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268909&action=review > > > Source/WebKit2/Shared/EditorState.cpp:131 > > +#if PLATFORM(MAC) > > + encoder << candidateRequestStartPosition; > > + encoder << paragraphContextForCandidateRequest; > > + encoder << stringForCandidateRequest; > > +#endif > > EditorState is a very unfortunate hack which cannot possibly result in > reliable behavior. > > Does the API require synchronous return? Can we request making it > asynchronous? I was not aware that EditorState is considered a hack! I'm open to other ideas for the best way to implement this. The API does not require synchronous return, but we do need to request candidates on every selection change, and the analogous iOS API is implemented via EditorState, so it seems like a good place for it.
Yes, let's talk about it today! This code crashes every time on bots, so I'm going to roll out for now anyway.
Re-opened since this is blocked by bug 153098
(In reply to comment #15) > Yes, let's talk about it today! > > This code crashes every time on bots, so I'm going to roll out for now > anyway. It's fine that this was rolled out, but to be clear, it crashed only one test: imported/blink/editing/text-iterator/read-past-cloned-first-letter.html and I was already investigating. My investigation indicates that this is a long-standing bug that we happen to trigger now. Of course I do need to fix it now that new code is triggering it, but I just want to clarify that this is not happening because EditorState is "hacky" or because of any other problem with the patch.
> it crashed only one test That's not what I saw on bots, unfortunately.
(In reply to comment #17) > (In reply to comment #15) > imported/blink/editing/text-iterator/read-past-cloned-first-letter.html > I filed https://bugs.webkit.org/show_bug.cgi?id=153104 and uploaded a patch to fix this bug.
Created attachment 269002 [details] Patch https://bugs.webkit.org/show_bug.cgi?id=153104 tracks fixing the blink test that crashes because of this patch (the patch has been reviewed and committed), and the crashing internal tests are fixed in this version of the patch.
Thanks Tim! http://trac.webkit.org/changeset/195078 Alexey and I talked, and I have some follow-up tasks to look into having the API changed a bit, which would let us make something more robust.
Hmm, not sure why EWS refused to process this patch.
Ah, that's because it was landed before EWS had a chance to look at it.