Bug 81300 - Convert hasSpellingMarker to use Internals interface
Summary: Convert hasSpellingMarker to use Internals interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-15 19:40 PDT by Gyuyoung Kim
Modified: 2012-03-20 10:23 PDT (History)
8 users (show)

See Also:


Attachments
Patch (61.23 KB, patch)
2012-03-15 21:28 PDT, Gyuyoung Kim
morrita: review-
gns: commit-queue-
Details | Formatted Diff | Diff
Test patch for EWS - Plz do not review (59.80 KB, patch)
2012-03-16 05:58 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Test patch for EWS - Plz do not review (59.92 KB, patch)
2012-03-16 08:49 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2012-03-15 19:40:27 PDT
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.
Comment 1 Gyuyoung Kim 2012-03-15 21:28:04 PDT
Created attachment 132192 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2012-03-15 21:39:07 PDT
Comment on attachment 132192 [details]
Patch

Attachment 132192 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11960199
Comment 3 Build Bot 2012-03-15 21:49:19 PDT
Comment on attachment 132192 [details]
Patch

Attachment 132192 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11960202
Comment 4 Build Bot 2012-03-15 21:54:50 PDT
Comment on attachment 132192 [details]
Patch

Attachment 132192 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11954989
Comment 5 Hajime Morrita 2012-03-16 00:18:30 PDT
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
Comment 6 Gyuyoung Kim 2012-03-16 02:51:35 PDT
(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.
Comment 7 Gyuyoung Kim 2012-03-16 05:58:32 PDT
Created attachment 132264 [details]
Test patch for EWS - Plz do not review
Comment 8 Gyuyoung Kim 2012-03-16 08:49:28 PDT
Created attachment 132291 [details]
Test patch for EWS - Plz do not review

Previous patch was something wrong. I submit same patch again.
Comment 9 Alexey Proskuryakov 2012-03-16 10:58:49 PDT
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.
Comment 10 Gyuyoung Kim 2012-03-19 17:50:53 PDT
(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 ?
Comment 11 Alexey Proskuryakov 2012-03-19 20:44:22 PDT
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.
Comment 12 Gyuyoung Kim 2012-03-19 21:00:50 PDT
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 ?
Comment 13 Alexey Proskuryakov 2012-03-19 21:06:56 PDT
The disconnect here is that you seem to view Internals as something inherently better than LayoutTestController. Why? What problem are you solving?
Comment 14 Gyuyoung Kim 2012-03-19 21:19:49 PDT
(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.
Comment 15 Ryosuke Niwa 2012-03-20 09:38:56 PDT
(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.
Comment 16 Alexey Proskuryakov 2012-03-20 09:45:41 PDT
For the record, I still think that this is a bad idea, but I don't enough interest to fight this case.
Comment 17 WebKit Review Bot 2012-03-20 10:00:02 PDT
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>
Comment 18 WebKit Review Bot 2012-03-20 10:00:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Alexey Proskuryakov 2012-03-20 10:23:02 PDT
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.