Bug 47659 – TextInputController.hasSpellingMarkers() should be owned by LayoutTestController

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Hajime Morrita]

Currently hasSpellingMarkers() is implemented as a method of textInputController.
But textInputController is only available mac and chromium, 
and hasSpellingMarkers() does not depend on these platforms. 
So it should be located on our universal LayoutTestController.

[Ryosuke Niwa]

(In reply to comment #0)
> Currently hasSpellingMarkers() is implemented as a method of textInputController.
> But textInputController is only available mac and chromium, 
> and hasSpellingMarkers() does not depend on these platforms. 
> So it should be located on our universal LayoutTestController.

I'm not sure moving hasSpellingMarkers to layoutTestController is what we want.  Can't we alternatively implement textInputController on other platforms?  Let me cc few gtk/qt folks here.

[Hajime Morrita]

(In reply to comment #1)
> > But textInputController is only available mac and chromium, 
> > and hasSpellingMarkers() does not depend on these platforms. 
> > So it should be located on our universal LayoutTestController.
> 
> I'm not sure moving hasSpellingMarkers to layoutTestController is what we want.  Can't we alternatively implement textInputController on other platforms?  Let me cc few gtk/qt folks here.

Hmm. I agree that It would be better to have TextInputController for each platform.
On the other hand, marker is not related to text input. It can worked with directly-inputted text.
Actually, implementation of hasSpellingMarker is in WebCore!
So it would be natural to have it on LTC, I think.

[Tony Chang]

Would it be possible to split TextInputController into a shared header and cpp file in WebKitTools/DumpRenderTree and have platform specific methods in the subdirectories (like we do for LayoutTestController.h,.cpp and LayoutTestController{Mac,Win,QT,etc})?  It seems like there are multiple methods in TextInputController that could be shared across platforms that use JSC.

[Hajime Morrita]

> Would it be possible to split TextInputController into a shared header and cpp file in WebKitTools/DumpRenderTree and have platform specific methods in the subdirectories (like we do for LayoutTestController.h,.cpp and LayoutTestController{Mac,Win,QT,etc})?  It seems like there are multiple methods in TextInputController that could be shared across platforms that use JSC.

It's possible. It's a kind of rewriting though.
It's more like as EventSender than LayoutTestController.
And each port has independent implementation for EventSender.
I don't wan't to push this kind of fragmentation.

Instead, I hope the portable part of the code to move into WebCore if possible, 
as I trying for LayourTestController. https://bugs.webkit.org/show_bug.cgi?id=42612

[Hajime Morrita]

Changed bug summary line.

[Tony Chang]

It's not clear to me when we decide to put something on LayoutTestController rather than creating a new object like EventSender or TextInputController.

Maybe Eric can fill me in on the history?  Is there a benefit other than namespacing the functions?

[Hajime Morrita]

(In reply to comment #6)
> It's not clear to me when we decide to put something on LayoutTestController rather than creating a new object like EventSender or TextInputController.
> 
> Maybe Eric can fill me in on the history?  Is there a benefit other than namespacing the functions?

It looks TextInputController maps underlying NSTextInput protocol
which is implemented by WebHTMLView.
http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/NSTextInputClient_Protocol/Reference/Reference.html
(NSTextInput protocol is deprecated by similar NSTextInputClient protocol.)
One exception is hasSpellingMarkers(), implemented by Google recently, just ignores such design intent.

These APIs are mainly used on tests under platform/mac/editing/* .
A few tests under LayoutTests/editing/* also uses  a part of TextController API, 
but such use isn't related to text input framework: getting caret rect, selected range, etc.
(We can even rewrite some of such test not to use textInputController.)

Following 4 APIs are used in LayoutTests/editing/* 
- textInputController.attributedSubstringFromRange
- textInputController.firstRectForCharacterRange
- textInputController.selectedRange
- textInputController.hasSpellingMarker
I thinks these API should be available ports other than mac and chromium
because they just export selection/caret state and not bound to specific IM framework.

Other APIs, used in LayoutTests/platform/mac/, are required for testing IM integration on Cocoa. 
So non-Mac ports don't need them.
(They should have their own testing API to exercise platform specific IM integration.
Chromium port is an exception because its rendering process isn't bound any specific IM framework and 
choose Cocoa-like abstraction for it. But that's  another story.)

So I'd like to:
- Move hasSpellingMarkers to LTC
- investigate Layouttest/editing/ tests to rewrite them to de-TextInputController-ize.
  - Add missing LTC method for it, if required.

Any suggestions are welcome!

[Tony Chang]

This makes sense to me.  Having TextInputController only used for mac specific tests seems ideal.  Let's try to move
- textInputController.attributedSubstringFromRange
- textInputController.firstRectForCharacterRange
- textInputController.selectedRange
- textInputController.hasSpellingMarker

to LTC since all ports need these methods for existing tests.  Starting with hasSpellingMarker sounds like a good idea.

[Eric Seidel (no email)]

LayoutTestController was the original JS-object exposed in DRT.  All the others came later.

I would like us to continue to move to a more cross-platform model.

I think we should consider long-term the idea of moving all of DRT's API to being plugins.  If we designed some sensible wrapper around NPAPI, we could easily expose most of DRT's helper logic via an NPAPI plugin, which could allow us to run layout tests in other configurations besides the DumpRenderTree executable (including chrome rendering processes, WebKit2 test shell, the Safari browser, or even in some future world Firefox!)

Basically the take-away, is that this kind of make-things-cross-platform is great!  Eventually we might even move away from using the JS API to do the object exposing and consider something JS-engine agnostic, like NPAPI or similar.

[Hajime Morrita]

Thanks much for your feedback!

> Basically the take-away, is that this kind of make-things-cross-platform is great!  Eventually we might even move away from using the JS API to do the object exposing and consider something JS-engine agnostic, like NPAPI or similar.
I have tried to move LayoutTestController to WebCore at Bug 42612,
This approach is portable between ports, JS engine agnostic, and possibly available with
non-DRT environment.
With this approach, we can provide to a plugin just to switch the WebCore::LTC on.
I would be happy if we could push forward Bug 42612.

Anyway, I'll move  TextController APIs to LTC soon.

[Hajime Morrita]

Making the master bug on Bug 47803 and narrowing the scope of this bug.

[Hajime Morrita]

Created attachment 71017 [details]
Patch

[Kent Tamura]

Comment on attachment 71017 [details]
Patch

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

> WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:801
> +bool LayoutTestController::hasSpellingMarker(int, int)

should add a FIXME comment.

> WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp:1394
> +bool LayoutTestController::hasSpellingMarker(int, int)

ditto.

[Hajime Morrita]

Committed r69952: <http://trac.webkit.org/changeset/69952>

[James Robinson]

I'm a bit confused about the test output in Chromium: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=editing%2Fspelling%2Fspelling-textarea.html&useWebKitCanary=true

On windows and linux the first line of output is "PASS 'zz' is not marked as misspelled."  I wonder if in http://trac.webkit.org/browser/trunk/LayoutTests/editing/spelling/spelling-textarea.html line 19 is intended to be testFailed instead of testPassed.  Is this result really a pass or a fail?

[Hajime Morrita]

> On windows and linux the first line of output is "PASS 'zz' is not marked as misspelled."  I wonder if in http://trac.webkit.org/browser/trunk/LayoutTests/editing/spelling/spelling-textarea.html line 19 is intended to be testFailed instead of testPassed.  Is this result really a pass or a fail?

It should FAIL for windows and linux
because current DRT for window and linux doesn't support spellcheck.
I agree that the test expectation is confusing and  filed Bug 48818 for it.
Thank you for letting me know.