Bug 153040 – WK2: Request completion candidates when needed

• Component: WebKit API
• Status: RESOLVED FIXED

[Beth Dakin]

WK2: Request completion candidates when needed

rdar://problem/24155631

[Beth Dakin]

Created attachment 268818 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Beth Dakin]

Created attachment 268909 [details]
Patch

[WebKit Commit Bot]

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.

[Enrica Casucci]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Beth Dakin]

Thanks Enrica! Good find. I fixed all three things.

http://trac.webkit.org/changeset/195002

[Alexey Proskuryakov]

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?

[Beth Dakin]

(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.

[Alexey Proskuryakov]

Yes, let's talk about it today!

This code crashes every time on bots, so I'm going to roll out for now anyway.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 153098

[Beth Dakin]

(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.

[Alexey Proskuryakov]

> it crashed only one test

That's not what I saw on bots, unfortunately.

[Beth Dakin]

(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.

[Beth Dakin]

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.

[Beth Dakin]

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.

[Alexey Proskuryakov]

Hmm, not sure why EWS refused to process this patch.

[Alexey Proskuryakov]

Ah, that's because it was landed before EWS had a chance to look at it.