Bug 229780 – Fixes for build-webkit --minimal

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Philip Chimento]

Fixes for build-webkit --minimal

[Philip Chimento]

Created attachment 437088 [details]
Patch

[Philip Chimento]

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.

[Philip Chimento]

Created attachment 437091 [details]
Patch

[EWS Watchlist]

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

[Fujii Hironori]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/82898564>

[Philip Chimento]

Created attachment 438287 [details]
Remove ENABLE(INTERSECTION_OBSERVER)

[Philip Chimento]

Created attachment 438288 [details]
Remove ENABLE(RESIZE_OBSERVER)

[Philip Chimento]

Created attachment 438289 [details]
Remove features that no longer exist

[Philip Chimento]

Created attachment 438290 [details]
Various changes for 'build-webkit --minimal'

[Philip Chimento]

Done! (I guess the ENABLE(RESIZE_OBSERVER) patch will not apply cleanly until the ENABLE(INTERSECTION_OBSERVER) patch lands.)

[Fujii Hironori]

Comment on attachment 438287 [details]
Remove ENABLE(INTERSECTION_OBSERVER)

LGTM if the EWS won't complain.

[Fujii Hironori]

Comment on attachment 438288 [details]
Remove ENABLE(RESIZE_OBSERVER)

LGTM if the EWS won't complain.

[Philip Chimento]

Created attachment 438305 [details]
Remove ENABLE(INTERSECTION_OBSERVER)

[Philip Chimento]

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

[EWS]

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].

[Philip Chimento]

Reopened, still 3 to land :-)

[EWS]

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].

[Philip Chimento]

Reopening to attach new patch.

[Philip Chimento]

Created attachment 438373 [details]
Remove ENABLE(RESIZE_OBSERVER)

[Philip Chimento]

Comment on attachment 438373 [details]
Remove ENABLE(RESIZE_OBSERVER)

Carrying forward r+

[Philip Chimento]

Created attachment 438381 [details]
Remove ENABLE(RESIZE_OBSERVER)

[Philip Chimento]

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.

[Fujii Hironori]

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.

[Philip Chimento]

Created attachment 438409 [details]
Various changes for 'build-webkit --minimal'

[EWS]

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].

[Philip Chimento]

Reopening for review of the last patch.

[Brent Fulgham]

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?

[Philip Chimento]

Created attachment 451741 [details]
Patch

[Philip Chimento]

Created attachment 451742 [details]
Patch

[Don Olmstead]

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.

[Philip Chimento]

Thanks for the review, I'll test building this again without those lines after the weekend.

[Philip Chimento]

(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.

[Philip Chimento]

Created attachment 452042 [details]
Patch

[Don Olmstead]

(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.

[Philip Chimento]

(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.

[EWS]

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].