Bug 186633

Add command line switch to use WebKit or WebKitLegacy

Created attachment 342763 [details] PATCH

Comment on attachment 342763 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=342763&action=review > Tools/MiniBrowser/win/Common.cpp:168 > +void parseCommandLine(bool& usesLayeredWebView, bool& useFullDesktop, bool& pageLoadTesting, MainWindow::BrowserWindowType& windowType, _bstr_t& requestedURL) Maybe it should return a struct of all parameters. > Tools/MiniBrowser/win/WinMain.cpp:56 > + MainWindow::BrowserWindowType windowType = MainWindow::BrowserWindowType::WebKitLegacy; Why is it legacy here? If parseCommandLine was returning a struct, there would be no need for setting these default values. > Tools/MiniBrowser/win/WinMain.cpp:109 > +extern "C" __declspec(dllexport) int WINAPI dllLauncherEntryPoint(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPWSTR lpstrCmdLine, int nCmdShow) Isn't it already defined in Tools/MiniBrowser/win/Common.cpp?

Comment on attachment 342763 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=342763&action=review >> Tools/MiniBrowser/win/Common.cpp:168 >> +void parseCommandLine(bool& usesLayeredWebView, bool& useFullDesktop, bool& pageLoadTesting, MainWindow::BrowserWindowType& windowType, _bstr_t& requestedURL) > > Maybe it should return a struct of all parameters. Make sense. >> Tools/MiniBrowser/win/WinMain.cpp:56 >> + MainWindow::BrowserWindowType windowType = MainWindow::BrowserWindowType::WebKitLegacy; > > Why is it legacy here? > If parseCommandLine was returning a struct, there would be no need for setting these default values. Got it. >> Tools/MiniBrowser/win/WinMain.cpp:109 >> +extern "C" __declspec(dllexport) int WINAPI dllLauncherEntryPoint(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPWSTR lpstrCmdLine, int nCmdShow) > > Isn't it already defined in Tools/MiniBrowser/win/Common.cpp? Moved from Common.cpp. Common.cpp was originally included from WinMain.cpp and it is now separate code to be compiled. It is more natural to put entry point to the same source file.

Created attachment 342770 [details] FIX Thanks youenn for r+.

Comment on attachment 342770 [details] FIX Rejecting attachment 342770 [details] from commit-queue. Basuke.Suzuki@sony.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Comment on attachment 342770 [details] FIX View in context: https://bugs.webkit.org/attachment.cgi?id=342770&action=review > Tools/MiniBrowser/win/Common.h:36 > + MainWindow::BrowserWindowType windowType; You could probably initialise its value there too so that you do not need the constructor.

Comment on attachment 342770 [details] FIX View in context: https://bugs.webkit.org/attachment.cgi?id=342770&action=review >> Tools/MiniBrowser/win/Common.h:36 >> + MainWindow::BrowserWindowType windowType; > > You could probably initialise its value there too so that you do not need the constructor. But the default value is conditional. I tried to put them here with #if block, but it seems so unnatural. I saw many examples of constructors with conditional initialization, so it comfortable for me. What do you think?

(In reply to Basuke Suzuki from comment #8) > Comment on attachment 342770 [details] > FIX > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342770&action=review > > >> Tools/MiniBrowser/win/Common.h:36 > >> + MainWindow::BrowserWindowType windowType; > > > > You could probably initialise its value there too so that you do not need the constructor. > > But the default value is conditional. I tried to put them here with #if > block, but it seems so unnatural. I saw many examples of constructors with > conditional initialization, so it comfortable for me. What do you think? I guess you could use a macro but the constructor is fine too.

Thanks. I go with constructor now.

Comment on attachment 342770 [details] FIX Clearing flags on attachment: 342770 Committed r232862: <https://trac.webkit.org/changeset/232862>

All reviewed patches have been landed. Closing bug.

<rdar://problem/41145521>