Bug 186427 – [Win][MiniBrowser] MiniBrowser class should be renamed to WebKitLegacyBrowserWindow

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fujii Hironori]

[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?

[Fujii Hironori]

Created attachment 342242 [details]
Patch (WK1BrowserWindow)

[EWS Watchlist]

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.

[Ryosuke Niwa]

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?

[Fujii Hironori]

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?

[Fujii Hironori]

What about LegacyBrowserWindow and ModernBrowserWindow?

[Fujii Hironori]

Created attachment 342248 [details]
Patch (WebKitLegacyBrowserWindow)

[EWS Watchlist]

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.

[Anders Carlsson]

(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.

[Ryosuke Niwa]

Yeah, I think BrowserWindow / LegacyBrowserBrowser makes most sense to me.

[Don Olmstead]

(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?

[Ryosuke Niwa]

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.

[Don Olmstead]

(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.

[Fujii Hironori]

(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

[Darin Adler]

Those sound good. It’s nice not to perpetuate the obsolete "WebKit 1/2" terminology.

[Fujii Hironori]

How about this names?

abstract class: BrowserWindow
WK1: LegacyBrowserWindow
WK2: WKBrowserWindow

It's using WK API.

[Ryosuke Niwa]

(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

[Fujii Hironori]

(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.

[Fujii Hironori]

Committed r232669: <https://trac.webkit.org/changeset/232669>

[Radar WebKit Bug Importer]

<rdar://problem/40990040>