Bug 61350

Summary:

[CMAKE] Split JSC related files from WebCore/CMakeLists.txt

Product:

WebKit

Reporter:

Ryuan Choi <ryuan.choi>

Component:

WebKit EFL

Assignee:

Ryuan Choi <ryuan.choi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dbates, gyuyoung.kim, kenneth, leandro, lucas.de.marchi, paroga, rakuco, tonikitoo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

60244

Attachments:

Description	Flags
Patch	none
Patch	none
rebased_patch	none

Ryuan Choi

Reported 2011-05-24 01:09:55 PDT

I already moved JSC related files to UseJSC.cmake at bug 56624, but some files were missing.

Attachments
Patch (3.94 KB, patch) 2011-05-24 01:15 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (4.09 KB, patch) 2011-06-01 08:20 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
rebased_patch (4.73 KB, patch) 2011-07-12 04:29 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Ryuan Choi

Comment 1 2011-05-24 01:15:46 PDT

Created attachment 94582 [details] Patch

Patrick R. Gansterer

Comment 2 2011-05-24 01:26:04 PDT

LGTM

Raphael Kubo da Costa (:rakuco)

Comment 3 2011-05-24 06:03:46 PDT

It looks mostly OK to me too. Should the platform-specific ScriptController${PLATFORM}.cpp files still be included in WebCore/CMakeLists{Efl,WinCE}.cmake?

Ryuan Choi

Comment 4 2011-05-24 17:55:03 PDT

(In reply to comment #3) > It looks mostly OK to me too. Should the platform-specific ScriptController${PLATFORM}.cpp files still be included in WebCore/CMakeLists{Efl,WinCE}.cmake? It looks reasonable to me, but WinCE use ScriptControllerWin.cpp. Patrick, can we clone or rename ScriptControllerWin.cpp to ScriptControllerWinCE.cpp?

Patrick R. Gansterer

Comment 5 2011-05-24 23:50:56 PDT

(In reply to comment #4) > (In reply to comment #3) > > It looks mostly OK to me too. Should the platform-specific ScriptController${PLATFORM}.cpp files still be included in WebCore/CMakeLists{Efl,WinCE}.cmake? > > It looks reasonable to me, > but WinCE use ScriptControllerWin.cpp. > > Patrick, can we clone or rename ScriptControllerWin.cpp to ScriptControllerWinCE.cpp? Don't WinApple and WinCairo use this file too? I don't think this is a good idea. IMHO using ${PLATFORM} for filenames and directories is a very bad idea, because different platforms share different code. An example where we have this problem already is http://trac.webkit.org/browser/trunk/Source/WebKit/CMakeLists.txt?rev=83792#L65. When I Create a WinApple, WinCairo and WinGDI port, all of them use the win-directory, but have different implementations of GraphicsCntext, NetworkInterface and so on. I'd like to keep adding of _all_ the platform specific files in different files.

Patrick R. Gansterer

Comment 6 2011-05-24 23:54:43 PDT

What's about merging ScriptControllerBrew, ScriptControllerEfl, ScriptControllerGtk, ScriptControllerHaiku, ScriptControllerWin and ScriptControllerWx and give it a better name like ScriptControllerDefault?

Ryuan Choi

Comment 7 2011-05-25 00:31:03 PDT

Ryuan Choi

Comment 8 2011-06-01 08:11:07 PDT

(In reply to comment #6) > What's about merging ScriptControllerBrew, ScriptControllerEfl, ScriptControllerGtk, ScriptControllerHaiku, ScriptControllerWin and ScriptControllerWx and give it a better name like ScriptControllerDefault? I consider it little more. ScripControllerDefault.cpp can be used to reduce duplication. but, we can't include this file in UseJSC.cmake because some port which use CMake may want own ScriptController${PORT}.cpp. For ScriptController${PLATFORM}.cpp now, I don't have idea without adding if statement to check JavaScript engine in each WebCore/CMakeLists{Efl,WinCE}.txt.

Ryuan Choi

Comment 9 2011-06-01 08:20:11 PDT

Created attachment 95604 [details] Patch

Patrick R. Gansterer

Comment 10 2011-06-01 08:21:50 PDT

Comment on attachment 95604 [details] Patch LGTM

Ryuan Choi

Comment 11 2011-06-06 18:05:22 PDT

could anyone review this?

Ryuan Choi

Comment 12 2011-07-12 04:29:19 PDT

Created attachment 100466 [details] rebased_patch

Daniel Bates

Comment 13 2011-07-25 19:31:58 PDT

Comment on attachment 100466 [details] rebased_patch OK

WebKit Review Bot

Comment 14 2011-07-25 22:21:20 PDT

Comment on attachment 100466 [details] rebased_patch Clearing flags on attachment: 100466 Committed r91742: <http://trac.webkit.org/changeset/91742>

WebKit Review Bot

Comment 15 2011-07-25 22:21:26 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.