Adjust hasSpellingMarker tests to use Internals instead of LayoutTestController interface. In my humble opinion, hasSpllingMarker() is able to be supported by Internals. Though I'm not sure whether document parameter can be added to hasSpellingMarker(), I add 'document' parameter to this patch in order to move it from LayoutTestController to Internals. If this can be accepted, it looks we can move more functions from LayoutTestController to Internals.
Created attachment 132192 [details] Patch
Comment on attachment 132192 [details] Patch Attachment 132192 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11960199
Comment on attachment 132192 [details] Patch Attachment 132192 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11960202
Comment on attachment 132192 [details] Patch Attachment 132192 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11954989
Comment on attachment 132192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132192&action=review Thanks for taking this. Please add export symbols to appropriate files. It will fix the build failure. > Source/WebCore/testing/Internals.cpp:708 > + if (!document || !document->frame() || !document->frame()->editor()) editor() is never null
(In reply to comment #5) > (From update of attachment 132192 [details]) > > Source/WebCore/testing/Internals.cpp:708 > > + if (!document || !document->frame() || !document->frame()->editor()) > > editor() is never null Thank you for your review. I will submit fixed patch.
Created attachment 132264 [details] Test patch for EWS - Plz do not review
Created attachment 132291 [details] Test patch for EWS - Plz do not review Previous patch was something wrong. I submit same patch again.
Wait, this looks like a wrong thing to do. Internals is for things that are not exposed as API - for things that have API, we should be doing end to end testing that includes WebKit layer.
(In reply to comment #9) > Wait, this looks like a wrong thing to do. Internals is for things that are not exposed as API - for things that have API, we should be doing end to end testing that includes WebKit layer. Hello Alexey, Sorry for late response. Could you let me know what is your concern correctly? As you know, some test functions of layout test controllers were moved to Internals. Do you think that this function can't move to Internals ?
LayoutTestController is built on top of WebKit. So, it tests not just WebCore, but also WebKit, which is much better. Testing through Internals is weaker, so it's only acceptable for things that WebKit isn't dealing with anyway.
However, in my humble opinion, there is no port specific implementation in hasSpllingMarker() of all ports. They implemented that test code for hasSpllingMarker() just calls editor()->selectionStartHasMarkerFor() function. So, I thought hasSpellingMarker() is able to be moved to Internals. Do you think there is differences of test implementation in each port ?
The disconnect here is that you seem to view Internals as something inherently better than LayoutTestController. Why? What problem are you solving?
(In reply to comment #13) > The disconnect here is that you seem to view Internals as something inherently better than LayoutTestController. Why? What problem are you solving? As Ryosuke suggested, to move from DRT APIs to Internals can reduce DRT's code size and lower the entry barrier for new ports. https://lists.webkit.org/pipermail/webkit-dev/2012-February/019491.html As far as I know, some functions of LayoutTestController were moved from LayoutTestController to Internals because they don't depend on port specific implementation. If you think this case is not accepted for that refactoring, I'm will to abandon this patch.
(In reply to comment #13) > The disconnect here is that you seem to view Internals as something inherently better than LayoutTestController. Why? What problem are you solving? In this particular case, the LTC method wans't using any WebKit layer code so just testing WebCore code should be fine. On the other hand, the patch reduces code duplication and increases the test coverage of the feature on the ports that had previously not implemented the method.
For the record, I still think that this is a bad idea, but I don't enough interest to fight this case.
Comment on attachment 132291 [details] Test patch for EWS - Plz do not review Clearing flags on attachment: 132291 Committed r111405: <http://trac.webkit.org/changeset/111405>
All reviewed patches have been landed. Closing bug.
I didn't quite realize that this was using Mac WebKit code that was added squarely for testing. It seems OK to move this to Internals. However, please note that code in "Private" headers is almost API, and it's not OK to remove it without someone at Apple checking whether that would break Apple applications. I checked now, and to the best of my knowledge, it's unused outside WebKit.