[webkit-reviews] review denied: [Bug 233988] Add ability to inject messages into webpushd : [Attachment 446372] Patch v2

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 233988: Add ability to inject messages into webpushd
https://bugs.webkit.org/show_bug.cgi?id=233988

Attachment 446372: Patch v2

https://bugs.webkit.org/attachment.cgi?id=446372&action=review




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 446372
  --> https://bugs.webkit.org/attachment.cgi?id=446372
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.