Bug 231007 – Add support for PushEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Add support for PushEvent

[youenn fablet]

Created attachment 439703 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83707470>

[youenn fablet]

Created attachment 439725 [details]
Patch

[Alex Christensen]

Comment on attachment 439725 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:1119
> +  webcoreBinding: RuntimeEnabledFeatures

Is there a reason we can't use Settings?

> Source/WebCore/workers/service/PushMessageData.h:55
> +    Vector<uint8_t> m_data;

Could we just store the PushMessageDataInit instead of copying it 1-2 times to get it to this form then copying it again on its way out?
Also, could you add a test that creates a PushEvent from an ArrayBuffer, gets an arrayBuffer from it, modifies the original ArrayBuffer, then checks to see if the gotten array buffer has the changes?

[youenn fablet]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 439725 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439725&action=review
> 
> > Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:1119
> > +  webcoreBinding: RuntimeEnabledFeatures
> 
> Is there a reason we can't use Settings?

Service Worker abides to runtime features but not to settings by default.

> > Source/WebCore/workers/service/PushMessageData.h:55
> > +    Vector<uint8_t> m_data;
> 
> Could we just store the PushMessageDataInit instead of copying it 1-2 times
> to get it to this form then copying it again on its way out?

My understanding is thatm in most cases, the push event is created by UA from native binary data which I represent/generate as a Vector.

> Also, could you add a test that creates a PushEvent from an ArrayBuffer,
> gets an arrayBuffer from it, modifies the original ArrayBuffer, then checks
> to see if the gotten array buffer has the changes?

Will add.

[youenn fablet]

Created attachment 439827 [details]
Patch

[youenn fablet]

Created attachment 439832 [details]
Patch

[Chris Dumez]

Comment on attachment 439832 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushEvent.cpp:46
> +        return { reinterpret_cast<const uint8_t*>(value->data()), value->byteLength() };

A static_cast should suffice.

> Source/WebCore/Modules/push-api/PushEvent.cpp:50
> +        return { reinterpret_cast<const uint8_t*>(value->baseAddress()), value->byteLength() };

ditto.

> Source/WebCore/Modules/push-api/PushEvent.cpp:74
> +        m_data = PushMessageData::create(WTFMove(*data));

Would be better in the initializer list:
, m_data(data ? PushMessageData::create(WTFMove(*data)) : nullptr)

> Source/WebCore/Modules/push-api/PushEvent.cpp:77
> +PushEvent::~PushEvent()

= default;

> Source/WebCore/Modules/push-api/PushEvent.h:40
> +    static Ref<PushEvent> create(const AtomString&, ExtendableEventInit&&, std::optional<Vector<uint8_t>>&&, IsTrusted);

Seems a little odd to have factory that takes in a ExtendableEventInit instead of a PushEventInit. I feel it would be nicer if the constructor and factory both used PushEventInit exclusively.

> Source/WebCore/Modules/push-api/PushEventInit.h:30
> +#include "ExtendableEvent.h"

Can't we include ExtendableEventInit.h instead?

> Source/WebCore/Modules/push-api/PushMessageData.cpp:46
> +        return Exception { OutOfMemoryError };

Please pass an exception message as second parameter.

> Source/WebCore/Modules/push-api/PushMessageData.cpp:60
> +        return Exception { SyntaxError };

ditto.

> Source/WebCore/Modules/push-api/PushMessageData.h:45
> +    ~PushMessageData() = default;

Why is this needed?

> Source/WebCore/Modules/push-api/PushMessageData.h:58
> +inline PushMessageData::PushMessageData(Vector<uint8_t>&& data)

I don't really think we need to inline this. Having it in the cpp like we usually do seems fine.

> Source/WebCore/page/RuntimeEnabledFeatures.h:341
> +    bool m_pushAPIEnabled { false };

Aren't RuntimeEnabledFeatures deprecated in favor on settings? The only remaining reason to use RuntimeEnabledFeatures was non-main thread use but this was addressed in settings a while back.

[youenn fablet]

Thanks for the review, I'll update he patch accordingly.

> > Source/WebCore/page/RuntimeEnabledFeatures.h:341
> > +    bool m_pushAPIEnabled { false };
> 
> Aren't RuntimeEnabledFeatures deprecated in favor on settings? The only
> remaining reason to use RuntimeEnabledFeatures was non-main thread use but
> this was addressed in settings a while back.

We do not have the automated infrastructure to handle settings correctly in service workers.
The main reason is that service workers are not attached to a particular page/webpage/webpageproxy. Service worker may in fact interact with different pages with different settings.

[Chris Dumez]

(In reply to youenn fablet from comment #9)
> Thanks for the review, I'll update he patch accordingly.
> 
> > > Source/WebCore/page/RuntimeEnabledFeatures.h:341
> > > +    bool m_pushAPIEnabled { false };
> > 
> > Aren't RuntimeEnabledFeatures deprecated in favor on settings? The only
> > remaining reason to use RuntimeEnabledFeatures was non-main thread use but
> > this was addressed in settings a while back.
> 
> We do not have the automated infrastructure to handle settings correctly in
> service workers.
> The main reason is that service workers are not attached to a particular
> page/webpage/webpageproxy. Service worker may in fact interact with
> different pages with different settings.

Ok

[youenn fablet]

Created attachment 439861 [details]
Patch for landing

[youenn fablet]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 439832 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439832&action=review
> 
> > Source/WebCore/Modules/push-api/PushEvent.cpp:46
> > +        return { reinterpret_cast<const uint8_t*>(value->data()), value->byteLength() };
> 
> A static_cast should suffice.

data is const char*, not void* here.
Ditto for the other call site.

> > Source/WebCore/Modules/push-api/PushEvent.cpp:74
> > +        m_data = PushMessageData::create(WTFMove(*data));
> 
> Would be better in the initializer list:
> , m_data(data ? PushMessageData::create(WTFMove(*data)) : nullptr)

OK

> > Source/WebCore/Modules/push-api/PushEvent.cpp:77
> > +PushEvent::~PushEvent()
> 
> = default;

I tend to prefer { } in source files, since we often need at some point to add some clean-up code.

> > Source/WebCore/Modules/push-api/PushEvent.h:40
> > +    static Ref<PushEvent> create(const AtomString&, ExtendableEventInit&&, std::optional<Vector<uint8_t>>&&, IsTrusted);
> 
> Seems a little odd to have factory that takes in a ExtendableEventInit
> instead of a PushEventInit. I feel it would be nicer if the constructor and
> factory both used PushEventInit exclusively.

It is like other events that have a create for JS and another for C++
I am not yet sure whether we need the ExtendableEventInit parameter in the C++ version. If not, I'll remove this parameter from this method.

> > Source/WebCore/Modules/push-api/PushEventInit.h:30
> > +#include "ExtendableEvent.h"
> 
> Can't we include ExtendableEventInit.h instead?

OK

> > Source/WebCore/Modules/push-api/PushMessageData.cpp:46
> > +        return Exception { OutOfMemoryError };

OutOfMemoryError seems self explanatory and is the only exception in this method.

> Please pass an exception message as second parameter.
> 
> > Source/WebCore/Modules/push-api/PushMessageData.cpp:60
> > +        return Exception { SyntaxError };
> 
> ditto.

OK

> > Source/WebCore/Modules/push-api/PushMessageData.h:45
> > +    ~PushMessageData() = default;
> 
> Why is this needed?

Removed.

> > Source/WebCore/Modules/push-api/PushMessageData.h:58
> > +inline PushMessageData::PushMessageData(Vector<uint8_t>&& data)
> 
> I don't really think we need to inline this. Having it in the cpp like we
> usually do seems fine.

Moving it to source does not bring more isolation like additional headers removed from PushMessageData.h and constructor is very straightforward.

[Chris Dumez]

(In reply to youenn fablet from comment #12)
> (In reply to Chris Dumez from comment #8)
> > Comment on attachment 439832 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=439832&action=review
> > 
> > > Source/WebCore/Modules/push-api/PushEvent.cpp:46
> > > +        return { reinterpret_cast<const uint8_t*>(value->data()), value->byteLength() };
> > 
> > A static_cast should suffice.
> 
> data is const char*, not void* here.
> Ditto for the other call site.
> 
> > > Source/WebCore/Modules/push-api/PushEvent.cpp:74
> > > +        m_data = PushMessageData::create(WTFMove(*data));
> > 
> > Would be better in the initializer list:
> > , m_data(data ? PushMessageData::create(WTFMove(*data)) : nullptr)
> 
> OK
> 
> > > Source/WebCore/Modules/push-api/PushEvent.cpp:77
> > > +PushEvent::~PushEvent()
> > 
> > = default;
> 
> I tend to prefer { } in source files, since we often need at some point to
> add some clean-up code.

FYI, I think it is not great practice because they are not equivalent. I believe that if you define a destructor with { }, even an empty one, then you lose default move constructor / assignments operators for e.g. It may not matter in this particular instance but `= default` is strictly superior in general unless you actually need to do something special in the destructor.

> 
> > > Source/WebCore/Modules/push-api/PushEvent.h:40
> > > +    static Ref<PushEvent> create(const AtomString&, ExtendableEventInit&&, std::optional<Vector<uint8_t>>&&, IsTrusted);
> > 
> > Seems a little odd to have factory that takes in a ExtendableEventInit
> > instead of a PushEventInit. I feel it would be nicer if the constructor and
> > factory both used PushEventInit exclusively.
> 
> It is like other events that have a create for JS and another for C++
> I am not yet sure whether we need the ExtendableEventInit parameter in the
> C++ version. If not, I'll remove this parameter from this method.
> 
> > > Source/WebCore/Modules/push-api/PushEventInit.h:30
> > > +#include "ExtendableEvent.h"
> > 
> > Can't we include ExtendableEventInit.h instead?
> 
> OK
> 
> > > Source/WebCore/Modules/push-api/PushMessageData.cpp:46
> > > +        return Exception { OutOfMemoryError };
> 
> OutOfMemoryError seems self explanatory and is the only exception in this
> method.
> 
> > Please pass an exception message as second parameter.
> > 
> > > Source/WebCore/Modules/push-api/PushMessageData.cpp:60
> > > +        return Exception { SyntaxError };
> > 
> > ditto.
> 
> OK
> 
> > > Source/WebCore/Modules/push-api/PushMessageData.h:45
> > > +    ~PushMessageData() = default;
> > 
> > Why is this needed?
> 
> Removed.
> 
> > > Source/WebCore/Modules/push-api/PushMessageData.h:58
> > > +inline PushMessageData::PushMessageData(Vector<uint8_t>&& data)
> > 
> > I don't really think we need to inline this. Having it in the cpp like we
> > usually do seems fine.
> 
> Moving it to source does not bring more isolation like additional headers
> removed from PushMessageData.h and constructor is very straightforward.

[EWS]

Committed r283377 (242385@main): <https://commits.webkit.org/242385@main>

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