Bug 177862 - Remove WOFF2 from Source/ThirdParty
Summary: Remove WOFF2 from Source/ThirdParty
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 177804 177994
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-04 03:48 PDT by Frédéric Wang (:fredw)
Modified: 2017-10-09 02:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (184.22 KB, patch)
2017-10-04 04:10 PDT, Tomas Popela
fred.wang: review-
Details | Formatted Diff | Diff
Patch (WIP) (193.22 KB, patch)
2017-10-04 04:34 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (198.07 KB, patch)
2017-10-04 06:09 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (177768+177804+177862) (1.11 MB, patch)
2017-10-04 06:12 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (195.50 KB, patch)
2017-10-05 02:50 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (1.91 MB, application/zip)
2017-10-05 04:31 PDT, Build Bot
no flags Details
Patch (181.47 KB, patch)
2017-10-05 23:58 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (181.44 KB, patch)
2017-10-06 08:33 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (181.44 KB, patch)
2017-10-06 08:37 PDT, Frédéric Wang (:fredw)
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2017-10-04 03:48:11 PDT
We can not do that until WOFF2 can be properly packaged, but let's try and prepare a patch for this.
Comment 1 Tomas Popela 2017-10-04 03:53:30 PDT
And also we should remove brotli as it is a dependency of WOFF2
Comment 2 Frédéric Wang (:fredw) 2017-10-04 03:58:27 PDT
(In reply to Tomas Popela from comment #1)
> And also we should remove brotli as it is a dependency of WOFF2

Yes, it's bug 177804 ;-)
Comment 3 Tomas Popela 2017-10-04 04:10:55 PDT
Created attachment 322650 [details]
Patch
Comment 4 Frédéric Wang (:fredw) 2017-10-04 04:30:44 PDT
Comment on attachment 322650 [details]
Patch

Sorry, I was not clear enough. We just want to remove the source from ThirdParty and use a jhbuild / system library instead.
Comment 5 Frédéric Wang (:fredw) 2017-10-04 04:34:46 PDT
Created attachment 322653 [details]
Patch (WIP)

This is what I meant. But I just realized that I also need to make woff2_out.h public, so I'll need to update my WOFF2 branch on github.
Comment 6 Frédéric Wang (:fredw) 2017-10-04 06:09:55 PDT
Created attachment 322660 [details]
Patch
Comment 7 Frédéric Wang (:fredw) 2017-10-04 06:12:04 PDT
Created attachment 322661 [details]
Patch (177768+177804+177862)
Comment 8 Michael Catanzaro 2017-10-04 06:29:30 PDT
(In reply to Frédéric Wang (:fredw) from comment #4)
> Comment on attachment 322650 [details]
> Patch
> 
> Sorry, I was not clear enough. We just want to remove the source from
> ThirdParty and use a jhbuild / system library instead.

I think this patch is what Tom will use in Fedora and RHEL until the license issue is resolved. This is also what we'll wind up using if we're unable to resolve the license situation with Google. Hopefully it won't come to that.
Comment 9 Frédéric Wang (:fredw) 2017-10-05 02:50:31 PDT
Created attachment 322811 [details]
Patch
Comment 10 Michael Catanzaro 2017-10-05 03:07:20 PDT
Comment on attachment 322811 [details]
Patch

Thanks for working on this!

I think this one is not ready for review yet. We'll need to decide whether to tell Linux distros to package your fork, or whether we can convince Google to release a usable library so we don't need your fork, or whether it's best to just keep it under ThirdParty (and update it very regularly!) if we fail at convincing upstream to put this under a reasonable license. Let's wait a few days and see if upstream takes some action on this.
Comment 11 Build Bot 2017-10-05 04:31:41 PDT
Comment on attachment 322811 [details]
Patch

Attachment 322811 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4765928

New failing tests:
workers/wasm-hashset.html
Comment 12 Build Bot 2017-10-05 04:31:43 PDT
Created attachment 322821 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 13 Frédéric Wang (:fredw) 2017-10-05 23:58:47 PDT
Created attachment 322994 [details]
Patch
Comment 14 Frédéric Wang (:fredw) 2017-10-06 08:33:59 PDT
Created attachment 323017 [details]
Patch

Now there is a release we can use as a reference: https://github.com/google/woff2/releases/tag/v1.0.1
Comment 15 Frédéric Wang (:fredw) 2017-10-06 08:37:06 PDT
Created attachment 323019 [details]
Patch
Comment 16 Michael Catanzaro 2017-10-06 09:13:14 PDT
Comment on attachment 323019 [details]
Patch

r=me, but we need to convince Carlos Garcia and Carlos Lopez before landing, since it was just released today. I think it makes sense to match what we are doing for libbrotli, though. And we are very early in the release cycle, so this is the right time to do it.
Comment 17 Carlos Garcia Campos 2017-10-06 09:16:14 PDT
I'm ok
Comment 18 Tomas Popela 2017-10-09 01:24:19 PDT
I tried this patch with freshly packaged woff2 for Fedora and it works (after applying one small, but fundamental patch). Thank you Fred!

--- a/Source/cmake/OptionsGTK.cmake
+++ b/Source/cmake/OptionsGTK.cmake
@@ -387,7 +387,7 @@ if (USE_WOFF2)
     if (NOT BROTLIDEC_FOUND)
        message(FATAL_ERROR "librotlidec is needed for USE_WOFF2.")
     endif ()
-    find_package(WOFF2Dec, 1.0.1)
+    find_package(WOFF2Dec 1.0.1)
     if (NOT WOFF2DEC_FOUND)
        message(FATAL_ERROR "liwoff2dec is needed for USE_WOFF2.")
     endif ()
Comment 19 Frédéric Wang (:fredw) 2017-10-09 01:31:33 PDT
(In reply to Tomas Popela from comment #18)
> I tried this patch with freshly packaged woff2 for Fedora and it works
> (after applying one small, but fundamental patch). Thank you Fred! 

Great thanks! Indeed I quickly edited the patch by hand before going to week-end and made that mistake!

(In reply to Michael Catanzaro from comment #16)
> Comment on attachment 323019 [details]
> Patch
> 
> r=me, but we need to convince Carlos Garcia and Carlos Lopez before landing,
> since it was just released today. I think it makes sense to match what we
> are doing for libbrotli, though. And we are very early in the release cycle,
> so this is the right time to do it.

It seems that the two Carlo's are fine with that change, so I'll land it.
Comment 20 Frédéric Wang (:fredw) 2017-10-09 02:08:56 PDT
Committed r223042: <http://trac.webkit.org/changeset/223042>