Probably we want to follow what the GTK+ port does, and expose an “USE_WOFF2” CMake build option.
Created attachment 323396 [details] Patch
CC'ing Fred — he has been touching the Brotli/WOFF2 stuff lately, and it's good to have him in the loop to make sure we are doing things well here :-)
Comment on attachment 323396 [details] Patch LGTM, but please enable fast/text/woff2-totalsfntsize.html and fast/text/woff2.html ; it seems that all the fast/text/ tests are skipped on WPE...
Comment on attachment 323396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323396&action=review > Source/cmake/OptionsWPE.cmake:85 > + find_package(BrotliDec 1.0.1) > + if (NOT BROTLIDEC_FOUND) > + message(FATAL_ERROR "librotlidec is needed for USE_WOFF2.") > + endif () We need to remember to remove this and require WOFF2Dec 1.0.2 (I'm guessing that will be the version number) after the next WOFF2 release, since brotli should be pulled in transitively.
Created attachment 323517 [details] Patch
(In reply to Michael Catanzaro from comment #4) > Comment on attachment 323396 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323396&action=review > > > Source/cmake/OptionsWPE.cmake:85 > > + find_package(BrotliDec 1.0.1) > > + if (NOT BROTLIDEC_FOUND) > > + message(FATAL_ERROR "librotlidec is needed for USE_WOFF2.") > > + endif () > > We need to remember to remove this and require WOFF2Dec 1.0.2 (I'm guessing > that will be the version number) after the next WOFF2 release, since brotli > should be pulled in transitively. Yes, let's remove it later once WOFF2 1.0.2 is released — I'll keep an eye on that.
(In reply to Adrian Perez from comment #5) > Created attachment 323517 [details] > Patch This only unskips the following tests: - fast/text/woff2.html - fast/text/woff2-totalsfntsize.html For the rest of fast/text/* tests I think it's better to handle them as a regular gardening.
BTW, I am having trouble with the JHBuild but I wanted to upload the updated patch anyway for review. Feel free to review it, but please don't cq+ it until I get to run the tests locally.
Comment on attachment 323517 [details] Patch r+ but please run the woff2 tests locally before landing as they are not executed on the EWS.
(In reply to Michael Catanzaro from comment #4) > We need to remember to remove this and require WOFF2Dec 1.0.2 (I'm guessing > that will be the version number) after the next WOFF2 release, since brotli > should be pulled in transitively. Bug #179630 for GTK. Please don't forget to make this change if enabling WOFF2 for WPE.
Created attachment 336311 [details] Patch for landing
Comment on attachment 336311 [details] Patch for landing Rejecting attachment 336311 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 336311, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Tools/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/7067035
(In reply to Adrian Perez from comment #11) > Created attachment 336311 [details] > Patch for landing This is a rebased version of the patch, with the only difference that the required woff2 version is 1.0.2 (same as in done in bug #179630 as suggested by Michael), now that I finally got back to this. The tests unskipped by the patch ran fine locally: % run-webkit-tests --wpe --release fast/text/woff2.html fast/text/woff2-totalsfntsize.html Using port 'wpe-wk2' Test configuration: <, x86, release> Placing test results in /home/aperez/wpe/WebKit/WebKitBuild/Release/layout-test-results Baseline search path: platform/wpe -> platform/wk2 -> generic Using Release build Pixel tests disabled Regular timeout: 15000, slow test timeout: 75000 Command line: /home/aperez/wpe/WebKit/Tools/jhbuild/jhbuild-wrapper --wpe run /home/aperez/wpe/WebKit/WebKitBuild/Release/bin/WebKitTestRunner - --lint-test-files warnings: LayoutTests/platform/wpe/TestExpectations:196 Path does not exist. imported/mathml-in-html5 Found 2 tests; running 2, skipping 0. Running 2 tests Running 1 WebKitTestRunner. All 2 tests ran as expected.
Comment on attachment 336311 [details] Patch for landing Rejecting attachment 336311 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 336311, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Tools/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/7067106
Created attachment 336313 [details] Patch for landing
Comment on attachment 336313 [details] Patch for landing Clearing flags on attachment: 336313 Committed r229866: <https://trac.webkit.org/changeset/229866>
All reviewed patches have been landed. Closing bug.
FWIW, I think web fonts are a core feature, so I would not have added the build option to disable them if this were me. It's needed in GTK due to our strict dependency policy there, but WPE hasn't had a release yet: we can depend on whatever we want.
(In reply to Adrian Perez from comment #13) > --lint-test-files warnings: > LayoutTests/platform/wpe/TestExpectations:196 Path does not exist. > imported/mathml-in-html5 This path was finally removed in r229601 so I think we can remove the expectation on WPE.
(In reply to Michael Catanzaro from comment #18) > FWIW, I think web fonts are a core feature, so I would not have added the > build option to disable them if this were me. It's needed in GTK due to our > strict dependency policy there, but WPE hasn't had a release yet: we can > depend on whatever we want. Good point. The savings of not having to install brotli+woff2 would be in the ballpark of 1 MiB. I wonder if that's enough of savings in embedded in 2018 to justify a build option, but I lean towards leaving the build option for now, at least for as long as we support it in the GTK+ port. Some embedded distributions where WPE may be built do not have brotli and woff2 packages (yet, I am working myself on getting Buildroot up to speed, at least).
My $0.02: it's going to be a lot harder to argue for its removal down the road than it would be to simply not add it now.
Maybe a compromise is to leave the build option, but not make it public.
I don't think that's a good compromise... I would either support it or don't, whichever you prefer.
(In reply to Frédéric Wang (:fredw) from comment #19) > (In reply to Adrian Perez from comment #13) > > --lint-test-files warnings: > > LayoutTests/platform/wpe/TestExpectations:196 Path does not exist. > > imported/mathml-in-html5 > > This path was finally removed in r229601 so I think we can remove the > expectation on WPE. Thanks for the heads up, I have updated the test expectations accordingly: https://trac.webkit.org/changeset/229909