Bug 79896 – [CMake][DRT] Add WebCoreTestSupport

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kangil Han]

Add WebCoreTestSupport to CMake for DRT.

[Kangil Han]

Created attachment 129428 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 129428 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests - just CMake changes for DRT.

IMO including this line makes no sense (it adds so additional info), but it's ok to keep it.

> Source/WebCore/CMakeLists.txt:2539
> +    INSTALL(TARGETS ${WebCore_LIBRARY_NAME} ${WebCoreTestSupport_LIBRARY_NAME} DESTINATION lib)

do you really want to INSTALL WebCoreTestSupport?

> Source/WebCore/UseJSC.cmake:281
> +    LIST(APPEND IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n")

no need for LIST(APPEND), you set the variable with a value already including the old value.

> Source/WebCore/UseV8.cmake:267
> +    LIST(APPEND IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n")

LIST(APPEND) -> SET()

[Kangil Han]

Created attachment 129803 [details]
Patch

[Kangil Han]

Done. :)

(In reply to comment #2)
> (From update of attachment 129428 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129428&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests - just CMake changes for DRT.
> 
> IMO including this line makes no sense (it adds so additional info), but it's ok to keep it.
> 
> > Source/WebCore/CMakeLists.txt:2539
> > +    INSTALL(TARGETS ${WebCore_LIBRARY_NAME} ${WebCoreTestSupport_LIBRARY_NAME} DESTINATION lib)
> 
> do you really want to INSTALL WebCoreTestSupport?
> 
> > Source/WebCore/UseJSC.cmake:281
> > +    LIST(APPEND IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n")
> 
> no need for LIST(APPEND), you set the variable with a value already including the old value.
> 
> > Source/WebCore/UseV8.cmake:267
> > +    LIST(APPEND IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n")
> 
> LIST(APPEND) -> SET()

[Patrick R. Gansterer]

Comment on attachment 129803 [details]
Patch

LGTM

[Raphael Kubo da Costa (:rakuco)]

Part of this patch overlaps with bug 79900. CC'ing tmpsantos, it'd be good if you guys could work this out before the patches are committed.

[Patrick R. Gansterer]

(In reply to comment #6)
> Part of this patch overlaps with bug 79900. CC'ing tmpsantos, it'd be good if you guys could work this out before the patches are committed.

Please look at the _new_ patch at that bug. I requested to split the original patch there to make "atomic" commits.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #7)
> Please look at the _new_ patch at that bug. I requested to split the original patch there to make "atomic" commits.

Yeah, but my point is that there are two patches from Kangil and another one from Thiago, and they don't completely overlap: Thiago's patch doesn't split WebCoreTestSupport into a separate library but updates the skipped list, which neither of Kangil's patches do. We need to coordinate this better.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #8)
> Thiago's patch doesn't split WebCoreTestSupport into a separate library but updates the skipped list

EFL's skipped list, I mean.

[Patrick R. Gansterer]

(In reply to comment #8)
> (In reply to comment #7)
> > Please look at the _new_ patch at that bug. I requested to split the original patch there to make "atomic" commits.
> 
> Yeah, but my point is that there are two patches from Kangil and another one from Thiago, and they don't completely overlap: Thiago's patch doesn't split WebCoreTestSupport into a separate library but updates the skipped list, which neither of Kangil's patches do. We need to coordinate this better.

I commented at the other bug in the meantime. Thiago already says it:
> I know there is a patch on the review queue already. I ended up duplicating this work because the bugzilla was down today and I could not search if there was some work ongoing on this already. :(

Sorry for the unclear comment here. :-(

[Thiago Marcos P. Santos]

Yes, (In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Please look at the _new_ patch at that bug. I requested to split the original patch there to make "atomic" commits.
> > 
> > Yeah, but my point is that there are two patches from Kangil and another one from Thiago, and they don't completely overlap: Thiago's patch doesn't split WebCoreTestSupport into a separate library but updates the skipped list, which neither of Kangil's patches do. We need to coordinate this better.
> 
> I commented at the other bug in the meantime. Thiago already says it:
> > I know there is a patch on the review queue already. I ended up duplicating this work because the bugzilla was down today and I could not search if there was some work ongoing on this already. :(
> 
> Sorry for the unclear comment here. :-(

My patch conflicts with this one because I'm adding things from WebCore/testing to WebCore library. I agree we should ship it separately since doesn't make sense to add some bits to WebCore library just because a DRT dependency.

I'll rewrite my patch taking this one into account. Thank you guys for pointing this out.

[Daniel Bates]

Comment on attachment 129803 [details]
Patch

r=me

[Patrick R. Gansterer]

Comment on attachment 129803 [details]
Patch

Clearing flags on attachment: 129803

Committed r109787: <http://trac.webkit.org/changeset/109787>

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.