Implement PlainTextController of DRT in efl port.
Created attachment 126898 [details] Patch
PlainTextController is a really unfortunate construct of DRT. How about to kill kill all them in existing test and add Internals API instead?
Created attachment 128146 [details] Patch
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.
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.
(In reply to comment #5) > Oops.. :0 > I will be back soon with another patch. :) Thank you for your cooperation!
Created attachment 128362 [details] Patch
Comment on attachment 128362 [details] Patch Attachment 128362 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11600060
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
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.
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.
Created attachment 128979 [details] Patch
Created attachment 129153 [details] Patch
Comment on attachment 129153 [details] Patch Attachment 129153 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11640429
Created attachment 129186 [details] Patch
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
(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.
Created attachment 129206 [details] Patch
(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 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
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.
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
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.
Created attachment 129399 [details] Patch
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?
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?
Created attachment 129423 [details] Patch
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.
Created attachment 129541 [details] Patch
(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 on attachment 129541 [details] Patch Looks good! Thanks for your cooperation! And I'm waiting for your followup patches ;-)
Comment on attachment 129541 [details] Patch Clearing flags on attachment: 129541 Committed r109402: <http://trac.webkit.org/changeset/109402>
All reviewed patches have been landed. Closing bug.