Summary: | Add ability to inject messages into webpushd | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Brady Eidson <beidson> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, ews-watchlist, hi, mkwst, thorton, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 234081 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Brady Eidson
2021-12-07 23:47:55 PST
Created attachment 446318 [details]
Patch v1
Created attachment 446372 [details]
Patch v2
Comment on attachment 446372 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=446372&action=review > Source/WebKit/Configurations/webpushtool.xcconfig:32 > +OTHER_LDFLAGS = -framework WebKit -l WTF you shouldn't need to link WTF. Link JavaScriptCore instead. > Source/WebKit/Shared/WebPushMessage.h:2 > + * Copyright (C) 2010-2020 Apple Inc. All rights reserved. 2021 > Source/WebKit/WebKit.xcodeproj/project.pbxproj:6486 > + 517B5F9E275F3F54002DC22D /* libicucore.tbd in Frameworks */, nope > Source/WebKit/WebKit.xcodeproj/project.pbxproj:9715 > + 517B5F9D275F3F54002DC22D /* libicucore.tbd */, nope > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:230 > + result[1] = (message.length() & 0x000000000000FF00) >> 8; the bitwise and assumes 64-bit size_t, and it's not necessary. > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:235 > + result[5 + i] = message[i]; This truncates 256-65535. not great.
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:235
> > + result[5 + i] = message[i];
>
> This truncates 256-65535. not great.
It didn't, because we're only encoding 8-bit strings.
But I made that more explicit.
Created attachment 446405 [details]
Patch v3
Comment on attachment 446405 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=446405&action=review > Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:94 > + CompletionHandler<void(Vector<WebPushMessage>&&)> replyHandler = [completionHandler = WTFMove(completionHandler)] (Vector<WebPushMessage> messages) mutable { This looks like an unneeded Vector copy. Can we just pass a Vector&& and avoid this wrapper too? > Source/WebKit/Shared/Cocoa/WebPushMessageCocoa.mm:2 > + * Copyright (C) 2010-2020 Apple Inc. All rights reserved. 2021 > Source/WebKit/Shared/Cocoa/WebPushMessageCocoa.mm:47 > + Vector<uint8_t> { (const uint8_t*)pushData.bytes, pushData.length }, static_cast > Source/WebKit/Shared/Cocoa/WebPushMessageCocoa.mm:48 > + URL { (NSURL *)url } unnecessary cast > Source/WebKit/webpushd/PushClientConnection.mm:126 > + messageIdentifier = makeString("[", signingIdentifer, " (0x", hex(reinterpret_cast<uint64_t>(m_xpcConnection.get()), WTF::HexConversionMode::Lowercase), ")] "); nit: I think "[" can be '[' > Source/WebKit/webpushd/WebPushDaemon.mm:369 > + resultMessages.append(WebKit::WebPushMessage { Vector<uint8_t> { reinterpret_cast<const uint8_t*>(data.data()), data.length() }, message.registrationURL }); nit: I wonder if you could use a Span instead of a Vector here because all you are doing is sending it. > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:235 > + >(utf8.length() >> 24); line break is probably a typo Created attachment 446424 [details]
PFL
Committed r286764 (245005@main): <https://commits.webkit.org/245005@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446424 [details]. Re-opened since this is blocked by bug 234081 Created attachment 446561 [details]
New PFL
webpushtool didn't have any dependencies so it was building out of order with WebKit.framework itself.
That's why incremental builds worked, but not clean builds.
Committed r286788 (245029@main): <https://commits.webkit.org/245029@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446561 [details]. |