[webkit-reviews] review granted: [Bug 235856] Import APSConnection-related SPI : [Attachment 450378] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted  review:
Bug 235856: Import APSConnection-related SPI
https://bugs.webkit.org/show_bug.cgi?id=235856

Attachment 450378: Patch

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




--- Comment #8 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 450378
  --> https://bugs.webkit.org/attachment.cgi?id=450378
Patch

r=me provided that the questions for |this| get either answered or fixed before
landing.

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10067
> +				EBA8D3B027A5E33F00CB7900 /*
FakePushServiceConnection.mm */,

We use Mock usually instead of Fake, can we rename it?

> Source/WebKit/webpushd/ApplePushServiceConnection.h:37
> +class ApplePushServiceConnection : public PushServiceConnection {

final?

> Source/WebKit/webpushd/ApplePushServiceConnection.h:41
> +    virtual ~ApplePushServiceConnection();

virtual probably not needed.

> Source/WebKit/webpushd/ApplePushServiceConnection.h:56
> +    void setTopicLists(TopicLists&&) final;

Do we need to expose all these methods or can we make the private?

> Source/WebKit/webpushd/ApplePushServiceConnection.mm:79
> +{

Since we are dealing with threads, we could add a isMainRunLoop assertion.

> Source/WebKit/webpushd/ApplePushServiceConnection.mm:86
> +	   handler(token.tokenURL, error);

It seems the dispatch to main queue makes it possible that |this| would not be
valid here.
I would either use a weak ptr or take a ref.

> Source/WebKit/webpushd/ApplePushServiceConnection.mm:100
> +	       handler(success, error);

Ditto.

> Source/WebKit/webpushd/ApplePushServiceConnection.mm:145
> +void ApplePushServiceConnection::setTopicLists(TopicLists&& topicLists)

Do we need a &&?
It seems a const& is good enough.

> Source/WebKit/webpushd/FakePushServiceConnection.h:34
> +class FakePushServiceConnection : public PushServiceConnection {

Mock and final?

> Source/WebKit/webpushd/FakePushServiceConnection.h:38
> +    virtual ~FakePushServiceConnection();

virtual probably not needed.

> Source/WebKit/webpushd/FakePushServiceConnection.h:53
> +    void setTopicLists(TopicLists&&) final;

Can we make them private?

> Source/WebKit/webpushd/PushServiceConnection.h:30
> +#include <wtf/Function.h>

Function unneeded?

> Source/WebKit/webpushd/PushServiceConnection.h:31
> +#include <wtf/RetainPtr.h>

Retain unneeded?

> Source/WebKit/webpushd/PushServiceConnection.h:33
> +#include <wtf/WorkQueue.h>

WorkQueue in source file?

> Source/WebKit/webpushd/PushServiceConnection.mm:40
> +    WorkQueue::main().dispatch([this]() mutable {

How are we use |this| is valid?