Bug 190018

Created attachment 351103 [details] Patch

Comment on attachment 351103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351103&action=review > Source/WebCore/ChangeLog:8 > + Add an architechture of the IDL and the class for MediaRecorder and BlobEvent. The patch looks good. You might also need to: - Enable the runtime flag for WebKitTestRunner/DumpRenderTree. - Add a test to verify that BlobEvent, MediaRecorder have constructors and do work. This test might already be covered partially in imported/w3c/web-platform-tests. > Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:5 > + * Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies). This copyright should only have the Apple line with 2018 as the year. Since BlobEvent.cpp is not needed, I would not include it here in this patch. > Source/WebCore/Modules/mediarecorder/BlobEvent.h:27 > +#if ENABLE(MEDIA_STREAM) Add a line here. > Source/WebCore/Modules/mediarecorder/BlobEvent.h:29 > +#include <wtf/text/AtomicString.h> Ditto here. > Source/WebCore/Modules/mediarecorder/BlobEvent.h:35 > + No need for this line. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:5 > + * Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies). Ditto for the copyright. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:33 > + Two lines can be removed. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:42 > +Ref<MediaRecorder> MediaRecorder::create(ScriptExecutionContext& context, Ref<MediaStream>&& stream, Options options) Should be Options&& > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:48 > + : ActiveDOMObject(&context) No need for this constructor, instead reuse the constructor below with a Options&& = { } in the declaration. > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:34 > +class MediaRecorder : public ActiveDOMObject, public RefCounted<MediaRecorder>, public EventTargetWithInlineData { "class MediaRecorder" final would be better. > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:40 > + unsigned long audioBitsPerSecond, videoBitsPerSecond, bitsPerSecond; One line per member. > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:44 > + static Ref<MediaRecorder> create(ScriptExecutionContext&, Ref<MediaStream>&&, Options); Options&& = { } should do the trick. > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:53 > + } Do we need to define this destructor. > Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:29 > + EnabledAtRuntime=MediaRecorder, Sort the keywords lexically. > Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:35 > +#include "PeerConnectionBackend.h" Is this change needed because of unified build issues? > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-18787 > - 574F55DF204F3744002948C6 /* LocalAuthenticator.mm */, Unrelated change?

Comment on attachment 351103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351103&action=review > Source/WebCore/Sources.txt:184 > +Modules/mediarecorder/MediaRecorder.cpp > +Modules/mediarecorder/BlobEvent.cpp Nit: reverse the order of these lines so the "mediarecorder" group is sorted.

Created attachment 351422 [details] Patch

Created attachment 351429 [details] Patch

Created attachment 351431 [details] Patch

Created attachment 351433 [details] Patch

Comment on attachment 351433 [details] Patch Attachment 351433 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9431681 New failing tests: fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html

Created attachment 351442 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Comment on attachment 351433 [details] Patch Attachment 351433 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9431709 New failing tests: imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor.html imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html

Created attachment 351446 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6

(In reply to Build Bot from comment #8) > Comment on attachment 351433 [details] > Patch > > Attachment 351433 [details] did not pass mac-wk2-ews (mac-wk2): > Output: https://webkit-queues.webkit.org/results/9431681 > > New failing tests: > fast/mediacapturefromelement/CanvasCaptureMediaStream- > imagebitmaprenderingcontext.html > fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html > fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM- > element.html These errors seem pretty nice as they seem like improvements. You might need to update the corresponding -expected.txt files.

Comment on attachment 351433 [details] Patch Attachment 351433 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9432434 New failing tests: imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor.html imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html media/remote-control-command-seek.html

Created attachment 351447 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 351464 [details] Patch

Created attachment 351467 [details] Patch

Comment on attachment 351467 [details] Patch Attachment 351467 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9435513 New failing tests: fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html

Created attachment 351475 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

Comment on attachment 351467 [details] 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.

Comment on attachment 351467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351467&action=review >> 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. Yes, I think it should be removed. It can always be added back later if and when it is needed. > Source/WebCore/Modules/mediarecorder/BlobEvent.h:30 > +#include <wtf/text/AtomicString.h> Not needed. > Source/WebCore/Modules/mediarecorder/BlobEvent.h:36 > +class BlobEvent : public Event { Class should be marked as final since Event is virtual. >> 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. I believe our current pattern in other event classes is also to have a third parameter like so: , IsTrusted isTrusted = IsTrusted::No Allowing the call site to construct an event that is trusted (trusted means that it is issued by the browser rather than the script on the page). >> Source/WebCore/Modules/mediarecorder/BlobEvent.h:48 >> + ~BlobEvent() > > Can we remove this destructor? +1 > Source/WebCore/Modules/mediarecorder/BlobEvent.idl:33 > + Why are we missed the attributes. If you plan to add them in a follow-up, could you please add a FIXME comment? > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:53 > + return false; Please add a FIXME comment and follow-up. Returning false here is rarely OK as it prevents page cache. > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:30 > +#include "MediaStream.h" Can this be forward declared instead? (You mean need an out-of-line destructor). > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:40 > + unsigned long audioBitsPerSecond; unsigned long -> unsigned (unsigned long in Web IDL maps to unsigned in C++). > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:41 > + unsigned long videoBitsPerSecond; ditto > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:42 > + unsigned long bitsPerSecond; ditto. > Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:31 > + ConstructorCallWith=ScriptExecutionContext, You could use ConstructorCallWith=Document since this is not exposed to workers. Then use Document& instead of ScriptExecutionContext& in your C++ implementation. > Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:35 > + readonly attribute RecordingState state; Could you please add a FIXME comment about the missing API? > Source/WebCore/Sources.txt:2774 > +JSBlobEvent.cpp Bad sorting

Comment on attachment 351467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351467&action=review >>> 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. > > Yes, I think it should be removed. It can always be added back later if and when it is needed. Yes, I think it should be removed. It can always be added back later if and when it is needed. >> Source/WebCore/Modules/mediarecorder/BlobEvent.h:30 >> +#include <wtf/text/AtomicString.h> > > Not needed. Not needed. >> Source/WebCore/Modules/mediarecorder/BlobEvent.h:36 >> +class BlobEvent : public Event { > > Class should be marked as final since Event is virtual. Class should be marked as final since Event is virtual. >>> 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. > > I believe our current pattern in other event classes is also to have a third parameter like so: > , IsTrusted isTrusted = IsTrusted::No > > Allowing the call site to construct an event that is trusted (trusted means that it is issued by the browser rather than the script on the page). I believe our current pattern in other event classes is also to have a third parameter like so: , IsTrusted isTrusted = IsTrusted::No Allowing the call site to construct an event that is trusted (trusted means that it is issued by the browser rather than the script on the page). > Source/WebCore/Modules/mediarecorder/BlobEvent.h:45 > + return adoptRef(*new BlobEvent(type, init, IsTrusted::No)); auto blobEvent = adoptRef(*new BlobEvent(type, init, isTrusted)); blobEvent->suspendIfNeeded(); return blobEvent; Otherwise, you'll get crashes. You need to call suspendIfNeeded() on ActiveDOMObjects. >>> Source/WebCore/Modules/mediarecorder/BlobEvent.h:48 >>> + ~BlobEvent() >> >> Can we remove this destructor? > > +1 +1 >> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:33 >> + > > Why are we missed the attributes. If you plan to add them in a follow-up, could you please add a FIXME comment? Why are we missed the attributes. If you plan to add them in a follow-up, could you please add a FIXME comment? >> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:53 >> + return false; > > Please add a FIXME comment and follow-up. Returning false here is rarely OK as it prevents page cache. Please add a FIXME comment and follow-up. Returning false here is rarely OK as it prevents page cache. >> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:30 >> +#include "MediaStream.h" > > Can this be forward declared instead? (You mean need an out-of-line destructor). Can this be forward declared instead? (You mean need an out-of-line destructor). >> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:40 >> + unsigned long audioBitsPerSecond; > > unsigned long -> unsigned (unsigned long in Web IDL maps to unsigned in C++). unsigned long -> unsigned (unsigned long in Web IDL maps to unsigned in C++). >> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:41 >> + unsigned long videoBitsPerSecond; > > ditto ditto >> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:42 >> + unsigned long bitsPerSecond; > > ditto. ditto. >> Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:31 >> + ConstructorCallWith=ScriptExecutionContext, > > You could use ConstructorCallWith=Document since this is not exposed to workers. Then use Document& instead of ScriptExecutionContext& in your C++ implementation. You could use ConstructorCallWith=Document since this is not exposed to workers. Then use Document& instead of ScriptExecutionContext& in your C++ implementation. >> Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:35 >> + readonly attribute RecordingState state; > > Could you please add a FIXME comment about the missing API? Could you please add a FIXME comment about the missing API? >> Source/WebCore/Sources.txt:2774 >> +JSBlobEvent.cpp > > Bad sorting Bad sorting

Comment on attachment 351467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351467&action=review >> Source/WebCore/Modules/mediarecorder/BlobEvent.h:45 >> + return adoptRef(*new BlobEvent(type, init, IsTrusted::No)); > > auto blobEvent = adoptRef(*new BlobEvent(type, init, isTrusted)); > blobEvent->suspendIfNeeded(); > return blobEvent; > > Otherwise, you'll get crashes. You need to call suspendIfNeeded() on ActiveDOMObjects. This comment was meant for the MediaRecorder create() function, sorry.

Created attachment 351562 [details] Patch

Created attachment 351564 [details] Patch

Comment on attachment 351564 [details] 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."

Created attachment 351576 [details] Patch

Comment on attachment 351576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351576&action=review > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:57 > + return false; // FIXME: Implement this function to solve the crash issue. This comment is wrong, this should NOT cause any crashes, this is merely inefficient as it prevents PageCache. The comment should be something like. // FIXME: We should do better here as this prevents entering PageCache.

Created attachment 351604 [details] Patch

Comment on attachment 351604 [details] Patch Clearing flags on attachment: 351604 Committed r236840: <https://trac.webkit.org/changeset/236840>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45018935>