Bug 229780 - Fixes for build-webkit --minimal
Summary: Fixes for build-webkit --minimal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-01 16:49 PDT by Philip Chimento
Modified: 2022-02-15 16:05 PST (History)
44 users (show)

See Also:

Attachments
Patch (73.33 KB, patch)
2021-09-01 17:16 PDT, Philip Chimento 		no flags Details | Formatted Diff | Diff
Patch (71.16 KB, patch)
2021-09-01 17:25 PDT, Philip Chimento 		no flags Details | Formatted Diff | Diff
Remove ENABLE(INTERSECTION_OBSERVER) (27.46 KB, patch)
2021-09-15 14:24 PDT, Philip Chimento 		Hironori.Fujii: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Remove ENABLE(RESIZE_OBSERVER) (24.19 KB, patch)
2021-09-15 14:25 PDT, Philip Chimento 		Hironori.Fujii: review+
Details | Formatted Diff | Diff
Remove features that no longer exist (4.01 KB, patch)
2021-09-15 14:26 PDT, Philip Chimento 		no flags Details | Formatted Diff | Diff
Various changes for 'build-webkit --minimal' (24.88 KB, patch)
2021-09-15 14:28 PDT, Philip Chimento 		no flags Details | Formatted Diff | Diff
Remove ENABLE(INTERSECTION_OBSERVER) (27.67 KB, patch)
2021-09-15 17:08 PDT, Philip Chimento 		no flags Details | Formatted Diff | Diff
Remove ENABLE(RESIZE_OBSERVER) (24.30 KB, patch)
2021-09-16 11:14 PDT, Philip Chimento 		philip.chimento: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Remove ENABLE(RESIZE_OBSERVER) (27.04 KB, patch)
2021-09-16 12:13 PDT, Philip Chimento 		no flags Details | Formatted Diff | Diff
Various changes for 'build-webkit --minimal' (24.95 KB, patch)
2021-09-16 15:49 PDT, Philip Chimento 		no flags Details | Formatted Diff | Diff
Patch (21.39 KB, patch)
2022-02-11 13:42 PST, Philip Chimento 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.48 KB, patch)
2022-02-11 13:52 PST, Philip Chimento 		no flags Details | Formatted Diff | Diff
Patch (19.79 KB, patch)
2022-02-15 09:49 PST, Philip Chimento 		no flags Details | Formatted Diff | Diff
Show Obsolete (12) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Chimento 2021-09-01 16:49:41 PDT 
Fixes for build-webkit --minimal
Comment 1 Philip Chimento 2021-09-01 17:16:23 PDT 
Created attachment 437088 [details]
Patch
Comment 2 Philip Chimento 2021-09-01 17:19:43 PDT 
I've been trying to use webkit-build --minimal and have made some progress on getting things to build. I was suggested on Slack to remove the INTERSECTION_OBSERVER and RESIZE_OBSERVER options altogether since disabling them was quite broken, it must mean that nobody actually needs to build with them disabled.

webkit-patch seems to have squashed my four commits into one patch, I hope that is OK! If that's too tedious to review, I'm happy to reupload four patches manually.
Comment 3 Philip Chimento 2021-09-01 17:25:24 PDT 
Created attachment 437091 [details]
Patch
Comment 4 EWS Watchlist 2021-09-01 17:26:44 PDT 
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 5 Fujii Hironori 2021-09-07 18:02:40 PDT 
(In reply to Philip Chimento from comment #2)
> webkit-patch seems to have squashed my four commits into one patch, I hope
> that is OK! If that's too tedious to review, I'm happy to reupload four
> patches manually.

Not OK. Please split the patch.
Comment 6 Radar WebKit Bug Importer 2021-09-08 16:50:20 PDT 
<rdar://problem/82898564>
Comment 7 Philip Chimento 2021-09-15 14:24:33 PDT 
Created attachment 438287 [details]
Remove ENABLE(INTERSECTION_OBSERVER)
Comment 8 Philip Chimento 2021-09-15 14:25:40 PDT 
Created attachment 438288 [details]
Remove ENABLE(RESIZE_OBSERVER)
Comment 9 Philip Chimento 2021-09-15 14:26:34 PDT 
Created attachment 438289 [details]
Remove features that no longer exist
Comment 10 Philip Chimento 2021-09-15 14:28:15 PDT 
Created attachment 438290 [details]
Various changes for 'build-webkit --minimal'
Comment 11 Philip Chimento 2021-09-15 14:29:33 PDT 
Done! (I guess the ENABLE(RESIZE_OBSERVER) patch will not apply cleanly until the ENABLE(INTERSECTION_OBSERVER) patch lands.)
Comment 12 Fujii Hironori 2021-09-15 14:39:21 PDT 
Comment on attachment 438287 [details]
Remove ENABLE(INTERSECTION_OBSERVER)

LGTM if the EWS won't complain.
Comment 13 Fujii Hironori 2021-09-15 14:40:18 PDT 
Comment on attachment 438288 [details]
Remove ENABLE(RESIZE_OBSERVER)

LGTM if the EWS won't complain.
Comment 14 Philip Chimento 2021-09-15 17:08:16 PDT 
Created attachment 438305 [details]
Remove ENABLE(INTERSECTION_OBSERVER)
Comment 15 Philip Chimento 2021-09-15 17:09:42 PDT 
Comment on attachment 438305 [details]
Remove ENABLE(INTERSECTION_OBSERVER)

Carrying forward r+, the only change is adding an #endif that had been swallowed into the next patch
Comment 16 EWS 2021-09-15 18:40:46 PDT 
Committed r282485 (241733@main): <https://commits.webkit.org/241733@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438289 [details].
Comment 17 Philip Chimento 2021-09-15 19:28:29 PDT 
Reopened, still 3 to land :-)
Comment 18 EWS 2021-09-15 21:39:59 PDT 
Committed r282487 (241735@main): <https://commits.webkit.org/241735@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438305 [details].
Comment 19 Philip Chimento 2021-09-16 11:14:21 PDT 
Reopening to attach new patch.
Comment 20 Philip Chimento 2021-09-16 11:14:25 PDT 
Created attachment 438373 [details]
Remove ENABLE(RESIZE_OBSERVER)
Comment 21 Philip Chimento 2021-09-16 11:15:31 PDT 
Comment on attachment 438373 [details]
Remove ENABLE(RESIZE_OBSERVER)

Carrying forward r+
Comment 22 Philip Chimento 2021-09-16 12:13:40 PDT 
Created attachment 438381 [details]
Remove ENABLE(RESIZE_OBSERVER)
Comment 23 Philip Chimento 2021-09-16 12:14:58 PDT 
Comment on attachment 438381 [details]
Remove ENABLE(RESIZE_OBSERVER)

Carrying forward r+. The change in this patch is that I removed some new instances of `#if ENABLE(RESIZE_OBSERVER)` that were added yesterday.
Comment 24 Fujii Hironori 2021-09-16 13:00:57 PDT 
Comment on attachment 438381 [details]
Remove ENABLE(RESIZE_OBSERVER)

r+ flag can be set only by reviewers. The commit queue will complain.
Nobody can set r+ flag for their own patch.
This patch has already got r+ by me, and you put the reviewers name in the ChangeLog entry. You don't need to set r+ flag. All this patch need is cq?.
I will set cq+ after EWS become all green.
Comment 25 Philip Chimento 2021-09-16 15:49:05 PDT 
Created attachment 438409 [details]
Various changes for 'build-webkit --minimal'
Comment 26 EWS 2021-09-16 19:29:22 PDT 
Committed r282628 (241785@main): <https://commits.webkit.org/241785@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438381 [details].
Comment 27 Philip Chimento 2021-09-17 10:04:48 PDT 
Reopening for review of the last patch.
Comment 28 Brent Fulgham 2022-02-09 16:04:50 PST 
It looks like this got lost in the shuffle -- could you upload a revised patch that applies against the current tree so we can land it and close this bug?
Comment 29 Philip Chimento 2022-02-11 13:42:53 PST 
Created attachment 451741 [details]
Patch
Comment 30 Philip Chimento 2022-02-11 13:52:02 PST 
Created attachment 451742 [details]
Patch
Comment 31 Don Olmstead 2022-02-11 15:23:46 PST 
Comment on attachment 451742 [details]
Patch

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

r=me with nits. Honestly you might want to do a non-unified with the minimal build as well.

> Source/WebCore/html/ColorInputType.cpp:42
> +#include "ElementChildIterator.h"

This still valid? I don't see that referenced in this cpp?

> Source/WebCore/inspector/agents/InspectorCanvasAgent.h:29
> +#include "Frame.h"

There's already a forward declaration of Frame here.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:37
> +#include "SharedBuffer.h"

This looks like it can be forward declared.
Comment 32 Philip Chimento 2022-02-11 20:45:10 PST 
Thanks for the review, I'll test building this again without those lines after the weekend.
Comment 33 Philip Chimento 2022-02-15 09:47:12 PST 
(In reply to Don Olmstead from comment #31)
> > Source/WebCore/html/ColorInputType.cpp:42
> > +#include "ElementChildIterator.h"
> 
> This still valid? I don't see that referenced in this cpp?

childrenOfType<...> is used in several places, so this is still valid.

> > Source/WebCore/inspector/agents/InspectorCanvasAgent.h:29
> > +#include "Frame.h"
> 
> There's already a forward declaration of Frame here.
> 
> > Source/WebCore/inspector/agents/InspectorNetworkAgent.h:37
> > +#include "SharedBuffer.h"
> 
> This looks like it can be forward declared.

Both of these removed, they don't seem to be necessary anymore. I also had to add another missing include of <wtf/text/WTFString.h> elsewhere.

This does not quite fix --minimal altogether. I still have to switch on ENABLE_CONTEXT_MENUS, ENABLE_POINTER_LOCK, ENABLE_MHTML, ENABLE_CONTENT_EXTENSIONS, ENABLE_INTELLIGENT_TRACKING_PREVENTION, and ENABLE_INPUT_TYPE_COLOR because there is a lot of code in the GLib portions that assumes these will be enabled. If I were to add guards around those, they would quickly overwhelm everything else in this patch, but I'm prepared to leave that for another patch if that's OK.
Comment 34 Philip Chimento 2022-02-15 09:49:20 PST 
Created attachment 452042 [details]
Patch
Comment 35 Don Olmstead 2022-02-15 09:56:14 PST 
(In reply to Philip Chimento from comment #33)
> This does not quite fix --minimal altogether. I still have to switch on
> ENABLE_CONTEXT_MENUS, ENABLE_POINTER_LOCK, ENABLE_MHTML,
> ENABLE_CONTENT_EXTENSIONS, ENABLE_INTELLIGENT_TRACKING_PREVENTION, and
> ENABLE_INPUT_TYPE_COLOR because there is a lot of code in the GLib portions
> that assumes these will be enabled. If I were to add guards around those,
> they would quickly overwhelm everything else in this patch, but I'm prepared
> to leave that for another patch if that's OK.

That's fine.

In the future if you get an r+ you can just replace the NOBODY (OOPS) in the Reviewed by line with the reviewers name and then just set cq? or cq+ it on your own when EWS comes back.
Comment 36 Philip Chimento 2022-02-15 10:04:02 PST 
(In reply to Don Olmstead from comment #35)
> In the future if you get an r+ you can just replace the NOBODY (OOPS) in the
> Reviewed by line with the reviewers name and then just set cq? or cq+ it on
> your own when EWS comes back.

Thanks and sorry if I pinged you unnecessarily! I thought I had replaced the NOBODY but looking at the arguments of `webkit-patch upload`, maybe I also needed to specify --no-review? I'll try that next time.
Comment 37 EWS 2022-02-15 16:05:39 PST 
Committed r289860 (247301@main): <https://commits.webkit.org/247301@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452042 [details].