Bug 178122 – [GTK][WPE] Don't require brotli

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Tomas Popela]

It is unnecessary to depend on brotli as it is only a dependency of woff2. Once there will be a new woff2 release with https://github.com/google/woff2/pull/95 included we should get rid off the brotli checks.

[Tomas Popela]

To be correct - it is not necessary to depend ..

[Tomas Popela]

Created attachment 323294 [details]
Patch

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

Comment on attachment 323294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323294&action=review

> Source/cmake/OptionsGTK.cmake:386
>      find_package(WOFF2Dec 1.0.1)

I'm wondering whether we should wait a new release of WOFF2 before landing this, and then update the version number here.

[Tomas Popela]

(In reply to Frédéric Wang (:fredw) from comment #3)
> I'm wondering whether we should wait a new release of WOFF2 before landing
> this, and then update the version number here.

Good point Fred! I will update the version once it will be released and I'll be landing this change.

[Konstantin Tokarev]

I think it would be better to avoid removing FindBrotliDec from the tree and instead do the following:

1. In FindWOFF2.cmake use check_cxx_symbol_exists(woff2::ConvertWOFF2ToTTF). This will create internal test file which is compiled and linked to WOFF2DEC_LIBRARIES.

2. If check_cxx_symbol_exists fails, e.g. because WOFF2DEC_LIBRARIES is incomplete (broken pkg-config file, or pkg-config was not used at all and WOFF2DEC_LIBRARIES was found by find_library()), invoke find_package(BrotliDec 1.0.1) and append its variables to WOFF2 variables.

[Konstantin Tokarev]

>and append its variables to WOFF2 variables.

Looks like just set(WOFF2DEC_LIBRARIES ${WOFF2DEC_LIBRARIES} {BROTLIDEC_LIBRARIES})

[Michael Catanzaro]

Comment on attachment 323294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323294&action=review

Good catch, Tom!

>> Source/cmake/OptionsGTK.cmake:386
>>      find_package(WOFF2Dec 1.0.1)
> 
> I'm wondering whether we should wait a new release of WOFF2 before landing this, and then update the version number here.

Yes. Perhaps Rick would be willing to make another release for us?

[Michael Catanzaro]

Ah, I forgot to read the last few comments before I wrote my reply. :)

(In reply to Konstantin Tokarev from comment #5)
> I think it would be better to avoid removing FindBrotliDec from the tree and
> instead do the following:
> 
> 1. In FindWOFF2.cmake use check_cxx_symbol_exists(woff2::ConvertWOFF2ToTTF).
> This will create internal test file which is compiled and linked to
> WOFF2DEC_LIBRARIES.
> 
> 2. If check_cxx_symbol_exists fails, e.g. because WOFF2DEC_LIBRARIES is
> incomplete (broken pkg-config file, or pkg-config was not used at all and
> WOFF2DEC_LIBRARIES was found by find_library()), invoke
> find_package(BrotliDec 1.0.1) and append its variables to WOFF2 variables.

This is really unnecessarily complicated. The WOFF2 library is brand new and not packaged by distros yet; there's no need for such a compatibility check.

[Tomas Popela]

Actually I realized that this is change is not needed at all, because we will require brotli at some point when we will the add support for brotli encoding as Dan doesn't want to include it in libsoup - https://bugzilla.gnome.org/show_bug.cgi?id=788418. Žan should we leave the brotli check in WebKit?

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

(In reply to Tomas Popela from comment #9)
> Actually I realized that this is change is not needed at all, because we
> will require brotli at some point when we will the add support for brotli
> encoding as Dan doesn't want to include it in libsoup -
> https://bugzilla.gnome.org/show_bug.cgi?id=788418. Žan should we leave the
> brotli check in WebKit?

OK. Anyway, I think this change is not too urgent. We would still need the change in Source/WebCore/CMakeLists.txt and the modification of the release version when a new version of WOFF2 is released.

[Konstantin Tokarev]

(In reply to Tomas Popela from comment #9)
> Actually I realized that this is change is not needed at all, because we
> will require brotli at some point when we will the add support for brotli
> encoding as Dan doesn't want to include it in libsoup -
> https://bugzilla.gnome.org/show_bug.cgi?id=788418. Žan should we leave the
> brotli check in WebKit?

IMO it would be a better idea to support zstd encoding[1], unlike brotli it's clearly superior to zlib on both client and server side[2]

[1] https://tools.ietf.org/id/draft-kucherawy-dispatch-zstd-00.html
[2] http://zstd.net/

[Michael Catanzaro]

(In reply to Tomas Popela from comment #9)
> Actually I realized that this is change is not needed at all, because we
> will require brotli at some point when we will the add support for brotli
> encoding as Dan doesn't want to include it in libsoup -
> https://bugzilla.gnome.org/show_bug.cgi?id=788418. Žan should we leave the
> brotli check in WebKit?

I don't think we should support brotli decoding unless it's supported by libsoup (which we might still decide to do), so let's still remove this unnecessary brotli check.

[Carlos Alberto Lopez Perez]

(In reply to Konstantin Tokarev from comment #11)
> (In reply to Tomas Popela from comment #9)
> > Actually I realized that this is change is not needed at all, because we
> > will require brotli at some point when we will the add support for brotli
> > encoding as Dan doesn't want to include it in libsoup -
> > https://bugzilla.gnome.org/show_bug.cgi?id=788418. Žan should we leave the
> > brotli check in WebKit?
> 
> IMO it would be a better idea to support zstd encoding[1], unlike brotli
> it's clearly superior to zlib on both client and server side[2]
> 
> [1] https://tools.ietf.org/id/draft-kucherawy-dispatch-zstd-00.html
> [2] http://zstd.net/

There is nowadays an egg-chicken problem with it, and I think the Linux WebKit ports doesn't have enough market share to change the status-quo.

I have made a python script that checks Alexa top N sites to check how many support a given encoding: https://gist.github.com/clopez/dcae6ba605e7fa521b5e989ca323f522

And the numbers I get after a run of the script is that only 3 domains (which are all facebook owned) support zstd

support of encoding zstd
 - Alexa top 100  : 1 domains (1%)
 - Alexa top 1000 : 3 domains (0.3%)
support of encoding br (brotli)
 - Alexa top 100  : 24 domains (24%)
 - Alexa top 1000 : 91 domains (9.1%)
support of encoding gzip
 - Alexa top 100  : 81 domains  (81%)
 - Alexa top 1000 : 867 domains (86.7%)

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #12)
> (In reply to Tomas Popela from comment #9)
> > Actually I realized that this is change is not needed at all, because we
> > will require brotli at some point when we will the add support for brotli
> > encoding as Dan doesn't want to include it in libsoup -
> > https://bugzilla.gnome.org/show_bug.cgi?id=788418. Žan should we leave the
> > brotli check in WebKit?
> 
> I don't think we should support brotli decoding unless it's supported by
> libsoup (which we might still decide to do), so let's still remove this
> unnecessary brotli check.

I think the current patch is right (no matter what we decide now), because there isn't currently any support in webkit for decoding brotli content (AFAIK)...  so lets remove this dependency check from CMake.

If in the future someone wants to propose any feature for WebKit that requires to interact directly with brotli encoded content they can bring this back.

[Michael Catanzaro]

Note a similar change will be needed for WPE (again, after the next WOFF2 release), see bug #178158.

[Zan Dobersek]

(In reply to Tomas Popela from comment #9)
> Actually I realized that this is change is not needed at all, because we
> will require brotli at some point when we will the add support for brotli
> encoding as Dan doesn't want to include it in libsoup -
> https://bugzilla.gnome.org/show_bug.cgi?id=788418. Žan should we leave the
> brotli check in WebKit?

No need, I can revert the removal once it's needed again.

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

Comment on attachment 323294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323294&action=review

> Source/WebCore/CMakeLists.txt:3663
> +  list(APPEND WebCore_LIBRARIES "${WOFF2DEC_LIBRARIES}")

Brotli dep was removed in bug 179630, however we forgot to take that part :-(

@Tomas, Michael: can one of you please handle that?

[Michael Catanzaro]

Thanks for pointing this out, Fred. I'll handle it.

Had forgotten this bug.

[Michael Catanzaro]

Created attachment 326880 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 326880 [details]
Patch

Clearing flags on attachment: 326880

Committed r224817: <https://trac.webkit.org/changeset/224817>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.