Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GTK] Fix cmake build when ENABLE_MHTML flag is off #9243

Closed
wants to merge 1 commit into from

Conversation

kiranpradeep
Copy link
Contributor

@kiranpradeep kiranpradeep commented Jan 27, 2023

62443ea

[GTK] Fix cmake build when ENABLE_MHTML flag is off
https://bugs.webkit.org/show_bug.cgi?id=146905

Unreviewed test gardening

ENABLE(MHTML) Preprocessor guards are added to WebKitWebView.cpp.
This approach  is copied from WKPageGetContentsAsMHTMLData() method
in file WKPage.cpp (path: UIProcess/API/C)

* Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:
(webkit_web_view_save):
(webkit_web_view_save_to_file):

62443ea

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ›  gtk   πŸ›  wincairo
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2   πŸ§ͺ api-mac   πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1   πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  watch-sim

https://bugs.webkit.org/show_bug.cgi?id=146905

Unreviewed test gardening

ENABLE(MHTML) Preprocessor guards are added to WebKitWebView.cpp.
This approach  is copied from WKPageGetContentsAsMHTMLData() method
in file WKPage.cpp (path: UIProcess/API/C)

* Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:
(webkit_web_view_save):
(webkit_web_view_save_to_file):
@kiranpradeep kiranpradeep requested a review from a team as a code owner January 27, 2023 18:52
Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, you need to make sure the callback always gets called. What you have now breaks if ENABLE(MHTML) is disabled: the call will never finish.

P.S. This is not a public build option and disabling it is not supported, but whatever, adding two guards is hardly a big deal.

@kiranpradeep
Copy link
Contributor Author

@mcatanzaro oh private flags - my bad. I was using cmake-gui which did not differentiate what was private flags from public ones. I was trying to build gtk port with just 3 options (Webkit, Webcore, JavascriptCore) enabled (I disabled everything else in cmake-gui). And hit this issue.

I could not test this with minibrowser. So followed what was already in WKPage.cpp line 2673. I am abandoing this PR as it is not expected to be used by public,

@mcatanzaro
Copy link
Contributor

I think cmake-gui should distinguish between ADVANCED options (for developers/experts) vs. user-facing options?

You'll have a real tough time with everything disabled. :)

@mcatanzaro
Copy link
Contributor

I could not test this with minibrowser. So followed what was already in WKPage.cpp line 2673. I am abandoing this PR as it is not expected to be used by public,

You actually found a bug there! The callback should always be executed. Feel free to file a bug report if you want.

@kiranpradeep
Copy link
Contributor Author

You actually found a bug there! The callback should always be executed. Feel free to file a bug report if you want.

@mcatanzaro Raised 251353. But it feels incomplete info as I could not find info on what should values of WKDataRef/WKErrorRef when callback is invoked (with MHTML off).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants