Bug 47659

Summary: TextInputController.hasSpellingMarkers() should be owned by LayoutTestController
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: Tools / TestsAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jamesr, mrobinson, ossy, rniwa, tonikitoo, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 47645, 47649    
Attachments:
Description Flags
Patch tkent: review+

Description Hajime Morrita 2010-10-14 01:26:59 PDT
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.
Comment 1 Ryosuke Niwa 2010-10-14 01:51:45 PDT
(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.
Comment 2 Hajime Morrita 2010-10-14 03:02:22 PDT
(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.
Comment 3 Tony Chang 2010-10-14 13:38:44 PDT
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.
Comment 4 Hajime Morrita 2010-10-14 18:35:39 PDT
> 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
Comment 5 Hajime Morrita 2010-10-14 19:08:35 PDT
Changed bug summary line.
Comment 6 Tony Chang 2010-10-14 19:30:10 PDT
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?
Comment 7 Hajime Morrita 2010-10-14 22:30:25 PDT
(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!
Comment 8 Tony Chang 2010-10-15 11:08:27 PDT
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.
Comment 9 Eric Seidel (no email) 2010-10-15 11:20:49 PDT
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.
Comment 10 Hajime Morrita 2010-10-18 00:46:44 PDT
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.
Comment 11 Hajime Morrita 2010-10-18 00:56:04 PDT
Making the master bug on Bug 47803 and narrowing the scope of this bug.
Comment 12 Hajime Morrita 2010-10-18 03:35:53 PDT
Created attachment 71017 [details]
Patch
Comment 13 Kent Tamura 2010-10-18 04:04:08 PDT
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.
Comment 14 Hajime Morrita 2010-10-18 04:46:34 PDT
Committed r69952: <http://trac.webkit.org/changeset/69952>
Comment 15 James Robinson 2010-11-01 13:36:02 PDT
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?
Comment 16 Hajime Morrita 2010-11-01 20:38:05 PDT
> 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.