Bug 78570 - [DRT] Remove all PlainTextController usages in existing tests by adding internal API.
Summary: [DRT] Remove all PlainTextController usages in existing tests by adding inter...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 79959
  Show dependency treegraph
 
Reported: 2012-02-13 20:14 PST by Kangil Han
Modified: 2012-03-01 12:56 PST (History)
14 users (show)

See Also:


Attachments
Patch (13.09 KB, patch)
2012-02-13 20:23 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (12.10 KB, patch)
2012-02-22 00:21 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (19.16 KB, patch)
2012-02-22 18:32 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (21.02 KB, patch)
2012-02-27 01:22 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (19.98 KB, patch)
2012-02-27 17:58 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (19.96 KB, patch)
2012-02-27 21:55 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (19.79 KB, patch)
2012-02-28 00:24 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (26.87 KB, patch)
2012-02-28 23:28 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (12.84 KB, patch)
2012-02-29 02:22 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (12.87 KB, patch)
2012-02-29 16:05 PST, Kangil Han
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kangil Han 2012-02-13 20:14:36 PST
Implement PlainTextController of DRT in efl port.
Comment 1 Kangil Han 2012-02-13 20:23:57 PST
Created attachment 126898 [details]
Patch
Comment 2 Hajime Morrita 2012-02-20 21:34:41 PST
PlainTextController is a really unfortunate construct of DRT.
How about to kill kill all them in existing test and add Internals API instead?
Comment 3 Kangil Han 2012-02-22 00:21:16 PST
Created attachment 128146 [details]
Patch
Comment 4 Hajime Morrita 2012-02-22 00:25:44 PST
Comment on attachment 128146 [details]
Patch

What I was talking about is Source/WebCore/testing/Internals.idl. 
I'm sorry for my short of explanation. But we cannot simply add new API in visible interfaces.
Comment 5 Kangil Han 2012-02-22 00:30:01 PST
Oops.. :0
I will be back soon with another patch. :)

(In reply to comment #4)
> (From update of attachment 128146 [details])
> What I was talking about is Source/WebCore/testing/Internals.idl. 
> I'm sorry for my short of explanation. But we cannot simply add new API in visible interfaces.
Comment 6 Hajime Morrita 2012-02-22 16:53:34 PST
(In reply to comment #5)
> Oops.. :0
> I will be back soon with another patch. :)
Thank you for your cooperation!
Comment 7 Kangil Han 2012-02-22 18:32:49 PST
Created attachment 128362 [details]
Patch
Comment 8 Philippe Normand 2012-02-22 18:41:49 PST
Comment on attachment 128362 [details]
Patch

Attachment 128362 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11600060
Comment 9 WebKit Review Bot 2012-02-22 20:02:40 PST
Comment on attachment 128362 [details]
Patch

Attachment 128362 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11576086

New failing tests:
fast/dom/prototype-inheritance.html
Comment 10 Hajime Morrita 2012-02-24 02:39:13 PST
For gtk and linux, it looks you need to edit the symbol lists. 
Some existing change like https://bugs.webkit.org/attachment.cgi?id=122870&action=review
may help.
Comment 11 Kangil Han 2012-02-26 17:20:40 PST
Thanks!
I've finished checking gtk build.
I am trying same on windows. ;)

(In reply to comment #10)
> For gtk and linux, it looks you need to edit the symbol lists. 
> Some existing change like https://bugs.webkit.org/attachment.cgi?id=122870&action=review
> may help.
Comment 12 Kangil Han 2012-02-27 01:22:19 PST
Created attachment 128979 [details]
Patch
Comment 13 Kangil Han 2012-02-27 17:58:34 PST
Created attachment 129153 [details]
Patch
Comment 14 Gyuyoung Kim 2012-02-27 20:54:56 PST
Comment on attachment 129153 [details]
Patch

Attachment 129153 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11640429
Comment 15 Kangil Han 2012-02-27 21:55:36 PST
Created attachment 129186 [details]
Patch
Comment 16 Gyuyoung Kim 2012-02-27 22:20:53 PST
Comment on attachment 129186 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129186&action=review

> Source/WebCore/ChangeLog:8
> +        As MORITA Hajime has recommended, this patch will remove all PlainTextController usages

It seems to me that "MORITA Hajime has recommended" is not unneeded in ChangeLog.

> Source/WebCore/PlatformEfl.cmake:277
> +# For details, please look at DumpRenderTreeChrome::onWindowObjectCleared function

Can't this test code be used by WinCE and Blackberry ? I think this code needs to be moved to WebCore/CMakeLists.txt
Comment 17 Kangil Han 2012-02-27 22:33:00 PST
(In reply to comment #16)
> (From update of attachment 129186 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129186&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        As MORITA Hajime has recommended, this patch will remove all PlainTextController usages
> 
> It seems to me that "MORITA Hajime has recommended" is not unneeded in ChangeLog.
> 

Okay, I will remove those comments from ChangeLog

> > Source/WebCore/PlatformEfl.cmake:277
> > +# For details, please look at DumpRenderTreeChrome::onWindowObjectCleared function
> 
> Can't this test code be used by WinCE and Blackberry ? I think this code needs to be moved to WebCore/CMakeLists.txt

I don't think other ports like this idea because they have own way to include Source/WebCore/testing to their ports.
Comment 18 Kangil Han 2012-02-28 00:24:17 PST
Created attachment 129206 [details]
Patch
Comment 19 Gyuyoung Kim 2012-02-28 00:25:02 PST
(In reply to comment #17)

> I don't think other ports like this idea because they have own way to include Source/WebCore/testing to their ports.

In my opinion, you need to ask WinCE and Blackberry maintainer.

CC'ing Patrick, Daniel.
Comment 20 Patrick R. Gansterer 2012-02-28 01:20:59 PST
Comment on attachment 129206 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129206&action=review

IMHO yhou should split this into a) Adding rangeAsText() and b) extend CMake files

> Source/WebCore/PlatformEfl.cmake:275
> +# EFL DRT needs use of WebCore/testing/Internals for testing purpose

Please move this stuff into the CMakeLists.txt as an additonal static library (with the name WebCoreTestSupport) and only link it when building DRT. WinApple port will need this too. I can post my current patch if you're interrested in it.

> Source/WebCore/PlatformEfl.cmake:299
> +    testing/js/WebCoreTestSupport.cpp
> +    testing/js/JSInternalsCustom.cpp

JSC specific -> preblems with V8 -> should go into UseJSC/UseV8.cmake

> Source/WebCore/PlatformEfl.cmake:329
> +        DEPENDS ${WEBCORE_DIR}/bindings/scripts/generate-bindings.pl ${SCRIPTS_BINDINGS} ${WEBCORE_DIR}/bindings/scripts/CodeGeneratorJS.pm ${EFL_ADDITIONAL_SUPPLEMENTAL_DEPENDENCY_FILE}

JSC specifc
Comment 21 Hajime Morrita 2012-02-28 17:05:42 PST
Windows bot is green! It's almost there.
Patrick, thanks for your review. 
Kangil, could you clear the point Patrick mentioned?
I'm happy to r+ this then.
Comment 22 Kangil Han 2012-02-28 17:23:37 PST
Thanks for your review!
I will start investigation.
BTW, could you post your current patch?
I am interested in it. ;)

(In reply to comment #20)
> (From update of attachment 129206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129206&action=review
> 
> IMHO yhou should split this into a) Adding rangeAsText() and b) extend CMake files
> 
> > Source/WebCore/PlatformEfl.cmake:275
> > +# EFL DRT needs use of WebCore/testing/Internals for testing purpose
> 
> Please move this stuff into the CMakeLists.txt as an additonal static library (with the name WebCoreTestSupport) and only link it when building DRT. WinApple port will need this too. I can post my current patch if you're interrested in it.
> 
> > Source/WebCore/PlatformEfl.cmake:299
> > +    testing/js/WebCoreTestSupport.cpp
> > +    testing/js/JSInternalsCustom.cpp
> 
> JSC specific -> preblems with V8 -> should go into UseJSC/UseV8.cmake
> 
> > Source/WebCore/PlatformEfl.cmake:329
> > +        DEPENDS ${WEBCORE_DIR}/bindings/scripts/generate-bindings.pl ${SCRIPTS_BINDINGS} ${WEBCORE_DIR}/bindings/scripts/CodeGeneratorJS.pm ${EFL_ADDITIONAL_SUPPLEMENTAL_DEPENDENCY_FILE}
> 
> JSC specifc
Comment 23 Kangil Han 2012-02-28 17:26:20 PST
Finally succeeded in set up windows build system. ;)
I will figure out Patrick's review.

(In reply to comment #21)
> Windows bot is green! It's almost there.
> Patrick, thanks for your review. 
> Kangil, could you clear the point Patrick mentioned?
> I'm happy to r+ this then.
Comment 24 Kangil Han 2012-02-28 23:28:46 PST
Created attachment 129399 [details]
Patch
Comment 25 Patrick R. Gansterer 2012-02-29 00:08:52 PST
Comment on attachment 129399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129399&action=review

i still like to see this patch split up

I updated the patch at bug 72816 with my current diff

> Source/WebCore/CMakeLists.txt:96
> +SET(WebCoreTestSupport_INCLUDE_DIRECTORIES
> +    "${WebCore_INCLUDE_DIRECTORIES}"
> +    "${WEBCORE_DIR}/testing"
> +)
> +

I'd prefer to set all WebCoreTestSupport_* variables after the WebCore_* variables (like we do with the features). IMHO this will make it easier to find which files are related to WebCoreTestSupport without searching across the big file.

> Source/WebCore/CMakeLists.txt:2522
> +IF (NOT WebCoreTestSupport_LIBRARY_NAME)
> +    SET(WebCoreTestSupport_LIBRARY_NAME webcoretestsupport)
> +ENDIF ()

Can you please add at SET(WebCoreTestSupport_LIBRARY_NAME WebCoreTestSupport) in the root CMakeLists.txt (like the other *_LIBRARY_NAME)

> Source/WebCore/CMakeLists.txt:2524
> +WEBKIT_WRAP_SOURCELIST(${WebCore_IDL_FILES} ${WebCore_SOURCES} ${WebCoreTestSupport_IDL_FILES} ${WebCoreTestSupport_SOURCES})

I'd prefere two call: 1 for WebCore and 1 for WebCoreTestSupport, but its ok for me too, if you prefer it that way :-)

> Source/WebCore/CMakeLists.txt:2529
> +ADD_LIBRARY(${WebCoreTestSupport_LIBRARY_NAME} ${WebCore_LIBRARY_TYPE} ${WebCoreTestSupport_SOURCES})

please create a new LIBRAR_TYPE. WCTS will be usually a static lib only used in DRT

> Source/WebCore/CMakeLists.txt:2532
> +ADD_DEPENDENCIES(${WebCoreTestSupport_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME} ${WebCore_LIBRARY_NAME})

is JSC needed when WebCore is there?

> Source/WebCore/CMakeLists.txt:2535
> +TARGET_LINK_LIBRARIES(${WebCoreTestSupport_LIBRARY_NAME} ${WebCore_LIBRARIES})

Do we need all WebCore libraries for WCTS?
Comment 26 Kangil Han 2012-02-29 00:54:47 PST
Okay, I will split up into 3 patches.

First, adding rangeAsText only in this bug.
Second, common cmake changes for WebCoreTestSupport library in a new bug.
Last, efl port changes in a new bug.

(In reply to comment #25)
> (From update of attachment 129399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129399&action=review
> 
> i still like to see this patch split up
> 
> I updated the patch at bug 72816 with my current diff
> 
> > Source/WebCore/CMakeLists.txt:96
> > +SET(WebCoreTestSupport_INCLUDE_DIRECTORIES
> > +    "${WebCore_INCLUDE_DIRECTORIES}"
> > +    "${WEBCORE_DIR}/testing"
> > +)
> > +
> 
> I'd prefer to set all WebCoreTestSupport_* variables after the WebCore_* variables (like we do with the features). IMHO this will make it easier to find which files are related to WebCoreTestSupport without searching across the big file.
> 
> > Source/WebCore/CMakeLists.txt:2522
> > +IF (NOT WebCoreTestSupport_LIBRARY_NAME)
> > +    SET(WebCoreTestSupport_LIBRARY_NAME webcoretestsupport)
> > +ENDIF ()
> 
> Can you please add at SET(WebCoreTestSupport_LIBRARY_NAME WebCoreTestSupport) in the root CMakeLists.txt (like the other *_LIBRARY_NAME)
> 
> > Source/WebCore/CMakeLists.txt:2524
> > +WEBKIT_WRAP_SOURCELIST(${WebCore_IDL_FILES} ${WebCore_SOURCES} ${WebCoreTestSupport_IDL_FILES} ${WebCoreTestSupport_SOURCES})
> 
> I'd prefere two call: 1 for WebCore and 1 for WebCoreTestSupport, but its ok for me too, if you prefer it that way :-)
> 
> > Source/WebCore/CMakeLists.txt:2529
> > +ADD_LIBRARY(${WebCoreTestSupport_LIBRARY_NAME} ${WebCore_LIBRARY_TYPE} ${WebCoreTestSupport_SOURCES})
> 
> please create a new LIBRAR_TYPE. WCTS will be usually a static lib only used in DRT
> 
> > Source/WebCore/CMakeLists.txt:2532
> > +ADD_DEPENDENCIES(${WebCoreTestSupport_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME} ${WebCore_LIBRARY_NAME})
> 
> is JSC needed when WebCore is there?
> 
> > Source/WebCore/CMakeLists.txt:2535
> > +TARGET_LINK_LIBRARIES(${WebCoreTestSupport_LIBRARY_NAME} ${WebCore_LIBRARIES})
> 
> Do we need all WebCore libraries for WCTS?
Comment 27 Kangil Han 2012-02-29 02:22:04 PST
Created attachment 129423 [details]
Patch
Comment 28 Patrick R. Gansterer 2012-02-29 03:33:07 PST
Comment on attachment 129423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129423&action=review

What about removing PlainTextController now, since it's not used any more?
IMHO keeping it can only result in new tests, not supported by EFL DRT.

> Source/WebCore/ChangeLog:11
> +        No new tests because this patch just modified existing test cases

IMHO this line added no additional value. Maybe sth like "Changed test to use internals.rangeAsText" is better.
Comment 29 Kangil Han 2012-02-29 16:05:13 PST
Created attachment 129541 [details]
Patch
Comment 30 Kangil Han 2012-02-29 16:12:34 PST
(In reply to comment #28)
> (From update of attachment 129423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129423&action=review
> 
> What about removing PlainTextController now, since it's not used any more?
> IMHO keeping it can only result in new tests, not supported by EFL DRT.

Okay, I will create a new bug to remove source code related to PlainTextController in other ports.

> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests because this patch just modified existing test cases
> 
> IMHO this line added no additional value. Maybe sth like "Changed test to use internals.rangeAsText" is better.

Done. :)
Comment 31 Hajime Morrita 2012-03-01 03:17:34 PST
Comment on attachment 129541 [details]
Patch

Looks good! Thanks for your cooperation! And I'm waiting for your followup patches ;-)
Comment 32 WebKit Review Bot 2012-03-01 12:56:09 PST
Comment on attachment 129541 [details]
Patch

Clearing flags on attachment: 129541

Committed r109402: <http://trac.webkit.org/changeset/109402>
Comment 33 WebKit Review Bot 2012-03-01 12:56:17 PST
All reviewed patches have been landed.  Closing bug.