WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
UNCONFIRMED
100219
[WK2][WTR] Text input controller needs firstRectForCharacterRange implementation
https://bugs.webkit.org/show_bug.cgi?id=100219
Summary
[WK2][WTR] Text input controller needs firstRectForCharacterRange implementation
Mariusz Grzegorczyk
Reported
2012-10-24 03:08:05 PDT
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
Attachments
patch
(17.23 KB, patch)
2012-10-30 08:30 PDT
,
Mariusz Grzegorczyk
ap
: review-
Details
Formatted Diff
Diff
Rebased, make asynchronous
(23.33 KB, patch)
2013-03-08 02:20 PST
,
Mariusz Grzegorczyk
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Jussi Kukkonen (jku)
Comment 1
2012-10-25 05:53:44 PDT
I'll take a look at this unless you've already got something in the works.
Mariusz Grzegorczyk
Comment 2
2012-10-25 06:02:20 PDT
(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)
Comment 3
2012-10-25 06:29:01 PDT
(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
Comment 4
2012-10-30 08:30:30 PDT
Created
attachment 171459
[details]
patch
Alexey Proskuryakov
Comment 5
2012-10-30 10:27:17 PDT
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
Comment 6
2012-10-31 02:59:34 PDT
(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
Comment 7
2012-10-31 03:19:51 PDT
(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
Comment 8
2012-10-31 04:13:02 PDT
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
Comment 9
2012-10-31 04:25:48 PDT
(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
Comment 10
2012-10-31 07:25:12 PDT
(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
Comment 11
2012-10-31 08:41:16 PDT
(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
Comment 12
2013-03-08 02:20:19 PST
Created
attachment 192183
[details]
Rebased, make asynchronous
Andreas Kling
Comment 13
2014-02-05 11:20:04 PST
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug