Bug 78570 – [DRT] Remove all PlainTextController usages in existing tests by adding internal API.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kangil Han]

Implement PlainTextController of DRT in efl port.

[Kangil Han]

Created attachment 126898 [details]
Patch

[Hajime Morrita]

PlainTextController is a really unfortunate construct of DRT.
How about to kill kill all them in existing test and add Internals API instead?

[Kangil Han]

Created attachment 128146 [details]
Patch

[Hajime Morrita]

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.

[Kangil Han]

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.

[Hajime Morrita]

(In reply to comment #5)
> Oops.. :0
> I will be back soon with another patch. :)
Thank you for your cooperation!

[Kangil Han]

Created attachment 128362 [details]
Patch

[Philippe Normand]

Comment on attachment 128362 [details]
Patch

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

[WebKit Review Bot]

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

[Hajime Morrita]

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.

[Kangil Han]

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.

[Kangil Han]

Created attachment 128979 [details]
Patch

[Kangil Han]

Created attachment 129153 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 129153 [details]
Patch

Attachment 129153 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11640429

[Kangil Han]

Created attachment 129186 [details]
Patch

[Gyuyoung Kim]

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

[Kangil Han]

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

[Kangil Han]

Created attachment 129206 [details]
Patch

[Gyuyoung Kim]

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

[Patrick R. Gansterer]

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

[Hajime Morrita]

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.

[Kangil Han]

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

[Kangil Han]

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.

[Kangil Han]

Created attachment 129399 [details]
Patch

[Patrick R. Gansterer]

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?

[Kangil Han]

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?

[Kangil Han]

Created attachment 129423 [details]
Patch

[Patrick R. Gansterer]

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.

[Kangil Han]

Created attachment 129541 [details]
Patch

[Kangil Han]

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

[Hajime Morrita]

Comment on attachment 129541 [details]
Patch

Looks good! Thanks for your cooperation! And I'm waiting for your followup patches ;-)

[WebKit Review Bot]

Comment on attachment 129541 [details]
Patch

Clearing flags on attachment: 129541

Committed r109402: <http://trac.webkit.org/changeset/109402>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.