Bug 99438 – [WK2][WTR] Text input controller needs selectedRange implementation

• Status: UNCONFIRMED
• Component: Tools / Tests

[Mariusz Grzegorczyk]

Because of missing selectedRange() implementation following test doesn't pass:
editing/selection/mixed-editability-10.html

[Mariusz Grzegorczyk]

Tests which uses textInputController.selectedRange:
editing/selection/5825350-1.html
editing/selection/5825350-2.html
editing/selection/extend-by-sentence-002.html
editing/selection/mixed-editability-10.html
editing/selection/move-left-right.html
platform/mac/editing/input/text-input-controller.html

[Mariusz Grzegorczyk]

Created attachment 170588 [details]
patch

[Chris Dumez]

Comment on attachment 170588 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170588&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3334
> +    WebCore::ExceptionCode ec;

Shouldn't we handle error case?

[Jussi Kukkonen (jku)]

Comment on attachment 170588 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170588&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3314
> +bool WebPage::selectedRangeForTesting(int* start, int* length)

Is there a reason for not just moving WebPage::getSelectedRange() from WebPageMac.mm to here?

[Mariusz Grzegorczyk]

(In reply to comment #4)
> (From update of attachment 170588 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170588&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3314
> > +bool WebPage::selectedRangeForTesting(int* start, int* length)
> 
> Is there a reason for not just moving WebPage::getSelectedRange() from WebPageMac.mm to here?

I'm afraid of NSRange usage in Mac port. There failure is marked as location = NSNotFound

[Jussi Kukkonen (jku)]

(In reply to comment #5)
> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3314
> > > +bool WebPage::selectedRangeForTesting(int* start, int* length)
> > 
> > Is there a reason for not just moving WebPage::getSelectedRange() from WebPageMac.mm to here?
> 
> I'm afraid of NSRange usage in Mac port. There failure is marked as location = NSNotFound

There's only one user for this function:
  WKView selectedRange
so assuming you modified getSelectedRange to return a boolean like your new function and not set NSNotFound, wouldn't something like this work?

diff --git a/Source/WebKit2/UIProcess/API/mac/WKView.mm b/Source/WebKit2/UIProcess/API/mac/WKView.mm
index 0f221c6..d0fda59 100644
--- a/Source/WebKit2/UIProcess/API/mac/WKView.mm
+++ b/Source/WebKit2/UIProcess/API/mac/WKView.mm
@@ -1427 +1427,2 @@ static const short kIOHIDEventTypeScroll = 6;
-    _data->_page->getSelectedRange(selectionStart, selectionLength);
+    if (!_data->_page->getSelectedRange(selectionStart, selectionLength))
+        selectionStart = NSNotFound;

[Mariusz Grzegorczyk]

(In reply to comment #6)
> (In reply to comment #5)
> > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3314
> > > > +bool WebPage::selectedRangeForTesting(int* start, int* length)
> > > 
> > > Is there a reason for not just moving WebPage::getSelectedRange() from WebPageMac.mm to here?
> > 
> > I'm afraid of NSRange usage in Mac port. There failure is marked as location = NSNotFound
> 
> There's only one user for this function:
>   WKView selectedRange
> so assuming you modified getSelectedRange to return a boolean like your new function and not set NSNotFound, wouldn't something like this work?
> 
> diff --git a/Source/WebKit2/UIProcess/API/mac/WKView.mm b/Source/WebKit2/UIProcess/API/mac/WKView.mm
> index 0f221c6..d0fda59 100644
> --- a/Source/WebKit2/UIProcess/API/mac/WKView.mm
> +++ b/Source/WebKit2/UIProcess/API/mac/WKView.mm
> @@ -1427 +1427,2 @@ static const short kIOHIDEventTypeScroll = 6;
> -    _data->_page->getSelectedRange(selectionStart, selectionLength);
> +    if (!_data->_page->getSelectedRange(selectionStart, selectionLength))
> +        selectionStart = NSNotFound;

Ok, I'll try this. Will see what Mac developers say.

[Mariusz Grzegorczyk]

Created attachment 170910 [details]
use getSelectedRange from WebPage's Mac port

[Build Bot]

Comment on attachment 170910 [details]
use getSelectedRange from WebPage's Mac port

Attachment 170910 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14551020

[Jussi Kukkonen (jku)]

Comment on attachment 170910 [details]
use getSelectedRange from WebPage's Mac port

View in context: https://bugs.webkit.org/attachment.cgi?id=170910&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3346
> +    if (range && TextIterator::getLocationAndLengthFromRange(frame->selection()->rootEditableElementOrDocumentElement(), range.get(), locationSize, lengthSize)) {
> +        location = static_cast<uint64_t>(locationSize);
> +        length = static_cast<uint64_t>(lengthSize);
> +    }

Need to return false if the test here is not true, to reproduce the original mac version behaviour.

[Kenneth Rohde Christiansen]

Comment on attachment 170910 [details]
use getSelectedRange from WebPage's Mac port

View in context: https://bugs.webkit.org/attachment.cgi?id=170910&action=review

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:70
> +WK_EXPORT bool WKBundlePageSelectedRange(WKBundlePageRef pageRef, uint64_t& location, uint64_t& length);

I'm not sure other C-WK-API methods return a bool for getters. Maybe making location = -1 makes more sense?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3334
> +{

location = WTF::notFound ?

[Mariusz Grzegorczyk]

Created attachment 171187 [details]
Applied Kenneth's comments

[Jussi Kukkonen (jku)]

(In reply to comment #12)
> Created an attachment (id=171187) [details]
> Applied Kenneth's comments

Looks good to me at least.

[Mariusz Grzegorczyk]

(In reply to comment #13)
> (In reply to comment #12)
> > Created an attachment (id=171187) [details] [details]
> > Applied Kenneth's comments
> 
> Looks good to me at least.

I'm wondering if all these functions should be exported to API. If mac is testing such api in the future maybe all ports will do same. See https://bugs.webkit.org/show_bug.cgi?id=100219 comments.

[Mariusz Grzegorczyk]

Created attachment 176500 [details]
Rebase and extend function to UIProcess

[Mariusz Grzegorczyk]

Adding chromium and qt reviewers since this change affects all ports.

[Gyuyoung Kim]

Comment on attachment 176500 [details]
Rebase and extend function to UIProcess

View in context: https://bugs.webkit.org/attachment.cgi?id=176500&action=review

> Source/WebKit2/ChangeLog:8
> +        Implementation of getting selected range from webpage.

IMO, it would be good if you say this patch moves the implementation of mac port to common place for all ports.

[Tony Chang]

(In reply to comment #16)
> Adding chromium and qt reviewers since this change affects all ports.

This doesn't affect Chromium since Chromium doesn't use WebKit2 or WTR.

[Mariusz Grzegorczyk]

Created attachment 178994 [details]
Rebased, added comment about moving mac implementation to common part

[Ryosuke Niwa]

Comment on attachment 178994 [details]
Rebased, added comment about moving mac implementation to common part

View in context: https://bugs.webkit.org/attachment.cgi?id=178994&action=review

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1431
>      NSRange result = NSMakeRange(selectionStart, selectionLength);
> -    if (result.location == NSNotFound)
> -        LOG(TextInput, "selectedRange -> (NSNotFound, %u)", result.length);
> +    if (result.location == WTF::notFound)
> +        LOG(TextInput, "selectedRange -> (notFound, %u)", result.length);

Don't we need to pass in NSNotFound to NSMakeRange?

[Mariusz Grzegorczyk]

(In reply to comment #20)
> (From update of attachment 178994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178994&action=review
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:1431
> >      NSRange result = NSMakeRange(selectionStart, selectionLength);
> > -    if (result.location == NSNotFound)
> > -        LOG(TextInput, "selectedRange -> (NSNotFound, %u)", result.length);
> > +    if (result.location == WTF::notFound)
> > +        LOG(TextInput, "selectedRange -> (notFound, %u)", result.length);
> 
> Don't we need to pass in NSNotFound to NSMakeRange?

In my opinion: no. NSMakeRange just fills structure with passed arguments. Because of common implementation getSelectedRange returns WTF::notFound instead of NSNotFound.
I suppose you are asking about following change:

...getSelectedRange(selectionStart, selectionLength);
+if (selectionStart == WTF::notFound)
+    selectionStart = NSNotFound;
if (result.location == NSNotFound)
   LOG(TextInput, "selectedRange -> (NSNotFound, %u)", result.length);

[Gyuyoung Kim]

Comment on attachment 178994 [details]
Rebased, added comment about moving mac implementation to common part

Looks good to me in terms of sharing port implementation. But, I think mac port reviewer or WK2 reviewer needs to have a final review.

[Gyuyoung Kim]

(In reply to comment #22)
> (From update of attachment 178994 [details])
> Looks good to me in terms of sharing port implementation. But, I think mac port reviewer or WK2 reviewer needs to have a final review.

If there is no comment or objection by next week, I'd like to land this patch. And, happy new year !!

[Gyuyoung Kim]

Comment on attachment 178994 [details]
Rebased, added comment about moving mac implementation to common part

LGTM. But, someone might wanna have a final look.

[Sam Weinig]

Comment on attachment 178994 [details]
Rebased, added comment about moving mac implementation to common part

This codifies a sync message to the WebProcess, something we need to eliminate and which is a huge no-no, in API.  r-.  Please find another way to do

[Mariusz Grzegorczyk]

(In reply to comment #25)
> (From update of attachment 178994 [details])
> This codifies a sync message to the WebProcess, something we need to eliminate and which is a huge no-no, in API.  r-.  Please find another way to do

Code is only shared between all ports and was moved from existing mac's implementation.

[Gyuyoung Kim]

(In reply to comment #25)
> > (From update of attachment 178994 [details] [details])
> > This codifies a sync message to the WebProcess, something we need to eliminate and which is a huge no-no, in API.  r-.  Please find another way to do

Sam, thank you for your review. But, we don't understand what is your point well. I also thought this patch is just share mac implementation with other ports. Could you let us know what something we missed in more detail ?

[Sam Weinig]

The Mac port does a bad thing here, it does a synchronous call to the WebProcess.  This is something that should be fixed, not extended to all the other ports.  By moving it into shared code and adding API, it makes it much harder to fix.

[Mariusz Grzegorczyk]

(In reply to comment #28)
> The Mac port does a bad thing here, it does a synchronous call to the WebProcess.  This is something that should be fixed, not extended to all the other ports.  By moving it into shared code and adding API, it makes it much harder to fix.

Are you proposing to make it using callbacks? What about changing API in mac? Who is using selectedRange there?

[Mariusz Grzegorczyk]

Created attachment 191781 [details]
Rebased, make asynchronous

[Kenneth Rohde Christiansen]

Comment on attachment 191781 [details]
Rebased, make asynchronous

View in context: https://bugs.webkit.org/attachment.cgi?id=191781&action=review

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1014
> +void WebPageProxy::getSelectedRange(PassRefPtr<DataCallback> prpCallback)

what is a prp callback? pass ref ptr... no, please don't name like this!

[Kenneth Rohde Christiansen]

Benjamin, can you look at this?

[Mariusz Grzegorczyk]

Comment on attachment 191781 [details]
Rebased, make asynchronous

View in context: https://bugs.webkit.org/attachment.cgi?id=191781&action=review

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1014
>> +void WebPageProxy::getSelectedRange(PassRefPtr<DataCallback> prpCallback)
> 
> what is a prp callback? pass ref ptr... no, please don't name like this!

All callbacks are named in this file in this manner.

[Andreas Kling]

Comment on attachment 191781 [details]
Rebased, make asynchronous

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.