Bug 79896

Summary: [CMake][DRT] Add WebCoreTestSupport
Product: WebKit Reporter: Kangil Han <kangil.han>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, gyuyoung.kim, lucas.de.marchi, mxie, paroga, rakuco, rwlbuis, tmpsantos, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 79900    
Attachments:
Description Flags
Patch
none
Patch none

Description Kangil Han 2012-02-29 01:50:27 PST
Add WebCoreTestSupport to CMake for DRT.
Comment 1 Kangil Han 2012-02-29 03:14:21 PST
Created attachment 129428 [details]
Patch
Comment 2 Patrick R. Gansterer 2012-02-29 03:27:31 PST
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()
Comment 3 Kangil Han 2012-03-01 21:12:20 PST
Created attachment 129803 [details]
Patch
Comment 4 Kangil Han 2012-03-01 21:13:53 PST
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()
Comment 5 Patrick R. Gansterer 2012-03-01 23:29:43 PST
Comment on attachment 129803 [details]
Patch

LGTM
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-03-02 04:15:33 PST
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.
Comment 7 Patrick R. Gansterer 2012-03-02 04:21:24 PST
(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.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-03-02 04:27:31 PST
(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.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-03-02 04:28:22 PST
(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.
Comment 10 Patrick R. Gansterer 2012-03-02 04:31:38 PST
(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. :-(
Comment 11 Thiago Marcos P. Santos 2012-03-02 04:46:44 PST
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.
Comment 12 Daniel Bates 2012-03-05 07:57:22 PST
Comment on attachment 129803 [details]
Patch

r=me
Comment 13 Patrick R. Gansterer 2012-03-05 13:02:58 PST
Comment on attachment 129803 [details]
Patch

Clearing flags on attachment: 129803

Committed r109787: <http://trac.webkit.org/changeset/109787>
Comment 14 Patrick R. Gansterer 2012-03-05 13:03:10 PST
All reviewed patches have been landed.  Closing bug.