Bug 204592

Summary: [Win] Ensure MiniBrowser can be built with !ENABLE(WEBKIT_LEGACY)
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: New BugsAssignee: 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 Flags
Patch none

Description Ross Kirsling 2019-11-25 13:47:29 PST
[Win] Ensure MiniBrowser can be built with !ENABLE(WEBKIT_LEGACY)
Comment 1 Ross Kirsling 2019-11-25 13:47:52 PST
Created attachment 384315 [details]
Patch
Comment 2 Ross Kirsling 2019-11-25 13:50:30 PST
Here's an alternative to bug 204483 which should hopefully be more palatable.
Comment 3 Don Olmstead 2019-11-25 14:20:54 PST
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.
Comment 4 Ross Kirsling 2019-11-25 14:30:13 PST
(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 5 Fujii Hironori 2019-11-25 17:33:01 PST
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.
Comment 6 Ross Kirsling 2019-11-25 17:41:24 PST
(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.
Comment 7 Fujii Hironori 2019-11-25 17:59:07 PST
It's obvious, but I don't object to land this patch if you insist.
Comment 8 Ross Kirsling 2019-11-25 18:13:09 PST
(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 9 WebKit Commit Bot 2019-11-25 18:56:41 PST
Comment on attachment 384315 [details]
Patch

Clearing flags on attachment: 384315

Committed r252872: <https://trac.webkit.org/changeset/252872>
Comment 10 WebKit Commit Bot 2019-11-25 18:56:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-11-25 18:57:17 PST
<rdar://problem/57480701>