[webkit-reviews] review granted: [Bug 231007] Add support for PushEvent : [Attachment 439832] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 231007: Add support for PushEvent
https://bugs.webkit.org/show_bug.cgi?id=231007

Attachment 439832: Patch

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




--- Comment #8 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 439832
  --> https://bugs.webkit.org/attachment.cgi?id=439832
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.