Bug 204592 – [Win] Ensure MiniBrowser can be built with !ENABLE(WEBKIT_LEGACY)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

[Win] Ensure MiniBrowser can be built with !ENABLE(WEBKIT_LEGACY)

[Ross Kirsling]

Created attachment 384315 [details]
Patch

[Ross Kirsling]

Here's an alternative to bug 204483 which should hopefully be more palatable.

[Don Olmstead]

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.

[Ross Kirsling]

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

[Fujii Hironori]

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.

[Ross Kirsling]

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

[Fujii Hironori]

It's obvious, but I don't object to land this patch if you insist.

[Ross Kirsling]

(In reply to Fujii Hironori from comment #7)
> It's obvious, but I don't object to land this patch if you insist.

Thanks.

[WebKit Commit Bot]

Comment on attachment 384315 [details]
Patch

Clearing flags on attachment: 384315

Committed r252872: <https://trac.webkit.org/changeset/252872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57480701>