Bug 186633 – [Win][MiniBrowser] Change to use WebKit by default if it's available

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Basuke Suzuki]

Add command line switch for WebKit

[Basuke Suzuki]

Add command line switch to use WebKit or WebKitLegacy

[Basuke Suzuki]

Created attachment 342763 [details]
PATCH

[youenn fablet]

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?

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 342770 [details]
FIX

Thanks youenn for r+.

[EWS]

Comment on attachment 342770 [details]
FIX

Rejecting attachment 342770 [details] from commit-queue.

[e-mail] 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.

[youenn fablet]

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.

[Basuke Suzuki]

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?

[youenn fablet]

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

[Basuke Suzuki]

Thanks. I go with constructor now.

[WebKit Commit Bot]

Comment on attachment 342770 [details]
FIX

Clearing flags on attachment: 342770

Committed r232862: <https://trac.webkit.org/changeset/232862>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41145521>