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

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.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 351564: Patch

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




--- Comment #25 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 351564
  --> https://bugs.webkit.org/attachment.cgi?id=351564
Patch

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

> Source/WebCore/ChangeLog:6
> +	   Reviewed by Youenn Fablet (OOPS!).

Please drop the OOPS.

> Source/WebCore/ChangeLog:12
> +	   Add an architechture of the IDL and the class for MediaRecorder and
BlobEvent.

Typo: architechture

> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:33
> +    // FIXME: The current patch is only for architecture of the BlobEvent

Please use period at the end of comments, as per coding style.

> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:34
> +    // FIXME: It will be implemented in follow-up patches

ditto.

Also, here is what we would usually do for such comments (we rarely use "Will
be implemented in follow-up patches" in code comments):

// FIXME: Implement these:
// [ SameObject] readonly attribute Blob data;
// readonly attribute DOMHighResTimeStamp timecode;

Then you only have to uncomment them later on.

> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:36
> +dictionary BlobEventInit : EventInit {

I would add a blank line before this.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:47
> +    , m_state(RecordingState::Inactive)

This could be inline in the header.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:58
> +    return false; // FIXME: This function will be implemented in a follow-up
patch

Please use a period at the end of the comment.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:49
> +    RecordingState state() const

I think we usually put these on one line:
RecordingState state() const { return m_state; }

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:72
> +    RecordingState m_state;

RecordingState m_state { RecordingState::Inactive };

> Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:37
> +    // FIXME: It will be implemented in follow-up patches

Same comments as above for the comments.

> LayoutTests/platform/mac-wk1/TestExpectations:633
> +webkit.org/b/189908
imported/w3c/web-platform-tests/resource-timing/resource_timing.worker.html [
Failure ]

Looks like this file has no real changes and should not be part of this patch.

> LayoutTests/platform/win/TestExpectations:4216
> +# MediaRecorder only enalbed for wk1 and wk2

Typo: enalbed.
Also missing a period at the end.
Finally, the comment does not really make sense because Windows uses WebKit1.
Maybe it should be "# MediaRecorder is not enabled on Windows."