[webkit-reviews] review granted: [Bug 239042] Handle public token updates in webpushd : [Attachment 457548] Patch feedback

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has granted Ben Nham <nham at apple.com>'s
request for review:
Bug 239042: Handle public token updates in webpushd
https://bugs.webkit.org/show_bug.cgi?id=239042

Attachment 457548: Patch feedback

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




--- Comment #13 from Brady Eidson <beidson at apple.com> ---
Comment on attachment 457548
  --> https://bugs.webkit.org/attachment.cgi?id=457548
Patch feedback

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

> Source/WebKit/webpushd/PushServiceConnection.h:46
> +    using PublicTokenHandler = Function<void(Vector<uint8_t>&&)>;

I generally really dislike these types of "using"s and typedefs. I think they
often make it harder to reason about code in other places.

This is a good example of that - this is masking over a very straightforward
"function that takes a uint8_t vector" with an unrelated name of
"PublicTokenHandler"

I don't think this "using" makes the code better. I think it makes the code
less readable.	More on this below:

> Source/WebKit/webpushd/PushServiceConnection.h:87
> +    PublicTokenHandler m_publicTokenHandler;
> +    Vector<uint8_t> m_pendingPublicToken;

These two members are obviously related to each other. There's obviously a
symmetry to them.
I think it would be more readable here (and in other places) if it simply read:

Function<void(Vector<uint8_t>&&)>  m_publicTokenChangeHandler;
Vector<uint8_t> m_pendingPublicToken;

(Notice I also renamed the function's variable name to make it even more clear
what it's for)