Bug 79896

Created attachment 129428 [details] Patch

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

Created attachment 129803 [details] Patch

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 on attachment 129803 [details] Patch LGTM

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.

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

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

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

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

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 on attachment 129803 [details] Patch r=me

Comment on attachment 129803 [details] Patch Clearing flags on attachment: 129803 Committed r109787: <http://trac.webkit.org/changeset/109787>

All reviewed patches have been landed. Closing bug.