Bug 100219 – [WK2][WTR] Text input controller needs firstRectForCharacterRange implementation

• Status: UNCONFIRMED
• Component: Tools / Tests

[Mariusz Grzegorczyk]

Tests which use textInputController.firstRectForCharacterRange function:

editing/inserting/caret-position.html
editing/selection/5825350-1.html
editing/selection/5825350-2.html
editing/selection/mixed-editability-10.html
editing/selection/move-left-right.html
editing/style/text-indent.html
fast/dom/tab-in-right-alignment.html
fast/forms/cursor-at-editable-content-boundary.html
fast/text/international/thai-cursor-position.html
platform/mac/editing/input/caret-primary-bidi.html
platform/mac/editing/input/firstrectforcharacterrange-plain.html
platform/mac/editing/input/firstrectforcharacterrange-styled.html
platform/mac/editing/input/range-for-empty-document.html
platform/mac/editing/input/text-input-controller.html
platform/mac/editing/input/wrapped-line-char-rect.html
platform/mac/fast/text/justified-text-rect.html
svg/text/caret-in-svg-text.xhtml

[Jussi Kukkonen (jku)]

I'll take a look at this unless you've already got something in the works.

[Mariusz Grzegorczyk]

(In reply to comment #1)
> I'll take a look at this unless you've already got something in the works.

I have some patch for this, but waiting for depending one to upload. Also I'm analysing upper tests. Unfortunatelly I have no possibility to run mac ones.

[Jussi Kukkonen (jku)]

(In reply to comment #2)
> (In reply to comment #1)
> > I'll take a look at this unless you've already got something in the works.
> 
> I have some patch for this, but waiting for depending one to upload.

ok, cool. It shouldn't really depend on the other bug at all, right? They just happen to touch same files. Feel free to upload your patch even if 99438 isn't in yet, I can take a look.

[Mariusz Grzegorczyk]

Created attachment 171459 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 171459 [details]
patch

I do not think that this is the right approach. the goal here is to test API on UI process side. If you add a separate implementation just for testing, and then break the one in WebPageProxy, the regression test won't notice that.

[Mariusz Grzegorczyk]

(In reply to comment #5)
> (From update of attachment 171459 [details])
> I do not think that this is the right approach. the goal here is to test API on UI process side. If you add a separate implementation just for testing, and then break the one in WebPageProxy, the regression test won't notice that.

This is done for WebKitTestRunner. See discussion on https://bugs.webkit.org/show_bug.cgi?id=99438. All functions in TextInputController uses WKBundle... functionality which uses WebPage. For API on UI process (in mac) everything should be the same. There is a lot of functions like setTracksRepaints which are accessible from API and through bundle. Do you suggest to make all my changes as functionForTesting ?

[Kenneth Rohde Christiansen]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 171459 [details] [details])
> > I do not think that this is the right approach. the goal here is to test API on UI process side. If you add a separate implementation just for testing, and then break the one in WebPageProxy, the regression test won't notice that.
> 
> This is done for WebKitTestRunner. See discussion on https://bugs.webkit.org/show_bug.cgi?id=99438. All functions in TextInputController uses WKBundle... functionality which uses WebPage. For API on UI process (in mac) everything should be the same. There is a lot of functions like setTracksRepaints which are accessible from API and through bundle. Do you suggest to make all my changes as functionForTesting ?

Like ap said. Some tests are for testing functionality in the engine, others are for testing that public API works correctly.

In WebKit we are trying to split these test up so that those testing the engine use internals methods (shared across ports) where as others will use the testRunner methods (per port).

This is not finished so many in the former category are implemented using testRunner.

So basically you need to find out which group these tests belong in. If they are for testing API they should should call to public API methods and not implement them in the testRunner itself.

[Mikhail Pozdnyakov]

Maybe it's worth also modifying of WebPageProxy so that firstRectForCharacterRange is declared and defined there for all the WK2 ports, since you've added common implementation already on WebProcess side.

[Mariusz Grzegorczyk]

(In reply to comment #8)
> Maybe it's worth also modifying of WebPageProxy so that firstRectForCharacterRange is declared and defined there for all the WK2 ports, since you've added common implementation already on WebProcess side.

The problem is that TextInputController is in WebProcess, so cannot use UIProcess' api, but injected bundle one. So we have 2 ways to achieve WebPage class' implementation. 
1. application -> API -> WebPageProxy -> WebPage
2. WebKitTestRunner -> TextInputController -> WKBundle -> WebPage

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (In reply to comment #8)
> > Maybe it's worth also modifying of WebPageProxy so that firstRectForCharacterRange is declared and defined there for all the WK2 ports, since you've added common implementation already on WebProcess side.
> 
> The problem is that TextInputController is in WebProcess, so cannot use UIProcess' api, but injected bundle one. So we have 2 ways to achieve WebPage class' implementation. 
> 1. application -> API -> WebPageProxy -> WebPage
> 2. WebKitTestRunner -> TextInputController -> WKBundle -> WebPage

I'm not telling to use WebPageProxy from test runner but just make the patch more consistent by modifying UI process code as well.

[Mariusz Grzegorczyk]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Maybe it's worth also modifying of WebPageProxy so that firstRectForCharacterRange is declared and defined there for all the WK2 ports, since you've added common implementation already on WebProcess side.
> > 
> > The problem is that TextInputController is in WebProcess, so cannot use UIProcess' api, but injected bundle one. So we have 2 ways to achieve WebPage class' implementation. 
> > 1. application -> API -> WebPageProxy -> WebPage
> > 2. WebKitTestRunner -> TextInputController -> WKBundle -> WebPage
> 
> I'm not telling to use WebPageProxy from test runner but just make the patch more consistent by modifying UI process code as well.

So as I understand you want to make common implementation for WebPageProxy class. Is it for future plans? Still only one port which will use this, and export to API will be Mac.

[Mariusz Grzegorczyk]

Created attachment 192183 [details]
Rebased, make asynchronous

[Andreas Kling]

Comment on attachment 192183 [details]
Rebased, make asynchronous

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