Bug 186427 - [Win][MiniBrowser] MiniBrowser class should be renamed to WebKitLegacyBrowserWindow
Summary: [Win][MiniBrowser] MiniBrowser class should be renamed to WebKitLegacyBrowser...
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: InRadar
Depends on:
Blocks: 184770
  Show dependency treegraph
 
Reported: 2018-06-07 22:55 PDT by Fujii Hironori
Modified: 2018-06-10 19:45 PDT (History)
10 users (show)

See Also:


Attachments
Patch (WK1BrowserWindow) (87.82 KB, patch)
2018-06-07 23:03 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (WebKitLegacyBrowserWindow) (88.46 KB, patch)
2018-06-08 03:01 PDT, Fujii Hironori
rniwa: 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-06-07 22:55:27 PDT
[Win][MiniBrowser] MiniBrowser class should be renamed to WK1BrowserWindow

(In reply to Per Arne Vollan from bug #184770 comment #12)
> What do you think about using WK1BrowserWindow/WK2BrowserWindow?
Comment 1 Fujii Hironori 2018-06-07 23:03:47 PDT
Created attachment 342242 [details]
Patch (WK1BrowserWindow)
Comment 2 Build Bot 2018-06-07 23:14:04 PDT
Attachment 342242 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/WK1BrowserWindow.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/WK1BrowserWindow.cpp:30:  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: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Ryosuke Niwa 2018-06-07 23:44:51 PDT
Comment on attachment 342242 [details]
Patch (WK1BrowserWindow)

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

> Tools/MiniBrowser/win/AccessibilityDelegate.cpp:29
> -#include "MiniBrowser.h"
> +#include "WK1BrowserWindow.h"

Can we call it WebKitLegacyBrowserWindow to match the naming convention we have?
Comment 4 Fujii Hironori 2018-06-07 23:53:40 PDT
Comment on attachment 342242 [details]
Patch (WK1BrowserWindow)

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

Thank you for the feedback.

>> Tools/MiniBrowser/win/AccessibilityDelegate.cpp:29
>> +#include "WK1BrowserWindow.h"
> 
> Can we call it WebKitLegacyBrowserWindow to match the naming convention we have?

No problem in my side.

Per wants to match with Mac port MiniBrowser which has WK1BrowserWindowController and WK2BrowserWindowController.

How should the BrowserWindow for modern WebKit be renamed? ModernWebKitBrowserWindow or just WebKitBrowserWindow?
Comment 5 Fujii Hironori 2018-06-07 23:58:51 PDT
What about LegacyBrowserWindow and ModernBrowserWindow?
Comment 6 Fujii Hironori 2018-06-08 03:01:45 PDT
Created attachment 342248 [details]
Patch (WebKitLegacyBrowserWindow)
Comment 7 Build Bot 2018-06-08 03:04:22 PDT
Attachment 342248 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/WebKitLegacyBrowserWindow.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/WebKitLegacyBrowserWindow.cpp:30:  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: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Anders Carlsson 2018-06-08 07:42:47 PDT
(In reply to Fujii Hironori from comment #5)
> What about LegacyBrowserWindow and ModernBrowserWindow?

I don't think you need to use Modern - how about just BrowserWindow and LegacyBrowserWindow.
Comment 9 Ryosuke Niwa 2018-06-08 13:22:22 PDT
Yeah, I think BrowserWindow / LegacyBrowserBrowser makes most sense to me.
Comment 10 Don Olmstead 2018-06-08 13:44:32 PDT
(In reply to Ryosuke Niwa from comment #9)
> Yeah, I think BrowserWindow / LegacyBrowserBrowser makes most sense to me.

Is the rest of the code r+able or are there more changes required?
Comment 11 Ryosuke Niwa 2018-06-08 14:24:36 PDT
I'm a bit confused here. Can we obsolete the old patches and make a new patch with the newly proposed names?

I don't think it makes sense to r+ the rest of the patch given the main point of this patch is rename the class.
Comment 12 Don Olmstead 2018-06-08 14:52:27 PDT
(In reply to Ryosuke Niwa from comment #11)
> I'm a bit confused here. Can we obsolete the old patches and make a new
> patch with the newly proposed names?
> 
> I don't think it makes sense to r+ the rest of the patch given the main
> point of this patch is rename the class.

Its the weekend in Japan so I was offering to rename the classes and files if there was no further issue there so we could get things landed before Fujii was back in the office.
Comment 13 Fujii Hironori 2018-06-08 15:13:34 PDT
(In reply to Anders Carlsson from comment #8)
> (In reply to Fujii Hironori from comment #5)
> > What about LegacyBrowserWindow and ModernBrowserWindow?
> 
> I don't think you need to use Modern - how about just BrowserWindow and
> LegacyBrowserWindow.

Thank you for the feedback.
The name 'BrowserWindow' is already used for the abstract class (bug 186421).

abstract class: BrowserWindow
WK1: LegacyBrowserWindow
WK2: ?

If I use COM for the interface, I can name them:

abstract class: IBrowserWindow
WK1: LegacyBrowserWindow
WK2: BrowserWindow
Comment 14 Darin Adler 2018-06-08 15:24:10 PDT
Those sound good. It’s nice not to perpetuate the obsolete "WebKit 1/2" terminology.
Comment 15 Fujii Hironori 2018-06-08 17:21:23 PDT
How about this names?

abstract class: BrowserWindow
WK1: LegacyBrowserWindow
WK2: WKBrowserWindow

It's using WK API.
Comment 16 Ryosuke Niwa 2018-06-08 22:11:18 PDT
(In reply to Fujii Hironori from comment #15)
> How about this names?
> 
> abstract class: BrowserWindow
> WK1: LegacyBrowserWindow
> WK2: WKBrowserWindow
> 
> It's using WK API.

That also works. But if we're using WebKit prefix, it's probably better to do:

abstract class: BrowserWindow
WK1: WebKitLegacyBrowserWindow
WK2: WebKitBrowserWindow
Comment 17 Fujii Hironori 2018-06-08 23:57:00 PDT
(In reply to Ryosuke Niwa from comment #16)

> That also works. But if we're using WebKit prefix, it's probably better to
> do:
> 
> abstract class: BrowserWindow
> WK1: WebKitLegacyBrowserWindow
> WK2: WebKitBrowserWindow

Agreed. Let’s do that. R+, please.
Comment 18 Fujii Hironori 2018-06-10 19:44:45 PDT
Committed r232669: <https://trac.webkit.org/changeset/232669>
Comment 19 Radar WebKit Bug Importer 2018-06-10 19:45:24 PDT
<rdar://problem/40990040>