[Win] MiniBrowser should support both WebKitLegacy WebView and modern WebKit WKView
Created attachment 338406 [details] Patch
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.
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.
Could anyone review this?
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
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.
Created attachment 338636 [details] Patch
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.
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?
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.
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?
(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?
(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.
All patches have been landed. Closed this bug. Thank you very much for supporting.