Bug 68464

Created attachment 110604 [details] proposed patch

Comment on attachment 110604 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=110604&action=review This looks pretty good. There's still the question of whether |context| should be first in constructors. Also, almost all the places in this patch where we're using unsigned, we probably should be using size_t. > Source/WebCore/dom/MediaStream.cpp:48 > + MediaStreamTrackVector trackVector; > + unsigned numberOfTracks = m_descriptor->numberOfComponents(); You can call reserveCapacity on the vector if you know ahead of time how many items you're going to append. > Source/WebCore/dom/MediaStreamTrack.cpp:50 > + DEFINE_STATIC_LOCAL(AtomicString, kind, (m_streamDescriptor->component(m_trackIndex)->source()->type() == MediaStreamSource::TypeAudio ? "audio" : "video")); It's strange to use a static local that's based on member variables... This doesn't seem right.

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

@fishd: This just deletes interfaces from the WebKit API. The plan is to add new interfaces to WebKitPlatformSupport later.

Created attachment 110871 [details] Updated patch (In reply to comment #2) > (From update of attachment 110604 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110604&action=review > > This looks pretty good. There's still the question of whether |context| should be first in constructors. I've updated the patch to follow the new context first convention. > Also, almost all the places in this patch where we're using unsigned, we probably should be using size_t. Fixed "internal" usage (left unsigned in the DOM API signatures). > > Source/WebCore/dom/MediaStream.cpp:48 > > + MediaStreamTrackVector trackVector; > > + unsigned numberOfTracks = m_descriptor->numberOfComponents(); > > You can call reserveCapacity on the vector if you know ahead of time how many items you're going to append. Good point. Fixed. > > Source/WebCore/dom/MediaStreamTrack.cpp:50 > > + DEFINE_STATIC_LOCAL(AtomicString, kind, (m_streamDescriptor->component(m_trackIndex)->source()->type() == MediaStreamSource::TypeAudio ? "audio" : "video")); > > It's strange to use a static local that's based on member variables... This doesn't seem right. True. That was a bad last minute optimization. :) Fixed.

Comment on attachment 110871 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=110871&action=review In one of your earlier patches, you moved all the non-platform files into a mediastream directory. I think that would be a valuable change to do sooner rather than later. Please fix the out-of-bounds read before landing. > Source/WebCore/ChangeLog:8 > + Update MediaStream to use WebCore platform interfaces > + https://bugs.webkit.org/show_bug.cgi?id=68464 > + > + Reviewed by NOBODY (OOPS!). > + > + Currently not testable. Generally we like a litt more information in ChangeLogs. For this change, I'd explain that this is part of a series of patches that convert the DOM interfaces over to using a new Platform API. Also, I'd include some information about how you plan to test this code and at what stage of this process we'll be able to add tests. I understand that there are a lots of wires to connect before everything is fully functional, but we might be able to add some basic API tests soon. > Source/WebCore/dom/MediaStream.h:84 > + ScriptExecutionContext* m_scriptExecutionContext; This doesn't need to be a RefPtr? > Source/WebCore/dom/MediaStreamTrack.cpp:51 > -const String& MediaStreamTrack::kind() const > +String MediaStreamTrack::kind() const > { > - return m_kind; > + DEFINE_STATIC_LOCAL(AtomicString, audioKind, ("audio")); > + DEFINE_STATIC_LOCAL(AtomicString, videoKind, ("video")); Should this function return an AtomicString? If not, then should these statics be Strings rather than AtomicStrings? (Let me know if you're unsure of the difference between Strings and AtomicStrings.) > Source/WebCore/dom/MediaStreamTrack.cpp:61 > + ASSERT_NOT_REACHED(); > } I'm surprised you don't need to return String() here. > Source/WebCore/dom/MediaStreamTrack.cpp:70 > + return m_streamDescriptor->component(m_trackIndex)->enabled(); It's kind of strange that all of these methods are basically trampolines into m_streamDescriptor components. I guess this object is like a DOM wrapper around the Platform concept, which makes sense, but they we're holding on to the root objects in the Platform object tree and walking the tree each time. I guess that's fine. It's just slightly different from what I woud have expected. > Source/WebCore/dom/MediaStreamTrack.h:54 > + const size_t m_trackIndex; We don't usually have const member variables, but it works nicely here. > Source/WebCore/dom/MediaStreamTrackList.cpp:54 > + ASSERT(index < length()); This is an API exposed to JavaScript, right? What ensures that JavaScript won't call this function with a nutty index? > Source/WebCore/page/MediaStreamController.h:44 > class MediaStreamController { All of MediaStreamController is going away eventually, right? > Source/WebCore/page/MediaStreamFrameController.h:52 > MediaStreamFrameController(Frame*); This one is going away too, I presume.

Created attachment 111261 [details] Patch 3 (In reply to comment #6) > (From update of attachment 110871 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110871&action=review > > In one of your earlier patches, you moved all the non-platform files into a mediastream directory. I think that would be a valuable change to do sooner rather than later. > I've created a bug to move the files. See http://webkit.org/b/70233 > Please fix the out-of-bounds read before landing. item() is an index getter and thus the binding will prevent out-of-bounds reads. > > Source/WebCore/ChangeLog:8 > > + Update MediaStream to use WebCore platform interfaces > > + https://bugs.webkit.org/show_bug.cgi?id=68464 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + Currently not testable. > > Generally we like a litt more information in ChangeLogs. For this change, I'd explain that this is part of a series of patches that convert the DOM interfaces over to using a new Platform API. > Fixed. > Also, I'd include some information about how you plan to test this code and at what stage of this process we'll be able to add tests. I understand that there are a lots of wires to connect before everything is fully functional, but we might be able to add some basic API tests soon. > Fixed (now referring to an existing bug). > > Source/WebCore/dom/MediaStream.h:84 > > + ScriptExecutionContext* m_scriptExecutionContext; > > This doesn't need to be a RefPtr? I made it a RefPtr to be safe. > > Source/WebCore/dom/MediaStreamTrack.cpp:51 > > -const String& MediaStreamTrack::kind() const > > +String MediaStreamTrack::kind() const > > { > > - return m_kind; > > + DEFINE_STATIC_LOCAL(AtomicString, audioKind, ("audio")); > > + DEFINE_STATIC_LOCAL(AtomicString, videoKind, ("video")); > > Should this function return an AtomicString? If not, then should these statics be Strings rather than AtomicStrings? (Let me know if you're unsure of the difference between Strings and AtomicStrings.) > Fixed (they don't have to be AtomicStrings). > > > Source/WebCore/dom/MediaStreamTrack.cpp:70 > > + return m_streamDescriptor->component(m_trackIndex)->enabled(); > > It's kind of strange that all of these methods are basically trampolines into m_streamDescriptor components. I guess this object is like a DOM wrapper around the Platform concept, which makes sense, but they we're holding on to the root objects in the Platform object tree and walking the tree each time. I guess that's fine. It's just slightly different from what I woud have expected. We need to hold on to the MediaStreamDescriptor object since a MediaStreamTrack can exist without its MediaStream and the descriptor is the key to do platform operations on a stream (including the tracks). Since the descriptor is available we can get the information we want from it, but we could cache the information in member variables. > > Source/WebCore/dom/MediaStreamTrackList.cpp:54 > > + ASSERT(index < length()); > > This is an API exposed to JavaScript, right? What ensures that JavaScript won't call this function with a nutty index? > The binding makes sure that this function is called with a valid index (0 <= index < length()). > > Source/WebCore/page/MediaStreamController.h:44 > > class MediaStreamController { > > All of MediaStreamController is going away eventually, right? > > > Source/WebCore/page/MediaStreamFrameController.h:52 > > MediaStreamFrameController(Frame*); > > This one is going away too, I presume. We have a single controller and a page client that only deals with user consent for getUserMedia(). I hope that they could be accepted as replacements for the current controllers and client. I'll file a bug to discuss this further.

Attachment 111261 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource Current branch master is up to date. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #4) > @fishd: This just deletes interfaces from the WebKit API. The plan is to add new interfaces to WebKitPlatformSupport later. OK!

Thanks!

Comment on attachment 111261 [details] Patch 3 Rejecting attachment 111261 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: rror: no match for 'operator!' in '!sources' Source/WebCore/page/MediaStreamFrameController.cpp:256: note: candidates are: operator!(bool) <built-in> Source/JavaScriptCore/wtf/text/WTFString.h:368: note: bool WTF::operator!(const WTF::String&) CXX(target) out/Debug/obj.target/webcore_remaining/Source/WebCore/page/WindowFeatures.o make: *** [out/Debug/obj.target/webcore_remaining/Source/WebCore/page/MediaStreamFrameController.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/10145353

Created attachment 111616 [details] Patch for landing Removed bad ASSERT.

Comment on attachment 111616 [details] Patch for landing Clearing flags on attachment: 111616 Committed r97904: <http://trac.webkit.org/changeset/97904>

All reviewed patches have been landed. Closing bug.