Bug 186427

Created attachment 342242 [details] Patch (WK1BrowserWindow)

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

What about LegacyBrowserWindow and ModernBrowserWindow?

Created attachment 342248 [details] Patch (WebKitLegacyBrowserWindow)

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.

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

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

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

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.

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

(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

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

How about this names? abstract class: BrowserWindow WK1: LegacyBrowserWindow WK2: WKBrowserWindow It's using WK API.

(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

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

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

<rdar://problem/40990040>