Bug 61350 - [CMAKE] Split JSC related files from WebCore/CMakeLists.txt
Summary: [CMAKE] Split JSC related files from WebCore/CMakeLists.txt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks: 60244
  Show dependency treegraph
 
Reported: 2011-05-24 01:09 PDT by Ryuan Choi
Modified: 2011-07-25 22:21 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2011-05-24 01:09:55 PDT
I already moved JSC related files to UseJSC.cmake at bug 56624, but some files were missing.
Comment 1 Ryuan Choi 2011-05-24 01:15:46 PDT
Created attachment 94582 [details]
Patch
Comment 2 Patrick R. Gansterer 2011-05-24 01:26:04 PDT
LGTM
Comment 3 Raphael Kubo da Costa (:rakuco) 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?
Comment 4 Ryuan Choi 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?
Comment 5 Patrick R. Gansterer 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.
Comment 6 Patrick R. Gansterer 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?
Comment 7 Ryuan Choi 2011-05-25 00:31:03 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?

Sure, It looks quite better.
I wish to make different bug for that.
Comment 8 Ryuan Choi 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.
Comment 9 Ryuan Choi 2011-06-01 08:20:11 PDT
Created attachment 95604 [details]
Patch
Comment 10 Patrick R. Gansterer 2011-06-01 08:21:50 PDT
Comment on attachment 95604 [details]
Patch

LGTM
Comment 11 Ryuan Choi 2011-06-06 18:05:22 PDT
could anyone review this?
Comment 12 Ryuan Choi 2011-07-12 04:29:19 PDT
Created attachment 100466 [details]
rebased_patch
Comment 13 Daniel Bates 2011-07-25 19:31:58 PDT
Comment on attachment 100466 [details]
rebased_patch

OK
Comment 14 WebKit Review Bot 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>
Comment 15 WebKit Review Bot 2011-07-25 22:21:26 PDT
All reviewed patches have been landed.  Closing bug.