Bug 184770 - [Win] MiniBrowser should support both WebKitLegacy and modern WebKit
Summary: [Win] MiniBrowser should support both WebKitLegacy and modern WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords:
Depends on: 174003 185418 185597 185814 186029 186079 186134 186263 186378 186387 186421 186427 186478
Blocks:
  Show dependency treegraph
 
Reported: 2018-04-18 21:43 PDT by Fujii Hironori
Modified: 2018-06-11 18:17 PDT (History)
9 users (show)

See Also:


Attachments
Patch (96.50 KB, patch)
2018-04-20 04:01 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (96.54 KB, patch)
2018-04-23 21:06 PDT, Fujii Hironori
Hironori.Fujii: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2018-04-18 21:43:42 PDT
[Win] MiniBrowser should support both WebKitLegacy WebView and modern WebKit WKView
Comment 1 Fujii Hironori 2018-04-20 04:01:32 PDT
Created attachment 338406 [details]
Patch
Comment 2 EWS Watchlist 2018-04-20 04:04:12 PDT
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.
Comment 3 Don Olmstead 2018-04-20 15:06:33 PDT
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.
Comment 4 Fujii Hironori 2018-04-23 03:17:34 PDT
Could anyone review this?
Comment 5 Don Olmstead 2018-04-23 11:42:41 PDT
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 6 Fujii Hironori 2018-04-23 14:50:08 PDT
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.
Comment 7 Fujii Hironori 2018-04-23 21:06:01 PDT
Created attachment 338636 [details]
Patch
Comment 8 EWS Watchlist 2018-04-23 21:07:38 PDT
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 9 Per Arne Vollan 2018-04-30 20:11:15 PDT
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 10 Fujii Hironori 2018-05-06 19:32:16 PDT
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.
Comment 11 Fujii Hironori 2018-05-10 21:03:26 PDT
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?
Comment 12 Per Arne Vollan 2018-05-11 06:56:18 PDT
(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?
Comment 13 Fujii Hironori 2018-05-13 18:35:39 PDT
(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.
Comment 14 Fujii Hironori 2018-06-11 18:16:58 PDT
All patches have been landed. Closed this bug. Thank you very much for supporting.