Summary: | [Win] MiniBrowser should support both WebKitLegacy and modern WebKit | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||
Component: | Tools / Tests | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, Basuke.Suzuki, bfulgham, don.olmstead, ews-watchlist, lforschler, pvollan, rniwa, ross.kirsling | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 174003, 185418, 185597, 185814, 186029, 186079, 186134, 186263, 186378, 186387, 186421, 186427, 186478 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Fujii Hironori
2018-04-18 21:43:42 PDT
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. |