Bug 233152

Summary: Disable experimental web features on platforms without UI to enable them
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebCore Misc.Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED WONTFIX    
Severity: Normal CC: achristensen, benjamin, bfulgham, cdumez, changseok, cmarcelo, commit-queue, dino, eric.carlson, esprehn+autocc, ews-watchlist, glenn, jer.noble, jonlee, koivisto, kondapallykalyan, pdr, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 233304, 233641    
Bug Blocks:    
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch v3
none
Patch v4
bfulgham: review+, ews-feeder: commit-queue-
Patch v5 for landing none

Description David Kilzer (:ddkilzer) 2021-11-15 14:58:15 PST
Disable experimental web features on platforms without UI to enable them.

Both tvOS and watchOS include WebKit, but they don't have UI to enable or disable experimental web features, so this just ends up being dead code on customer hardware/

The initial version of this patch disables these experimental features (chosen from macros used to enable features in Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml):

ENABLE_CORE_IMAGE_ACCELERATED_FILTER_RENDER
ENABLE_CSS_PAINTING_API
ENABLE_CSS_TYPED_OM
ENABLE_MODEL_ELEMENT
ENABLE_SEPARATED_MODEL
HAVE_CELESTIAL
HAVE_CFNETWORK_ALTERNATIVE_SERVICE
HAVE_AVSAMPLEBUFFERVIDEOOUTPUT
Comment 1 Radar WebKit Bug Importer 2021-11-15 15:03:51 PST
<rdar://problem/85430643>
Comment 2 David Kilzer (:ddkilzer) 2021-11-15 15:13:52 PST
Created attachment 444311 [details]
Patch v1
Comment 3 David Kilzer (:ddkilzer) 2021-11-15 15:24:59 PST
(In reply to David Kilzer (:ddkilzer) from comment #2)
> Created attachment 444311 [details]
> Patch v1

Curious...this patch built a couple days ago on watchOS just fine.
Comment 4 David Kilzer (:ddkilzer) 2021-11-15 16:44:04 PST
(In reply to David Kilzer (:ddkilzer) from comment #3)
> (In reply to David Kilzer (:ddkilzer) from comment #2)
> > Created attachment 444311 [details]
> > Patch v1
> 
> Curious...this patch built a couple days ago on watchOS just fine.

I'm about 80% certain that the issue is that Source/WebCore/DerivedSources-input.xcfilelist doesn't include dependences for Platform.h or the headers it includes (as part of Source/WebCore/DerivedSources.make), so the IDLs are not rebuilt on incremental builds when only a header file changes.

I'm trying to figure out how to make Tools/Scripts/generate-xcfilelists include Platform.h header dependencies, but it might be a while.  Lots of new-to-me Python code that that wasn't easy to read at first glance.
Comment 5 Alex Christensen 2021-11-15 16:45:18 PST
How much does this reduce binary size?
Comment 6 David Kilzer (:ddkilzer) 2021-11-15 18:52:06 PST
Created attachment 444327 [details]
Patch v2
Comment 7 David Kilzer (:ddkilzer) 2021-11-15 18:53:37 PST
(In reply to David Kilzer (:ddkilzer) from comment #6)
> Created attachment 444327 [details]
> Patch v2

Manually update Source/WebCore/DerivedSources-input.xcfilelist to include header files from Source/WTF as input dependencies to see if that fixes the tv/watch builds.
Comment 8 David Kilzer (:ddkilzer) 2021-11-15 19:57:00 PST
(In reply to David Kilzer (:ddkilzer) from comment #7)
> (In reply to David Kilzer (:ddkilzer) from comment #6)
> > Created attachment 444327 [details]
> > Patch v2
> 
> Manually update Source/WebCore/DerivedSources-input.xcfilelist to include
> header files from Source/WTF as input dependencies to see if that fixes the
> tv/watch builds.

Need to check what path is being used for WTF headers when pre-processing <wtf/Platform.h>.
Comment 9 David Kilzer (:ddkilzer) 2021-11-16 07:52:50 PST
(In reply to Alex Christensen from comment #5)
> How much does this reduce binary size?

Savings in WebCore.framework/WebCore alone are about 1.2% to 1.3% of the total size:

- armv7k:  (59215528 - 58461164) / 1024 = 737 Kb
- arm64_32:  (69203584 - 68297120) / 1024 = 885 Kb
- arm64e:  (72847152 - 71949768) / 1024 = 876 Kb
Comment 10 David Kilzer (:ddkilzer) 2021-11-16 16:33:23 PST
Created attachment 444449 [details]
Patch v3
Comment 11 David Kilzer (:ddkilzer) 2021-11-16 16:34:59 PST
(In reply to David Kilzer (:ddkilzer) from comment #10)
> Created attachment 444449 [details]
> Patch v3

Add debugging output for Source/WebCore/DerivedSources.make to see which Platform.h headers are used.
Comment 12 David Kilzer (:ddkilzer) 2021-11-16 20:15:18 PST
(In reply to David Kilzer (:ddkilzer) from comment #11)
> (In reply to David Kilzer (:ddkilzer) from comment #10)
> > Created attachment 444449 [details]
> > Patch v3
> 
> Add debugging output for Source/WebCore/DerivedSources.make to see which
> Platform.h headers are used.

And I probably worked around the missing dependency by modifying DerivedSources.make.
Comment 13 David Kilzer (:ddkilzer) 2021-11-17 07:01:57 PST
Just realized this morning that both the targets in DerivedSources.make and DerivedSources-input.xcfilelist need dependencies on the Platform.h headers for this to work correctly.

Also, I’m curious why Keith Rollin didn’t just add a target to DerivedSources.make to regenerate DerivedSources-input.xcfilelist (instead of writing Python scripts) as the Makefile has all the lists of files needed to do so.
Comment 14 David Kilzer (:ddkilzer) 2021-11-17 20:24:01 PST
(In reply to David Kilzer (:ddkilzer) from comment #13)
> Also, I’m curious why Keith Rollin didn’t just add a target to
> DerivedSources.make to regenerate DerivedSources-input.xcfilelist (instead
> of writing Python scripts) as the Makefile has all the lists of files needed
> to do so.

Answer: The Python script is doing something like this, as fixing a FIXME about missing header dependencies updates DerivedSources-input.xcfilelist without any Python code changes.  Magical!
Comment 15 David Kilzer (:ddkilzer) 2021-11-18 18:41:28 PST
Created attachment 444770 [details]
Patch v4
Comment 16 Brent Fulgham 2021-11-29 12:39:25 PST
Comment on attachment 444770 [details]
Patch v4

Nice! r=me
Comment 17 EWS 2021-11-29 13:36:59 PST
Tools/Scripts/svn-apply failed to apply attachment 444770 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 18 David Kilzer (:ddkilzer) 2021-11-29 14:42:54 PST
Created attachment 445348 [details]
Patch v5 for landing
Comment 19 David Kilzer (:ddkilzer) 2021-11-29 15:20:41 PST
Comment on attachment 445348 [details]
Patch v5 for landing

Adding cq+ since tvOS and watchOS builds succeeded.
Comment 20 EWS 2021-11-29 16:20:39 PST
Committed r286272 (244633@main): <https://commits.webkit.org/244633@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445348 [details].
Comment 21 WebKit Commit Bot 2021-11-30 09:06:12 PST
Re-opened since this is blocked by bug 233641
Comment 22 David Kilzer (:ddkilzer) 2021-11-30 10:50:52 PST
Moving this to RESOLVED/WONTFIX since we need to take a different approach.