Bug 184770 – [Win] MiniBrowser should support both WebKitLegacy and modern WebKit

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fujii Hironori]

[Win] MiniBrowser should support both WebKitLegacy WebView and modern WebKit WKView

[Fujii Hironori]

Created attachment 338406 [details]
Patch

[EWS Watchlist]

Attachment 338406 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/WK2ContentWindow.cpp:25:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/Common.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/WinMain.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/MiniBrowser.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/BrowserWindow.cpp:29:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/BrowserWindow.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 6 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Only thing I wanted to make sure of is that we don't have a requirement on CoreFoundation on WinCairo with Modern WebKit. Ideally we want to jettison CFLite once we have Modern WebKit working.

[Fujii Hironori]

Could anyone review this?

[Don Olmstead]

Comment on attachment 338406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338406&action=review

Just noting some CF code included.

> Tools/MiniBrowser/win/MiniBrowser.cpp:575
> +static void setWindowText(HWND dialog, UINT field, CFDictionaryRef dictionary, CFStringRef key, UINT& total)

CF code without any USE(CF) guards

> Tools/MiniBrowser/win/WinMain.cpp:32
> +#include <CoreFoundation/CFRunLoop.h>

This should have a USE(CF) guard

[Fujii Hironori]

Comment on attachment 338406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338406&action=review

>> Tools/MiniBrowser/win/MiniBrowser.cpp:575
>> +static void setWindowText(HWND dialog, UINT field, CFDictionaryRef dictionary, CFStringRef key, UINT& total)
> 
> CF code without any USE(CF) guards

MiniBrowser.cpp is only for WK1. I will rename this file WK1ContentWindow.cpp.

>> Tools/MiniBrowser/win/WinMain.cpp:32
>> +#include <CoreFoundation/CFRunLoop.h>
> 
> This should have a USE(CF) guard

Will do.

[Fujii Hironori]

Created attachment 338636 [details]
Patch

[EWS Watchlist]

Attachment 338636 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/WK2ContentWindow.cpp:25:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/Common.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/WinMain.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/MiniBrowser.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/BrowserWindow.cpp:29:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/BrowserWindow.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 6 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Comment on attachment 338636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338636&action=review

Perhaps you could consider splitting this up in several patches if possible? This will make it easier to reason about the changes.

> Tools/MiniBrowser/win/MiniBrowser.h:49
> +class MiniBrowser : public ContentWindow {

Would it make sense for MiniBrowser to have a ContentWindow, instead of being a ContentWindow?

> Tools/MiniBrowser/win/WK2ContentWindow.cpp:162
> +void WK2ContentWindow::setAVFoundationEnabled(bool)
> +{
> +}
> +
> +void WK2ContentWindow::setAcceleratedCompositingEnabled(bool)
> +{
> +}
> +
> +void WK2ContentWindow::setAuthorAndUserStylesEnabled(bool)
> +{
> +}
> +
> +void WK2ContentWindow::setFullScreenEnabled(bool)
> +{
> +}
> +
> +void WK2ContentWindow::setJavaScriptEnabled(bool)
> +{
> +}

Isn't an implementation needed in the modern WebKit case?

[Fujii Hironori]

Comment on attachment 338636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338636&action=review

Thank you for the review, Per. I'll split this patch.

>> Tools/MiniBrowser/win/MiniBrowser.h:49
>> +class MiniBrowser : public ContentWindow {
> 
> Would it make sense for MiniBrowser to have a ContentWindow, instead of being a ContentWindow?

No, I can't. I need a abstract class to switch WK1 and WK2. So, I added a abstract class ContentWindow.
WK2ContentWinodow and MiniBrowser need to inherit the ContentWindow.
I will rename MiniBrowser to WK1ContentWinodow.

>> Tools/MiniBrowser/win/WK2ContentWindow.cpp:162
>> +}
> 
> Isn't an implementation needed in the modern WebKit case?

Yes. Not implemented yet.

[Fujii Hironori]

Per, 

We had a discussion this morning, and agreed to rename MiniBrowser class to WK1BrowserWindowController not WK1ContentWinodow because Mac MiniBrowser has WK1BrowserWindowController.

But, after that discussion, I remembered the reason why I named it WK1ContentWinodow.

Mac MiniBrowser has BrowserWindowController. This is a controller of BrowserWindow as the sense of MVC model.

On the other hand, WK1BrowserWindowController of Windows port MinBrowser represents a browser content window, not a simple controller of M or V.

Should I named it WK1BrowserWindowController for the sake of consistency with Mac port?

[Per Arne Vollan]

(In reply to Fujii Hironori from comment #11)
> Per, 
> 
> We had a discussion this morning, and agreed to rename MiniBrowser class to
> WK1BrowserWindowController not WK1ContentWinodow because Mac MiniBrowser has
> WK1BrowserWindowController.
> 
> But, after that discussion, I remembered the reason why I named it
> WK1ContentWinodow.
> 
> Mac MiniBrowser has BrowserWindowController. This is a controller of
> BrowserWindow as the sense of MVC model.
> 
> On the other hand, WK1BrowserWindowController of Windows port MinBrowser
> represents a browser content window, not a simple controller of M or V.
> 
> Should I named it WK1BrowserWindowController for the sake of consistency
> with Mac port?

What do you think about using WK1BrowserWindow/WK2BrowserWindow?

[Fujii Hironori]

(In reply to Per Arne Vollan from comment #12)
> What do you think about using WK1BrowserWindow/WK2BrowserWindow?

Sounds good. I will add a new class MainWindow as the parent window of WK1BrowserWindow/WK2BrowserWindow.

[Fujii Hironori]

All patches have been landed. Closed this bug. Thank you very much for supporting.