Bug 233714 - Add an InstallCoordination webpushd backend
Summary: Add an InstallCoordination webpushd backend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-01 11:07 PST by Brady Eidson
Modified: 2022-02-18 11:02 PST (History)
8 users (show)

See Also:

Attachments
EWS v1 (41.88 KB, patch)
2021-12-01 14:00 PST, Brady Eidson 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
EWS v2 (42.14 KB, patch)
2021-12-01 16:15 PST, Brady Eidson 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
EWS v3 (43.13 KB, patch)
2021-12-01 17:05 PST, Brady Eidson 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v1 (47.83 KB, patch)
2021-12-01 17:51 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch v2 (47.85 KB, patch)
2021-12-01 18:02 PST, Brady Eidson 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v3 (49.34 KB, patch)
2021-12-01 20:11 PST, Brady Eidson 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v4 (47.62 KB, patch)
2021-12-01 20:43 PST, Brady Eidson 		achristensen: review+
Details | Formatted Diff | Diff
PFL (47.67 KB, patch)
2021-12-02 12:21 PST, Brady Eidson 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Revived for EWS + landing (51.79 KB, patch)
2022-02-17 19:33 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
Revived for EWS + landing v2 (51.79 KB, patch)
2022-02-17 19:34 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
Show Obsolete (9) 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 Brady Eidson 2021-12-01 11:07:24 PST 
Add an InstallCoordination webpushd backend
Comment 1 Brady Eidson 2021-12-01 14:00:25 PST 
Created attachment 445613 [details]
EWS v1
Comment 2 Tim Horton 2021-12-01 14:13:15 PST 
Comment on attachment 445613 [details]
EWS v1

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

> Source/WebKit/webpushd/AppBundleRequest.mm:56
> +#if PLATFORM(IOS)

Please give yourself an ENABLE for all of these

> Source/WebKit/webpushd/ICAppBundle.mm:190
> +    LSApplicationRecord *appRecord = [[LSApplicationRecord alloc] initWithBundleIdentifier:(NSString *)getBundleIdentifier() allowPlaceholder:YES error:&error];

This leaks
Comment 3 Brady Eidson 2021-12-01 16:15:05 PST 
Created attachment 445631 [details]
EWS v2
Comment 4 Brady Eidson 2021-12-01 17:05:01 PST 
Created attachment 445639 [details]
EWS v3
Comment 5 Brady Eidson 2021-12-01 17:51:15 PST 
Created attachment 445648 [details]
Patch v1
Comment 6 Brady Eidson 2021-12-01 18:02:06 PST 
Created attachment 445649 [details]
Patch v2
Comment 7 Brady Eidson 2021-12-01 20:11:34 PST 
Created attachment 445661 [details]
Patch v3
Comment 8 Brady Eidson 2021-12-01 20:43:18 PST 
Created attachment 445666 [details]
Patch v4
Comment 9 Alex Christensen 2021-12-02 11:37:00 PST 
Comment on attachment 445666 [details]
Patch v4

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

> Source/WebKit/webpushd/ICAppBundle.mm:137
> +    appDigest->addBytes(utf8Identifier.data(), utf8Identifier.length());

Do we want to add a salt?

> Source/WebKit/webpushd/ICAppBundle.mm:295
> +    dataPromise.get().percentComplete = 1.0;

Should this be 100?

> Source/WebKit/webpushd/ICAppBundle.mm:313
> +void ICAppBundle::bundleCreationFailed(NSError *error)

Why error parameter?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:333
> +#if PLATFORM(IOS)

ENABLE(INSTALL_COORDINATION_BUNDLES)
Comment 10 Brady Eidson 2021-12-02 12:21:38 PST 
(In reply to Alex Christensen from comment #9)
> Comment on attachment 445666 [details]
> Patch v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445666&action=review
> 
> > Source/WebKit/webpushd/ICAppBundle.mm:137
> > +    appDigest->addBytes(utf8Identifier.data(), utf8Identifier.length());
> 
> Do we want to add a salt?

Salt discussion happening out of band, can be added later if needed.

> 
> > Source/WebKit/webpushd/ICAppBundle.mm:295
> > +    dataPromise.get().percentComplete = 1.0;
> 
> Should this be 100?

Misnamed SPI - It's actually 0.0 to 1.0 despite the "percent" name

> 
> > Source/WebKit/webpushd/ICAppBundle.mm:313
> > +void ICAppBundle::bundleCreationFailed(NSError *error)
> 
> Why error parameter?

TBD

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:333
> > +#if PLATFORM(IOS)
> 
> ENABLE(INSTALL_COORDINATION_BUNDLES)

Changed.
Comment 11 Brady Eidson 2021-12-02 12:21:52 PST 
Created attachment 445759 [details]
PFL
Comment 12 EWS 2021-12-02 14:23:38 PST 
ChangeLog entry in Source/WebCore/PAL/ChangeLog contains OOPS!.
Comment 13 Radar WebKit Bug Importer 2021-12-08 11:08:23 PST 
<rdar://problem/86220489>
Comment 14 Brady Eidson 2022-02-17 19:33:04 PST 
Created attachment 452470 [details]
Revived for EWS + landing
Comment 15 Brady Eidson 2022-02-17 19:34:35 PST 
Created attachment 452471 [details]
Revived for EWS + landing v2
Comment 16 EWS 2022-02-18 11:02:39 PST 
Committed r290146 (247485@main): <https://commits.webkit.org/247485@main>

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