Bug 81300 – Convert hasSpellingMarker to use Internals interface

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 132192 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 132192 [details]
Patch

Attachment 132192 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11960199

[Build Bot]

Comment on attachment 132192 [details]
Patch

Attachment 132192 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11960202

[Build Bot]

Comment on attachment 132192 [details]
Patch

Attachment 132192 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11954989

[Hajime Morrita]

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

[Gyuyoung Kim]

(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.

[Gyuyoung Kim]

Created attachment 132264 [details]
Test patch for EWS - Plz do not review

[Gyuyoung Kim]

Created attachment 132291 [details]
Test patch for EWS - Plz do not review

Previous patch was something wrong. I submit same patch again.

[Alexey Proskuryakov]

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.

[Gyuyoung Kim]

(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 ?

[Alexey Proskuryakov]

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.

[Gyuyoung Kim]

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 ?

[Alexey Proskuryakov]

The disconnect here is that you seem to view Internals as something inherently better than LayoutTestController. Why? What problem are you solving?

[Gyuyoung Kim]

(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.

[Ryosuke Niwa]

(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.

[Alexey Proskuryakov]

For the record, I still think that this is a bad idea, but I don't enough interest to fight this case.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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.