Bug 178158 – [WPE] Enable WOFF2 support

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Adrian Perez]

Probably we want to follow what the GTK+ port does, and expose
an “USE_WOFF2” CMake build option.

[Adrian Perez]

Created attachment 323396 [details]
Patch

[Adrian Perez]

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 :-)

[Frédéric Wang (:fredw)]

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

[Michael Catanzaro]

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.

[Adrian Perez]

Created attachment 323517 [details]
Patch

[Adrian Perez]

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

[Adrian Perez]

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

[Adrian Perez]

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.

[Frédéric Wang (:fredw)]

Comment on attachment 323517 [details]
Patch

r+ but please run the woff2 tests locally before landing as they are not executed on the EWS.

[Michael Catanzaro]

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

[Adrian Perez]

Created attachment 336311 [details]
Patch for landing

[WebKit Commit Bot]

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

[Adrian Perez]

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

[WebKit Commit Bot]

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

[Adrian Perez]

Created attachment 336313 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 336313 [details]
Patch for landing

Clearing flags on attachment: 336313

Committed r229866: <https://trac.webkit.org/changeset/229866>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

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.

[Frédéric Wang (:fredw)]

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

[Adrian Perez]

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

[Michael Catanzaro]

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.

[Carlos Alberto Lopez Perez]

Maybe a compromise is to leave the build option, but not make it public.

[Michael Catanzaro]

I don't think that's a good compromise... I would either support it or don't, whichever you prefer.

[Adrian Perez]

(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