[webkit-reviews] review canceled: [Bug 120883] MediaStream API: Changing the device enumeration to be async : [Attachment 211604] Add blink revisions this is based on to ChangeLogs

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has canceled Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 120883: MediaStream API: Changing the device enumeration to be async
https://bugs.webkit.org/show_bug.cgi?id=120883

Attachment 211604: Add blink revisions this is based on to ChangeLogs
https://bugs.webkit.org/attachment.cgi?id=211604&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
(In reply to comment #9)
> (From update of attachment 211604 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=211604&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:125
> > +	 if
(!MediaStreamCenter::instance().getMediaStreamTrackSources(request.release()))
> 
> Not part of the patch, but is instance() the right name for the singleton? I
think we typically use sharedBlah() or even just shared()?
> 
Sure.

> > Source/WebCore/Modules/mediastream/MediaStreamTrack.idl:42
> > +	 [CallWith=ScriptExecutionContext, RaisesException] static void
getSources(MediaStreamTrackSourcesCallback callback);
> 
> Is this right? The more recent specs list a synchronous call called
getSourceInfos(). If that's the case, I don't think the callback/request
classes are necessary.
>
As Tommy notes, this change was agreed to informally on the mailing list in
June. The spec bug is here:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=22269.

 
> > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.cpp:50
> > +void MediaStreamTrackSourcesRequest::didCompleteRequest(const
TrackSourceInfoVector& requstSourceInfos)
> 
> Typo: requestSourceInfos
> 
Fixed.

> > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:54
> > +	 String origin() { return m_origin; }
> 
> should be const.
> 
Fixed.

> > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:56
> > +	 PassRefPtr<ExtraData> extraData() const { return m_extraData; }
> 
> This is strange. Shouldn't it just be ExtraData*?
> 
Actually, we don't need this at all so I removed it. "ExtraData" is a chromium
platform pattern we don't use).

> > Source/WebCore/Modules/mediastream/SourceInfo.cpp:56
> > +	     return videoKind;
> 
> This logic seems completely wrong.
> 
Oops, fixed.

> > Source/WebCore/bindings/js/JSDOMBinding.h:380
> > +	 inline JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject*
globalObject, Vector<RefPtr<T> > vector)
> 
> Spaces between consecutive >'s is no longer needed, right?
> 
Fixed.

> >
Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:61
> > +	 SourceKind kind() { return m_kind; }
> 
> Why not const?
> 
Why not!

> >
Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:62
> > +	 VideoFacingMode facing() { return m_facing; }
> 
> Ditto?
> 
Ditto!

> >
Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:79
> > +typedef Vector<RefPtr<TrackSourceInfo> > TrackSourceInfoVector;
> 
> Vector<RefPtr<TrackSourceInfo>>
> 
Fixed.

> > LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html:5
> > +<script src="../js/resources/js-test-pre.js"></script>
> 
> this is now ../../resources/
> 
Fixed, and removed the reference to "../js/resources/js-test-style.css".

> > LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html:56
> > +
> 
> We should add a setTimeout to force the test to complete without waiting for
the long test timeout.
> 
Fixed.

> > LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html:60
> > +<script src="../js/resources/js-test-post.js"></script>
> 
> ../../resources
>
Fixed.