Bug 177804

Summary:

Remove Brotli from Source/ThirdParty

Product:

WebKit

Reporter:

Frédéric Wang (:fredw) <fred.wang>

Component:

WebKitGTK

Assignee:

Frédéric Wang (:fredw) <fred.wang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aperez, bugs-noreply, mcatanzaro

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugzilla.mozilla.org/show_bug.cgi?id=1405703

Bug Depends on:

177768

Bug Blocks:

177862

Attachments:

Description	Flags
Patch (WIP)	none
Patch	none
Patch (177768+177804)	none
Patch	mcatanzaro: review+

Frédéric Wang (:fredw)

Reported 2017-10-03 01:59:52 PDT

After the upgrade to brotli 1.0.1, I believe we can just remove the sources from the WebKit repository and instead use a jhbuild or system library for brotli.

Attachments
Patch (WIP) (825.19 KB, patch) 2017-10-03 03:11 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (833.18 KB, patch) 2017-10-03 08:47 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (177768+177804) (984.65 KB, patch) 2017-10-03 08:49 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (831.97 KB, patch) 2017-10-05 01:00 PDT, Frédéric Wang (:fredw)	mcatanzaro: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Frédéric Wang (:fredw)

Comment 1 2017-10-03 03:11:40 PDT

Created attachment 322505 [details] Patch (WIP) This finds brotli using cmake. Still need to do the jhbuild part.

Michael Catanzaro

Comment 2 2017-10-03 03:19:29 PDT

Comment on attachment 322505 [details] Patch (WIP) View in context: https://bugs.webkit.org/attachment.cgi?id=322505&action=review Super, thanks > Source/cmake/OptionsGTK.cmake:383 > +if (USE_WOFF2) > + find_package(BrotliDec) > + if (NOT BROTLIDEC_FOUND) > + message(FATAL_ERROR "librotlidec is needed for USE_WOFF2.") > + endif () > +endif () Good, but also, instead of setting USE_WOFF2 at the top of OptionsGTK.cmake, you'll need to remove that and set it using WEBKIT_OPTION_DEFINE. It should go next to USE_LIBNOTIFY/USE_LIBHYPHEN/USE_LIBSECRET

Adrian Perez

Comment 3 2017-10-03 03:25:14 PDT

(In reply to Frédéric Wang (:fredw) from comment #0) > After the upgrade to brotli 1.0.1, I believe we can just remove the sources > from the WebKit repository and instead use a jhbuild or system library for > brotli. Less bundled dependencies are always a good thing! \o/

Frédéric Wang (:fredw)

Comment 4 2017-10-03 08:47:52 PDT

Created attachment 322527 [details] Patch Addressing Michael's feedback and adding jhbuild config for brotli.

Frédéric Wang (:fredw)

Comment 5 2017-10-03 08:49:43 PDT

Created attachment 322528 [details] Patch (177768+177804)

Frédéric Wang (:fredw)

Comment 6 2017-10-04 04:29:43 PDT

Brotli should also be removed from Tools/Scripts/webkitpy/style/checker.py

Frédéric Wang (:fredw)

Comment 7 2017-10-05 01:00:35 PDT

Created attachment 322800 [details] Patch

Michael Catanzaro

Comment 8 2017-10-05 01:04:18 PDT

Comment on attachment 322800 [details] Patch Awesome!

Michael Catanzaro

Comment 9 2017-10-05 01:06:17 PDT

You need to pass the min required version to find_package().

Frédéric Wang (:fredw)

Comment 10 2017-10-05 01:16:58 PDT

(In reply to Michael Catanzaro from comment #9) > You need to pass the min required version to find_package(). Right, I'll do it. > UnicodeDecodeError: 'utf8' codec can't decode byte 0xe8 in position 738: invalid continuation byte It seems this is a Python bug with the script that applies the patch, so I'll land it manually.

Frédéric Wang (:fredw)

Comment 11 2017-10-05 02:33:05 PDT

Committed r222907: <http://trac.webkit.org/changeset/222907>

Note You need to log in before you can comment on or make changes to this bug.