Bug 178158 - [WPE] Enable WOFF2 support
Summary: [WPE] Enable WOFF2 support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-11 00:16 PDT by Adrian Perez
Modified: 2018-03-23 11:37 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.69 KB, patch)
2017-10-11 00:55 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (4.98 KB, patch)
2017-10-12 02:54 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch for landing (5.06 KB, patch)
2018-03-22 14:11 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch for landing (5.06 KB, patch)
2018-03-22 14:19 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2017-10-11 00:16:28 PDT
Probably we want to follow what the GTK+ port does, and expose
an “USE_WOFF2” CMake build option.
Comment 1 Adrian Perez 2017-10-11 00:55:30 PDT
Created attachment 323396 [details]
Patch
Comment 2 Adrian Perez 2017-10-11 00:56:53 PDT
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 3 Frédéric Wang (:fredw) 2017-10-11 01:06:04 PDT
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 4 Michael Catanzaro 2017-10-11 05:23:11 PDT
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.
Comment 5 Adrian Perez 2017-10-12 02:54:01 PDT
Created attachment 323517 [details]
Patch
Comment 6 Adrian Perez 2017-10-12 02:55:27 PDT
(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.
Comment 7 Adrian Perez 2017-10-12 02:57:07 PDT
(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.
Comment 8 Adrian Perez 2017-10-12 03:01:02 PDT
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 9 Frédéric Wang (:fredw) 2017-10-12 06:47:57 PDT
Comment on attachment 323517 [details]
Patch

r+ but please run the woff2 tests locally before landing as they are not executed on the EWS.
Comment 10 Michael Catanzaro 2017-11-13 13:24:08 PST
(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.
Comment 11 Adrian Perez 2018-03-22 14:11:33 PDT
Created attachment 336311 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2018-03-22 14:13:52 PDT
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
Comment 13 Adrian Perez 2018-03-22 14:16:02 PDT
(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 14 WebKit Commit Bot 2018-03-22 14:18:14 PDT
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
Comment 15 Adrian Perez 2018-03-22 14:19:36 PDT
Created attachment 336313 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2018-03-22 14:56:23 PDT
Comment on attachment 336313 [details]
Patch for landing

Clearing flags on attachment: 336313

Committed r229866: <https://trac.webkit.org/changeset/229866>
Comment 17 WebKit Commit Bot 2018-03-22 14:56:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Michael Catanzaro 2018-03-22 15:30:46 PDT
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.
Comment 19 Frédéric Wang (:fredw) 2018-03-22 15:36:07 PDT
(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.
Comment 20 Adrian Perez 2018-03-22 16:24:12 PDT
(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).
Comment 21 Michael Catanzaro 2018-03-22 18:08:21 PDT
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.
Comment 22 Carlos Alberto Lopez Perez 2018-03-23 05:58:26 PDT
Maybe a compromise is to leave the build option, but not make it public.
Comment 23 Michael Catanzaro 2018-03-23 09:51:23 PDT
I don't think that's a good compromise... I would either support it or don't, whichever you prefer.
Comment 24 Adrian Perez 2018-03-23 11:37:26 PDT
(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