| Summary: | Fixes for build-webkit --minimal | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Philip Chimento <philip.chimento> | ||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, andresg_22, annulen, apinheiro, benjamin, berto, bfulgham, cdumez, cfleizach, cgarcia, changseok, cmarcelo, dmazzoni, don.olmstead, esprehn+autocc, ews-watchlist, fred.wang, glenn, gustavo, gyuyoung.kim, hi, Hironori.Fujii, japhet, jbedard, jcraig, jdiggs, joepeck, kangil.han, keith_miller, kondapallykalyan, macpherson, mark.lam, menard, mifenton, msaboff, pangle, pdr, ryuan.choi, saam, samuel_white, sergio, simon.fraser, tzagallo, webkit-bug-importer | ||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Philip Chimento
2021-09-01 16:49:41 PDT
Created attachment 437088 [details]
Patch
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. Created attachment 437091 [details]
Patch
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 (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. Created attachment 438287 [details]
Remove ENABLE(INTERSECTION_OBSERVER)
Created attachment 438288 [details]
Remove ENABLE(RESIZE_OBSERVER)
Created attachment 438289 [details]
Remove features that no longer exist
Created attachment 438290 [details]
Various changes for 'build-webkit --minimal'
Done! (I guess the ENABLE(RESIZE_OBSERVER) patch will not apply cleanly until the ENABLE(INTERSECTION_OBSERVER) patch lands.) Comment on attachment 438287 [details]
Remove ENABLE(INTERSECTION_OBSERVER)
LGTM if the EWS won't complain.
Comment on attachment 438288 [details]
Remove ENABLE(RESIZE_OBSERVER)
LGTM if the EWS won't complain.
Created attachment 438305 [details]
Remove ENABLE(INTERSECTION_OBSERVER)
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
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]. Reopened, still 3 to land :-) 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]. Reopening to attach new patch. Created attachment 438373 [details]
Remove ENABLE(RESIZE_OBSERVER)
Comment on attachment 438373 [details]
Remove ENABLE(RESIZE_OBSERVER)
Carrying forward r+
Created attachment 438381 [details]
Remove ENABLE(RESIZE_OBSERVER)
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 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.
Created attachment 438409 [details]
Various changes for 'build-webkit --minimal'
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]. Reopening for review of the last patch. 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? Created attachment 451741 [details]
Patch
Created attachment 451742 [details]
Patch
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. Thanks for the review, I'll test building this again without those lines after the weekend. (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. Created attachment 452042 [details]
Patch
(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. (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. 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]. |