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.
(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.
(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.
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.
> 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
Changed bug summary line.
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?
(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!
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.
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.
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.
Making the master bug on Bug 47803 and narrowing the scope of this bug.
Created attachment 71017 [details] Patch
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.
Committed r69952: <http://trac.webkit.org/changeset/69952>
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?
> 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.