Bug 235856 – Import APSConnection-related SPI

• Component: New Bugs
• Status: RESOLVED FIXED

[Ben Nham]

Import APSConnection-related SPI

[Ben Nham]

Created attachment 450318 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/88219151>

[Ben Nham]

Created attachment 450332 [details]
Patch

[Ben Nham]

Created attachment 450342 [details]
Patch

[Ben Nham]

Created attachment 450349 [details]
Patch

[Ben Nham]

Created attachment 450355 [details]
Patch

[Ben Nham]

Created attachment 450378 [details]
Patch

[youenn fablet]

Comment on attachment 450378 [details]
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?

[Ben Nham]

(In reply to youenn fablet from comment #8)
> Comment on attachment 450378 [details]
> > Source/WebKit/webpushd/ApplePushServiceConnection.h:56
> > +    void setTopicLists(TopicLists&&) final;
> 
> Do we need to expose all these methods or can we make the private?

Right now I am only calling the big setTopicLists setter, since APS recommends calling that over the more granular setters if you need to initialize all state at once (e.g. at launch time). But in a future patch I will likely call the more granular setters also. So I decided to expose them all here.

> > 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.

Right, this applies to both this patch and PushService in 235857. To be technically correct in all scenarios, this object would have to be ref-counted, and the lambdas would have to protectThis or do something similar.

However I omitted that in both this class and in PushService (https://bugs.webkit.org/show_bug.cgi?id=235857) because these classes are held onto by a NeverDestroyed<WebPushDaemon> singleton instance.

Maybe that's a bad idea and I should just write the class so that it could be used outside of the context of WebPushDaemon, though I'm not sure what other use case there would be. That's also why I put this class in the WebPushD namespace rather than the WebKit namespace.

What do you think?

> > Source/WebKit/webpushd/ApplePushServiceConnection.mm:145
> > +void ApplePushServiceConnection::setTopicLists(TopicLists&& topicLists)
> 
> Do we need a &&?
> It seems a const& is good enough.

I made it && since VectorCocoa wants && and it does actually seem to move the elements out of the array.

Actually, now that I've said that, I should probably change the other topic setters to use && instead of const &.

[youenn fablet]

> > > Source/WebKit/webpushd/ApplePushServiceConnection.mm:145
> > > +void ApplePushServiceConnection::setTopicLists(TopicLists&& topicLists)
> > 
> > Do we need a &&?
> > It seems a const& is good enough.
> 
> I made it && since VectorCocoa wants && and it does actually seem to move
> the elements out of the array.
> 
> Actually, now that I've said that, I should probably change the other topic
> setters to use && instead of const &.

If it is a &&, we should use some WTFMove within the setter to make use of &&.
I do not see any such use hence why const& might be sufficient.

[Ben Nham]

Created attachment 450430 [details]
patch feedback

[Ben Nham]

Created attachment 450434 [details]
patch feedback

[EWS]

Committed r288902 (246646@main): <https://commits.webkit.org/246646@main>

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

[Robert Jenner]

Reverted r288902 for reason:

Broke compile-webkit for iOS-15-Simulator

Committed r289073 (?): <https://commits.webkit.org/r289073>

[Ben Nham]

Created attachment 450825 [details]
update tbd

[Ben Nham]

Created attachment 450861 [details]
patch for relanding

[EWS]

Committed r289114 (246811@main): <https://commits.webkit.org/246811@main>

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