Summary: | [Win] Ensure MiniBrowser can be built with !ENABLE(WEBKIT_LEGACY) | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||
Component: | New Bugs | Assignee: | Ross Kirsling <ross.kirsling> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, annulen, bfulgham, commit-queue, don.olmstead, ews-watchlist, gyuyoung.kim, Hironori.Fujii, pvollan, ryuan.choi, sergio, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Ross Kirsling
2019-11-25 13:47:29 PST
Created attachment 384315 [details]
Patch
Here's an alternative to bug 204483 which should hopefully be more palatable. Comment on attachment 384315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384315&action=review Fix the nits and we can merge assuming Apple folks have no complaints. > Tools/MiniBrowser/win/WinMain.cpp:46 > +#include "WebKitLegacyBrowserWindow.h" Know this is copy pasta but any reason why this isn't angle brackets? > Tools/PlatformWin.cmake:6 > +if (ENABLE_WEBKIT OR ENABLE_WEBKIT_LEGACY) > + add_subdirectory(MiniBrowser/win) > +endif () > + You can probably just `add_subdirectory` here since its always going to be true. (In reply to Don Olmstead from comment #3) > > Tools/MiniBrowser/win/WinMain.cpp:46 > > +#include "WebKitLegacyBrowserWindow.h" > > Know this is copy pasta but any reason why this isn't angle brackets? It's just a local file and not part of Source/WebKitLegacy. > > Tools/PlatformWin.cmake:6 > > +if (ENABLE_WEBKIT OR ENABLE_WEBKIT_LEGACY) > > + add_subdirectory(MiniBrowser/win) > > +endif () > > + > > You can probably just `add_subdirectory` here since its always going to be > true. I originally did, but since there's some logic for the "both" case, I thought this might be a nice assertion that the "neither" case is not a thing. Could go either way though, I suppose. Comment on attachment 384315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384315&action=review > Tools/ChangeLog:7 > + What is the motivation? Why do you want to disable WebKit1? Disabling only WinCairo WebKit1 doesn't reduce the maintenance cost, but increases because we can't break AppleWin WebKit1. We should accelerate FTW port in order to deprecate AppleWin WebKit1. > Tools/MiniBrowser/win/WinMain.cpp:118 > IWebKitMessageLoopPtr messageLoop; I think IWebKitMessageLoop is problematic because we can't customize the message loop. 1. We can't use IsDialogMessage in the message loop for implementing modal dialogs 2. I have an issue of TranslateAccelerator. We can't customize how to call TranslateAccelerator in the message loop like this https://stackoverflow.com/a/20138696 I'm going to file another bug. (In reply to Fujii Hironori from comment #5) > Comment on attachment 384315 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384315&action=review > > > Tools/ChangeLog:7 > > + > > What is the motivation? > Why do you want to disable WebKit1? > Disabling only WinCairo WebKit1 doesn't reduce the maintenance cost, but > increases because we can't break AppleWin WebKit1. > We should accelerate FTW port in order to deprecate AppleWin WebKit1. This patch doesn't in itself deprecate anything though, it simply ensures that Win MiniBrowser (and thus the full WinCairo build) doesn't require WKL in order to function. This should be independently desirable; I had even assumed that it was already the case prior to investigating this last week. It's obvious, but I don't object to land this patch if you insist. (In reply to Fujii Hironori from comment #7) > It's obvious, but I don't object to land this patch if you insist. Thanks. Comment on attachment 384315 [details] Patch Clearing flags on attachment: 384315 Committed r252872: <https://trac.webkit.org/changeset/252872> All reviewed patches have been landed. Closing bug. |