Bug 232318 – Possible non-unified build issues

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Kolesa]

Since the recent refactors regarding moves to standard std:: containers as well as the JSC ArrayBuffer refactoring, there have been issues with forward declarations in Mac builds (both 10.15 Intel and 11 arm64) in the Playwright project. My current suspicion is that the patches in Playwright change the way builds are unified, which exposes missing includes, but have no good way to explicitly verify that. Therefore, I will raise a patch here that contains the includes that need to be added to make that build again, to get comments.

[Daniel Kolesa]

Created attachment 442507 [details]
possible patch

[Daniel Kolesa]

Created attachment 442508 [details]
fixed patch

apologies, somehow i submitted a malformed patch

[Fujii Hironori]

Comment on attachment 442508 [details]
fixed patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.h:34
> +#include <variant>

style checker EWS is failing.

ERROR: Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.h:34:  Alphabetical sorting problem.  [build/include_order] [4]

[Fujii Hironori]

(In reply to Daniel Kolesa from comment #0)
> but have no good way to explicitly verify that. 

There is a experimental CMake build of Mac port, and CMake has ENABLE_UNIFIED_BUILDS option.
However, this patch looks no problem to me.

[Daniel Kolesa]

Created attachment 442688 [details]
fixed style (no functional changes)

well then, thanks

fixed style

[Don Olmstead]

Comment on attachment 442688 [details]
fixed style (no functional changes)

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

I'll just r+ this on Fujii's behalf but in the future that's the workflow when you get an r+ but a cq-.

> Source/WebKit/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

When you get an r+ in a previous patch with nits what you do is you replace the `NOBODY (OOPS!)` and then a cq+ is all that's needed.

[Don Olmstead]

(In reply to Don Olmstead from comment #6)
> Comment on attachment 442688 [details]
> fixed style (no functional changes)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442688&action=review
> 
> I'll just r+ this on Fujii's behalf but in the future that's the workflow
> when you get an r+ but a cq-.
> 
> > Source/WebKit/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> When you get an r+ in a previous patch with nits what you do is you replace
> the `NOBODY (OOPS!)` and then a cq+ is all that's needed.

In this case the line would've been

Reviewed by Fujii Hironori.

You can just search the ChangeLog and copy paste the reviewer's name from there.

[EWS]

Committed r284990 (243636@main): <https://commits.webkit.org/243636@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/84762460>

[Daniel Kolesa]

> I'll just r+ this on Fujii's behalf but in the future that's the workflow
> when you get an r+ but a cq-.

noted, thanks