[webkit-reviews] review granted: [Bug 190018] Runtime flag and IDL for MediaRecorder : [Attachment 351467] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Wendy <yuhan_wu at apple.com>'s
request for review:
Bug 190018: Runtime flag and IDL for MediaRecorder
https://bugs.webkit.org/show_bug.cgi?id=190018

Attachment 351467: Patch

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




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

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

> Source/WebCore/ChangeLog:6
> +	   Reviewed by Youenn Fablet.

At this stage of the patch process, it should probably be marked OOPS.

> Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:30
> +namespace WebCore {

This file does not define anything.
It is probably fine if we are we sure we will need it in the future.
If we are not sure, maybe we should remove it for now.

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:43
> +    static Ref<BlobEvent> create(const AtomicString& type, const Init& init)

If BlobEvent is expected to keep a RefPtr<Blob>, we might want to change const
Init& to Init&&.
That can be done later on when actually implementing BlobEvent.

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:48
> +    ~BlobEvent()

Can we remove this destructor?

> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:31
> +    Constructor(DOMString type, BlobEventInit eventInitDict)

Maybe we can sort the keywords (Conditional, Constructor, Enabled, Exposed)

> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:100
> +    macro(BlobEvent) \

Should be moved up for sorting.

> LayoutTests/platform/mac-wk1/TestExpectations:635
> +# MediaRecorder is not supporrted on WK1

s/supporrted/supported/
Canvas capture is supported in WK1 so we should not skip the Canvas Capture
tests.
As of Media Recorder, we might consider adding support for it since it should
not be WK2 specific.
Maybe all that is needed is to update enableExperimentalFeatures() function in
DumpRenderTree.mm to activate the runtime flag.

You might also want to skip these tests for the win platform.