Page MenuHomeFreeBSD

www/qt5-webengine: update to 5.12.1
AcceptedPublic

Authored by kai on Jan 5 2019, 8:10 PM.

Details

Reviewers
rakuco
tcberner
Group Reviewers
kde
Summary

Import the update of Kai Knoblich <freebsd_ports@k-worx.org>

Test Plan
  • F it, we'll do it live!
  • Builds fine on 11.2-, 12.0-RELEASE, 13.0-CURRENT@r344874 amd64 + i386 for ALSA/SNDIO/PULSE
  • Playback of multimedia content should be smoother (thanks to rakuco)

Diff Detail

Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23288
Build 22320: arc lint + arc unit

Event Timeline

tcberner created this revision.Jan 5 2019, 8:10 PM
novel added a subscriber: novel.Jan 13 2019, 3:58 PM
kai updated this revision to Diff 55230.Tue, Mar 19, 1:02 PM

Update www/qt5-webengine to 5.12.1. Many kudos to rakuco and tcberner!

kai retitled this revision from www/qt5-webengine: update to 5.12.0 to www/qt5-webengine: update to 5.12.1.Tue, Mar 19, 1:09 PM
kai edited the test plan for this revision. (Show Details)
kai updated this revision to Diff 55245.Tue, Mar 19, 7:39 PM

Add missing debug files in pkg-plist.

tcberner accepted this revision.Thu, Mar 21, 10:43 AM

As long as @rakuco has nothing to add, ship it!

Macro stlgtm:

This revision is now accepted and ready to land.Thu, Mar 21, 10:43 AM
In D18757#420567, @kai wrote:

Add missing debug files in pkg-plist.

That's not supposed to work, and I think the corresponding commit to the qtwebengine-5.12.1 branch is wrong as well. See https://github.com/freebsd/freebsd-ports-kde/commit/081752e6ca70e2fb3f66452928048e5b776f890a for a longer explanation.

kai updated this revision to Diff 55325.Thu, Mar 21, 4:27 PM

Prefixed *.debug entries with @comment in pkg-plist as suggested by @tcberner.

I forgot to mention that I also did some minor clean-up of some patches to reduce the overall churn.

This revision now requires review to proceed.Thu, Mar 21, 4:27 PM

Thanks a lot for working on this. I've left some inline comments in some of the patches, but there isn't a lot that I think needs changing \o/

www/qt5-webengine/files/patch-config.tests-enable-on-FreeBSD
1 ↗(On Diff #55325)

I think it makes sense to keep this patch (which has an explanation and groups the related changes together) rather than splitting it up into two separate ones.

www/qt5-webengine/files/patch-configure.pri
8 ↗(On Diff #55325)

Is this patch really necessary? We shouldn't be detecting glibc in the first place (and we seem to disable the check in the configure.prf patch), and this change is actually replacing ldd with a completely unrelated program.

www/qt5-webengine/files/patch-mkspecs_features_functions.prf
4

I think it makes sense to keep the patch explanation at the top.

www/qt5-webengine/files/patch-src_3rdparty_chromium_chrome_app_generated__resources.grd
8

is_is_posix should be is_posix here.

www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h
1 ↗(On Diff #55325)

These web_ui patches (I think there are 4 or 5) that replace base::StringPiece usage with std::string seem wrong to me. StringPiece is a core part of Chromium (and it's used everywhere, so it should be working on FreeBSD), and removing those patches passed in my build-test on HEAD.

www/qt5-webengine/files/patch-src_3rdparty_chromium_net_proxy__resolution_proxy__config__service__linux.cc
18

Why do we need to disable KDE support here?

www/qt5-webengine/files/patch-src_3rdparty_chromium_third__party_skia_src_images_SkJpegEncoder.cpp
1 ↗(On Diff #55325)

Do you know why this patch (as well as patch-src_3rdparty_chromium_third__party_skia_src_ports_SkFontHost__FreeType.cpp and patch-src_3rdparty_chromium_ui_gfx_codec_jpeg__codec.cc) is needed?

www/qt5-webengine/files/patch-src_3rdparty_chromium_third__party_yasm_BUILD.gn
1 ↗(On Diff #55325)

Do we need this? We're unbundling yasm.

www/qt5-webengine/files/patch-src_core_core__module.pro
5

Can you restore the patch explanation here?

kai commandeered this revision.Sun, Mar 24, 9:24 PM
kai removed a reviewer: kai.
kai updated this revision to Diff 55414.Sun, Mar 24, 11:19 PM

Addressing the most of @rakuco's suggestions:

  • Restore patch-config.test-enable-on-FreeBSD (and remove both patch-config.tests_{khr_khr,snappy_snappy}.pro)
  • Fixed typo in patch-src_3rdparty_chromium_chrome_app_generated_resources.grd

Restore explanations/boiler plates:

  • patch-mkspecs_features_functions.prf
  • patch-src_code_core__module.pro

Remove superflous patches:

  • Remove superflous patch-configure.pri (it was a leftover from the phase where I tried to get around the glibc check)
  • patch-src_3rdparty_chromium_third__party_skia_src_images_SkJpegEncoder.cpp
  • patch-src_3rdparty_chromium_thirdparty_skia_src_ports_SkFontHostFreeType.cpp
  • patch-src_3rdparty_chromium_third__party_yasm_BUILD.gn
  • patch-src_3rdparty_chromium_ui_gfx_codec_jpeg__codec.cc
kai marked 4 inline comments as done.Sun, Mar 24, 11:29 PM
kai added inline comments.
www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h
1 ↗(On Diff #55325)

Hrm, I removed all four patch-src_3rdparty_chromium_content_browser_webui_web__ui* patches and then the build failed with

../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/content/browser/webui/web_ui_impl.h:56:65: error: non-virtual member function marked 'override' hides virtual member function
                               const MessageCallback& callback) override;
                                                                ^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/content/public/browser/web_ui.h:77:16: note: hidden overloaded virtual function 'content::WebUI::RegisterMessageCallback' declared here: type mismatch at 1st parameter ('const std::string &' (aka 'const basic_string<char, char_traits<char>, allocator<char> > &') vs 'base::StringPiece' (aka 'BasicStringPiece<basic_string<char, char_traits<char>, allocator<char> > >'))
  virtual void RegisterMessageCallback(const std::string& message,
www/qt5-webengine/files/patch-src_3rdparty_chromium_net_proxy__resolution_proxy__config__service__linux.cc
18

I tried to build webengine without the both hunks that exclude the KDE support but it failed with:

../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/base/message_loop/message_loop_current.h:190:22: note: copy assignment operator of 'MessageLoopCurrent' is implicitly deleted because field 'current_' is of const-qualified type 'base::MessageLoop *const'
  MessageLoop* const current_;
                     ^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:589:19: error: use of undeclared identifier 'inotify_init'
    inotify_fd_ = inotify_init();
                  ^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:633:27: error: use of undeclared identifier 'IN_MODIFY'
                          IN_MODIFY | IN_MOVED_TO) < 0) {
                          ^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:633:39: error: use of undeclared identifier 'IN_MOVED_TO'
                          IN_MODIFY | IN_MOVED_TO) < 0) {
                                      ^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:911:28: error: use of undeclared identifier 'inotify_event'
    char event_buf[(sizeof(inotify_event) + NAME_MAX + 1) * 4];
                           ^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:919:9: error: unknown type name 'inotify_event'
        inotify_event* event = reinterpret_cast<inotify_event*>(event_ptr);
        ^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:919:49: error: unknown type name 'inotify_event'
        inotify_event* event = reinterpret_cast<inotify_event*>(event_ptr);
                                                ^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:921:37: error: use of undeclared identifier 'inotify_event'
        CHECK_LE(event_ptr + sizeof(inotify_event), event_buf + r);
kai updated this revision to Diff 55415.Sun, Mar 24, 11:33 PM

Really add back files/patch-config.tests-enable-on-FreeBSD

rakuco added inline comments.Mon, Mar 25, 12:55 PM
www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h
1 ↗(On Diff #55325)

Sorry, I should've been more specific. These are the files I removed locally to get the port to build:

  • patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.cc
  • patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h
  • patch-src_3rdparty_chromium_content_browser_webui_web__ui__impl.cc
  • patch-src_3rdparty_chromium_content_browser_webui_web__ui__impl.h
  • patch-src_3rdparty_chromium_content_public_browser_web__ui.h
  • patch-src_3rdparty_chromium_content_public_browser_web__ui__data__source.h
kai updated this revision to Diff 55427.Mon, Mar 25, 2:07 PM

Remove further superfluous patches pointed out by @rakuco.

Many thanks for your all your hints, @rakuco, and that you took the time to look at it all!

Actually now only the thing with patch-src_3rdparty_chromium_net_proxyresolution_proxyconfigservicelinux.cc seems to be open? Otherwise nothing more would stand in the way to bring it finally into the ports tree... \o/

kai marked 2 inline comments as done.Mon, Mar 25, 2:15 PM
kai added inline comments.
www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h
1 ↗(On Diff #55325)

Maybe it makes sense to remove those patches from the www/chromium port where I took them, as well?

rakuco accepted this revision.Mon, Mar 25, 2:45 PM

Thank you very much for this patch. I think we're good to go, and the KDE inotify thing can be tracked separately. I haven't test-built the latest version of this patch in older releases like 11.2 though, it'd be good to do the following:

  • File a bug for tracking the KDE-inotify bug.
  • Test this on at least 11.2 if you haven't already (I can do it tonight if you prefer).
  • Land and start worrying about the next update :-)

@tcberner had different results with the .debug files in the plist, but I think we can also follow this separately and not block the update on it.

www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h
1 ↗(On Diff #55325)

I think it would be a good idea, yes. It's not clear to me why they were needed in the first place.

This revision is now accepted and ready to land.Mon, Mar 25, 2:45 PM
kai marked an inline comment as done.Mon, Mar 25, 3:40 PM

Thank you very much for this patch. I think we're good to go, and the KDE inotify thing can be tracked separately. I haven't test-built the latest version of this patch in older releases like 11.2 though, it'd be good to do the following:

  • File a bug for tracking the KDE-inotify bug.

If I'm not mistaken the Chromium Bug-Tracker is the right platform for that bug?

  • Test this on at least 11.2 if you haven't already (I can do it tonight if you prefer).

I'll do a final build for all releases (11.2-, 12.0-RELEASE and 13.0-CURRENT@r345355 amd64 + i386) with the ALSA or SNDIO or PULSE (and with/without DEBUG) options enabled. I'm optimistic, that all builds will succeed.

  • Land and start worrying about the next update :-)

I did some preparations for qt5-webengine 5.12.2 and as far I can tell is that the same set of patches as here with 5.12.1 work seamless. I did also a quick runtime test that went without any issues. ;-)

Well, qt5-webengine 5.13.x is another story...

In D18757#422029, @kai wrote:
  • File a bug for tracking the KDE-inotify bug.

If I'm not mistaken the Chromium Bug-Tracker is the right platform for that bug?

I meant a bug in our very own bugs.freebsd.org -- from an upstream Chromium point of view everything's working fine on Linux, we just need to fix that on FreeBSD.

kai added a comment.Mon, Mar 25, 5:14 PM

If I'm not mistaken the Chromium Bug-Tracker is the right platform for that bug?

I meant a bug in our very own bugs.freebsd.org -- from an upstream Chromium point of view everything's working fine on Linux, we just need to fix that on FreeBSD.

Ah, k, the bug has been filed under PR236787 . The builders are running...it's only a matter of hours now.