[EWS] Support PRs in UploadTestResults and ExtractTestResults by JonWBedard · Pull Request #137 · WebKit/WebKit

JonWBedard · 2022-02-10T16:39:25Z

`5d7eeaf`

[EWS] Support PRs in UploadTestResults and ExtractTestResults
https://bugs.webkit.org/show_bug.cgi?id=236446
<rdar://problem/88756592 >

Reviewed by Aakash Jain.

* CISupport/ews-build/steps.py:
(ConfigureBuild.add_pr_details): Shorten hash to 8 characters.
(UploadTestResults.__init__): Use change_id instead of patch_id.
(ExtractTestResults.__init__): Ditto.
* CISupport/ews-build/steps_unittest.py:


Canonical link: https://commits.webkit.org/247136@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@289625 268f45cc-cd09-0410-ab3c-d52691b4dbfc

aj062 · 2022-02-10T18:31:52Z

Sample runs please.

aj062 · 2022-02-10T18:33:40Z

should have some test-case for git-hash as well

aj062

There would be an extra r before the hash in the url. It might be good to remove that for hashes, as it mixes with the hash while reading. Although it's not completely accurate to include that r before patch_id as well.
Also, not sure if you want to trim the hash it to say 8 or 10 characters, in order to limit the length of the url.

aj062 · 2022-02-10T19:00:33Z

this patch_id also needs to be updated.

aj062 · 2022-02-11T13:18:19Z

Does this affect other places we are using revision? Please verify affect on those places.

If you look at the line above, this is only ever going to be a hash. It will also effect our build url, but that's sort of the point, our build download URL should also get this.

The builds I just linked are full a full build + test.

I meant change_id.
I just checked and it affects UploadBuiltProduct, TransferToS3 and DownloadBuiltProduct. In those steps the url is constructed only based on this hash. So chances of collision are higher. Although we store the builds only for 14 days, so the time-duration reduces the chances of collision. I'll let you decide if you actually want to use 8 characters git hash in those steps.

We should be fine, we get about a 1% change of collision with 10,000 commits at 8 characters, and we have ~600 commits every 2 weeks. Figuring 10 iterations per commit, thats 6,000 hashes every 2 weeks, which is .5% chance of collision. Back-of-the-napkin calculation of .5% chance every 2 weeks means about a single collision every 5 years.

aj062 · 2022-02-11T13:19:50Z

I wonder if removing this r will break anything for patch workflow. Can you please test that on uat instance.

I don't see how it possibly could, we don't have public_html/results/r anywhere in Tools. But we can send a patch through for good measure.

JonWBedard · 2022-02-11T14:36:41Z

Some linked examples:
https://ews-build.webkit-uat.org/#/builders/81/builds/8
https://ews-build.webkit-uat.org/#/builders/84/builds/8

The builder:
https://ews-build.webkit-uat.org/#/builders/45/builds/94

aj062

Looks good. Two things:

please verify that it doesn't break patch workflow by doing a test build on uat instance
decide about the hash length to use

JonWBedard · 2022-02-11T16:44:15Z

Looks good. Two things:

please verify that it doesn't break patch workflow by doing a test build on uat instance

Here is a patch workflow example: https://ews-build.webkit-uat.org/#/builders/84/builds/9

decide about the hash length to use

I'm going with 8 at the moment, since that's what we're doing with the display length too. We can easily change it going forward if we have issues with collisions.

JonWBedard · 2022-02-11T17:24:02Z

Landed https://commits.webkit.org/247136@main (5d7eeaf)!

…ent (WebKit#137) Opening a message terminated Mail with an uncaught NSInvalidArgumentException: -[WKBrowsingContextGroup removeAllUserStyleSheets] unrecognized selector, sent from -[MUIWebDocumentViewGroup _refreshUserStyleSheet] while building the message view. Mail also drives _refreshUserScripts (addUserScript:/removeAll). The page-group user-content model was removed upstream: WebPageGroup lost its user content controller and the WKPageGroupAdd*/RemoveAll* C SPI was gutted to no-ops, while the legacy WKBrowsingContextGroup ObjC class (which Mail and QuickLook drive) only had addUserStyleSheet: as an inert no-op and was missing removeAllUserStyleSheets / addUserScript: / removeAllUserScripts entirely. Restore the path faithfully (matches the pre-removal implementation): - WebPageGroup again owns a WebUserContentControllerProxy (userContentController()). - The WKPageGroup user-content C SPI builds API::UserStyleSheet/API::UserScript and adds them to that controller (adapted to current WebCore ctor signatures). - WKView seeds the page configuration's user content controller from the page group, so pages created in the group share it and the content is injected. - WKBrowsingContextGroup forwards all four user-content methods to the C SPI. Verified on the 10.9 VM: Mail no longer crashes opening/selecting a message (survives single- and double-click; no crash reports, no unrecognized selector). Full message-body rendering additionally needs the WKView auto-layout / intrinsic-content-size SPI (MUIWKView), restored separately. Co-Authored-By: Claude Opus 4.8 <[email protected]>

) Apple Mail's WebContent injected bundle (MailUIWebBundle) talks to the app over the legacy WKConnection bundle<->app channel, which had been removed upstream before this backport's base. Reconstruct that subsystem so message bodies flow to the UI process and images load: - WKConnection / WKConnectionInternal / WKConnection.mm: the WKConnection class + WKConnectionDelegate protocol, with a recursive ObjCObjectGraph body serializer (String/Number/Data/Array/Dictionary) and a controller round-trip keyed by WebPageProxyIdentifier (shared across processes). - WKProcessGroup: WKConnection delegate plumbing (processGroup:didCreateConnectionToWebProcessPlugIn:). - WKWebProcessPlugIn: lazily vend a WKConnection backed by WKBundlePostMessage and register the browser-context controller under the page-proxy id. - WKBrowsingContextController: restore activeURL/provisionalURL/committedURL/ title/estimatedProgress over the live WKPageCopy* C SPI; route loadData:...userData: through WKPageLoadDataWithUserData so the userData (which carries the image-loading context) is no longer dropped (WebKit#142). - WKTypeRefWrapper: restore the legacy ObjC wrapper class the bundle dlopens. - WKPage: in PolicyClient::decidePolicyForNavigationAction, let legacy V0/V1 clients' own client/user-input loads through without a redundant policy round-trip they don't expect (matches Safari-7-era WebKit2 behavior). - GPUCanvasContextCocoa / MavericksBackportWebCoreGlue: supply WebCore symbols whose normal impl TU is excluded from the 10.9 build, so the eagerly-bound injected bundle resolves at load time (isRegexpMatching via NSRegularExpression; GPUCanvasContext::create returns nullptr as upstream does off-Cocoa). Co-Authored-By: Claude Opus 4.8 <[email protected]>

…lip, datastore guess, shadow singleton, visibility latch, drag-Copy fallback adversarial-hack-reviewer APPROVED (all premises retested; upstream restored). - MediaPlayerPrivateAVFoundationObjC.mm: fabricated presentationSize fallback (naturalSize probe + 400x300 last resort) removed; upstream player->presentationSize() restored. Verified: <video> renders frames at its natural size with controls. - BorderPainter.cpp: unconditional border-side clip ("#25-adjacent CG fill drop") removed; byte-identical upstream shouldClip restored. Verified: button test page renders every border edge complete (the real #25 fix lives in PathCG.cpp, untouched). - WebProcessProxy.cpp getNetworkProcessConnection: silent guessed- WebsiteDataStore bind removed; byte-identical upstream ASSERT_NOT_REACHED + RELEASE_LOG_FAULT + reply({}) restored (the "silently bound" log never fired). - WebKitLegacy WebView.mm: WK109FallbackHistoryItemClient fastMalloc shadow singleton removed; LegacyHistoryItemClient::singleton() restored at both page-config call sites (TZone-null premise falsified). - WebViewImpl.mm: E10 force-rootLayer/sublayer-frames block removed (proven non-load-bearing: restoring it did NOT fix the inspector blank, and with the real fix in place everything renders without it); sendDragEndToPage restored byte-identical to upstream (view-converted client location). - WKView.mm + MinimalPageClient.mm: the force-visible-when-windowless latch REMOVED in full (init-frame-size heuristic, extern, setter, member). A windowless view now reports truthful state — not visible, not focused, not active, not in window, visually idle. The latch was the SIXTH dead compensation of the visibility-latch family: with the 2026-07-16 keystone, Safari's windowless Top Sites snapshot fetcher loads, lays out, and snapshots fine under truthful state (verified: wiped Webpage Previews, fetcher captured a full fresh set, all 12 grid tiles repopulated with real renders). - WKPage.cpp + PageClient.h + MinimalPageClient.mm: the isOffscreenRenderClient offscreen-bypass branch/virtual/override removed — it duplicated the general WebKit#137 client-request-without-gesture bypass directly below it (which covers the fetcher's WKPageLoadURL loads) and would have overridden the legacy V0 ignore() contract for content-initiated navigations. Snapshot capture re-verified through the WebKit#137 path alone. - WKView.mm draggingUpdated: now mirrors upstream WebViewImpl — returns the page's current drag operation, None while the async reply is pending (the Copy-while-pending fallback and its comment removed). - WKView.mm incidental fixes: _wkState null-guards on the three Services responder methods; initWithFrame:configurationRef: stub now releases self before returning nil; the false "copy/cut shield against navigation" comment removed (the forwarding itself is legit and mirrors WebViewImpl WEBCORE_COMMAND; verified Cmd+A/Cmd+C does not navigate). - MinimalPageClient.mm didRelaunchProcess: E12 drawing-area size re-seed removed (relaunch-loop premise dead; Top Sites verified without it). Co-Authored-By: Claude Fable 5 <[email protected]>

…ent (WebKit#137) Opening a message terminated Mail with an uncaught NSInvalidArgumentException: -[WKBrowsingContextGroup removeAllUserStyleSheets] unrecognized selector, sent from -[MUIWebDocumentViewGroup _refreshUserStyleSheet] while building the message view. Mail also drives _refreshUserScripts (addUserScript:/removeAll). The page-group user-content model was removed upstream: WebPageGroup lost its user content controller and the WKPageGroupAdd*/RemoveAll* C SPI was gutted to no-ops, while the legacy WKBrowsingContextGroup ObjC class (which Mail and QuickLook drive) only had addUserStyleSheet: as an inert no-op and was missing removeAllUserStyleSheets / addUserScript: / removeAllUserScripts entirely. Restore the path faithfully (matches the pre-removal implementation): - WebPageGroup again owns a WebUserContentControllerProxy (userContentController()). - The WKPageGroup user-content C SPI builds API::UserStyleSheet/API::UserScript and adds them to that controller (adapted to current WebCore ctor signatures). - WKView seeds the page configuration's user content controller from the page group, so pages created in the group share it and the content is injected. - WKBrowsingContextGroup forwards all four user-content methods to the C SPI. Verified on the 10.9 VM: Mail no longer crashes opening/selecting a message (survives single- and double-click; no crash reports, no unrecognized selector). Full message-body rendering additionally needs the WKView auto-layout / intrinsic-content-size SPI (MUIWKView), restored separately. Co-Authored-By: Claude Opus 4.8 <[email protected]>

) Apple Mail's WebContent injected bundle (MailUIWebBundle) talks to the app over the legacy WKConnection bundle<->app channel, which had been removed upstream before this backport's base. Reconstruct that subsystem so message bodies flow to the UI process and images load: - WKConnection / WKConnectionInternal / WKConnection.mm: the WKConnection class + WKConnectionDelegate protocol, with a recursive ObjCObjectGraph body serializer (String/Number/Data/Array/Dictionary) and a controller round-trip keyed by WebPageProxyIdentifier (shared across processes). - WKProcessGroup: WKConnection delegate plumbing (processGroup:didCreateConnectionToWebProcessPlugIn:). - WKWebProcessPlugIn: lazily vend a WKConnection backed by WKBundlePostMessage and register the browser-context controller under the page-proxy id. - WKBrowsingContextController: restore activeURL/provisionalURL/committedURL/ title/estimatedProgress over the live WKPageCopy* C SPI; route loadData:...userData: through WKPageLoadDataWithUserData so the userData (which carries the image-loading context) is no longer dropped (WebKit#142). - WKTypeRefWrapper: restore the legacy ObjC wrapper class the bundle dlopens. - WKPage: in PolicyClient::decidePolicyForNavigationAction, let legacy V0/V1 clients' own client/user-input loads through without a redundant policy round-trip they don't expect (matches Safari-7-era WebKit2 behavior). - GPUCanvasContextCocoa / MavericksBackportWebCoreGlue: supply WebCore symbols whose normal impl TU is excluded from the 10.9 build, so the eagerly-bound injected bundle resolves at load time (isRegexpMatching via NSRegularExpression; GPUCanvasContext::create returns nullptr as upstream does off-Cocoa). Co-Authored-By: Claude Opus 4.8 <[email protected]>

…lip, datastore guess, shadow singleton, visibility latch, drag-Copy fallback adversarial-hack-reviewer APPROVED (all premises retested; upstream restored). - MediaPlayerPrivateAVFoundationObjC.mm: fabricated presentationSize fallback (naturalSize probe + 400x300 last resort) removed; upstream player->presentationSize() restored. Verified: <video> renders frames at its natural size with controls. - BorderPainter.cpp: unconditional border-side clip ("#25-adjacent CG fill drop") removed; byte-identical upstream shouldClip restored. Verified: button test page renders every border edge complete (the real #25 fix lives in PathCG.cpp, untouched). - WebProcessProxy.cpp getNetworkProcessConnection: silent guessed- WebsiteDataStore bind removed; byte-identical upstream ASSERT_NOT_REACHED + RELEASE_LOG_FAULT + reply({}) restored (the "silently bound" log never fired). - WebKitLegacy WebView.mm: WK109FallbackHistoryItemClient fastMalloc shadow singleton removed; LegacyHistoryItemClient::singleton() restored at both page-config call sites (TZone-null premise falsified). - WebViewImpl.mm: E10 force-rootLayer/sublayer-frames block removed (proven non-load-bearing: restoring it did NOT fix the inspector blank, and with the real fix in place everything renders without it); sendDragEndToPage restored byte-identical to upstream (view-converted client location). - WKView.mm + MinimalPageClient.mm: the force-visible-when-windowless latch REMOVED in full (init-frame-size heuristic, extern, setter, member). A windowless view now reports truthful state — not visible, not focused, not active, not in window, visually idle. The latch was the SIXTH dead compensation of the visibility-latch family: with the 2026-07-16 keystone, Safari's windowless Top Sites snapshot fetcher loads, lays out, and snapshots fine under truthful state (verified: wiped Webpage Previews, fetcher captured a full fresh set, all 12 grid tiles repopulated with real renders). - WKPage.cpp + PageClient.h + MinimalPageClient.mm: the isOffscreenRenderClient offscreen-bypass branch/virtual/override removed — it duplicated the general WebKit#137 client-request-without-gesture bypass directly below it (which covers the fetcher's WKPageLoadURL loads) and would have overridden the legacy V0 ignore() contract for content-initiated navigations. Snapshot capture re-verified through the WebKit#137 path alone. - WKView.mm draggingUpdated: now mirrors upstream WebViewImpl — returns the page's current drag operation, None while the async reply is pending (the Copy-while-pending fallback and its comment removed). - WKView.mm incidental fixes: _wkState null-guards on the three Services responder methods; initWithFrame:configurationRef: stub now releases self before returning nil; the false "copy/cut shield against navigation" comment removed (the forwarding itself is legit and mirrors WebViewImpl WEBCORE_COMMAND; verified Cmd+A/Cmd+C does not navigate). - MinimalPageClient.mm didRelaunchProcess: E12 drawing-area size re-seed removed (relaunch-loop premise dead; Top Sites verified without it). Co-Authored-By: Claude Fable 5 <[email protected]>

JonWBedard self-assigned this Feb 10, 2022

JonWBedard force-pushed the eng/github-ews-pr-test-result branch from 3092419 to 8f75a5f Compare February 10, 2022 18:17

JonWBedard changed the base branch from safari-613-branch to main February 10, 2022 18:17

JonWBedard requested a review from aj062 February 10, 2022 18:18

aj062 reviewed Feb 10, 2022

View reviewed changes

Comment thread Tools/CISupport/ews-build/steps_unittest.py Outdated

aj062 Feb 10, 2022

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should have some test-case for git-hash as well

aj062 reviewed Feb 10, 2022

View reviewed changes

Comment thread Tools/CISupport/ews-build/steps.py Outdated

aj062 Feb 10, 2022

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this patch_id also needs to be updated.

JonWBedard mentioned this pull request Feb 10, 2022

[EWS] Rebase PRs on tip of branch #135

Merged

JonWBedard force-pushed the eng/github-ews-pr-test-result branch from 8f75a5f to 7d06b5c Compare February 11, 2022 00:48

aj062 reviewed Feb 11, 2022

View reviewed changes

aj062 approved these changes Feb 11, 2022

View reviewed changes

JonWBedard mentioned this pull request Feb 11, 2022

[frame-rate] [custom-effect] allow setting frameRate as an option passed to document.timeline.animate() #128

Merged

JonWBedard closed this Feb 11, 2022

JonWBedard force-pushed the eng/github-ews-pr-test-result branch from 7d06b5c to 5d7eeaf Compare February 11, 2022 17:23

JonWBedard merged commit 5d7eeaf into WebKit:main Feb 11, 2022

JonWBedard deleted the eng/github-ews-pr-test-result branch February 11, 2022 17:24

JonWBedard added Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases WebKit Nightly Build labels Mar 29, 2022

Uh oh!

Conversation

JonWBedard commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aj062 commented Feb 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aj062 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aj062 Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonWBedard commented Feb 11, 2022

Uh oh!

aj062 left a comment

Choose a reason for hiding this comment

Uh oh!

JonWBedard commented Feb 11, 2022

Uh oh!

JonWBedard commented Feb 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JonWBedard commented Feb 10, 2022 •

edited

Loading

aj062 Feb 11, 2022 •

edited

Loading