Bug 67946

Created attachment 107159 [details] patch for discussion (can be applied) The previous patch contained diffs of moved files and thus could not be applied. The diffs of the moved files will be provided in a separate attachment to make it easier to see the changes.

Created attachment 107161 [details] diff of moved files

Created attachment 107176 [details] design diagram

any chance that you could give a brief textual overview of the design and how it's different? That might help to spark the discussion.

Hi, I have looked at the patch attached to this bug and also had a look at the code that is already committed in Webkit. I think it is not only about the Webkit client design and WebCore platform abstractions but also about the level of details getting added to WebCore. Lot of details like camera preferences, peer connection handling, etc is getting added to WebCore where as it is left to the user agent(client) with the design already committed in WebKit. What details should be added and what should be left to the client will have different opinions as spec doesn't say much about it. Please correct me if I have misinterpreted anything. While adding more details to WebCore/Webkit, we also need to make sure that we are not restricting the implementation to video conferencing. Peer to peer component could separately be used for exchanging some other real time data and same is the case for recording components. I think, the WebCore platform abstractions approach will suit well to most of the platforms but will take some extra efforts for chrome because of the sadboxed environment. But if it gets finalized, it needs to be fine tuned before commit(for example, small things like moving options parsing code from navigator to UserMediaController would make it look better:)) Code wise, I personally liked MediaStreamFrameController and MediaStreamController's request map over UserMediaController and callback bundles. If the design is not finalized yet, comments from other ports(mac, qt, etc) contributers and maintainers would be useful. May be combining good things from both the designs would lead to a design that will suit well to most of the Webkit ports. No matter which design is finalized, as mentioned earlier, we are interested in this feature and we would be glad to contribute in filling the missing gaps in implementation.

(In reply to comment #5) > Lot of details like camera preferences, peer connection handling, etc is getting added to WebCore where as it is left to the user agent(client) with the design already committed in WebKit. What details should be added and what should be left to the client will have different opinions as spec doesn't say much about it. Both camera preferences and PeerConnection handling are in the specification. Generally, it's preferred to share as much code as possible - especially if it concerns standard related behavior since it helps to maintain consistency between ports.

(In reply to comment #4) > any chance that you could give a brief textual overview of the design and how it's different? That might help to spark the discussion. The main difference is that calls to low-level functions are made down to the WebCore platform, rather than up to a WebKit client (and to the browser). Additionally, we have put more of the logic from the specification in the shared WebCore code.

(In reply to comment #7) > The main difference is that calls to low-level functions are made down to the WebCore platform, rather than up to a WebKit client (and to the browser). Additionally, we have put more of the logic from the specification in the shared WebCore code. Yeah, after I asked that question a few days ago I spent a bit of time studying the patch, and I must say that it looks like a major improvement in terms of implementing this stuff in other ports. I'm also interested in helping out on the gtk port implementation. Any chance you have the rest of your gstreamer/gtk implementation code around somewhere I could look at? (e.g. PeerHandlerPrivateGStreamer, etc)?

While there are much to like in this patch there are a few things I (the main commiter for this feature) would like to point out: This patch completely removes the functionality that shuts down everything when a frame is moved/deleted or the page goes away before the frame etc. This is handled in the MediaStreamController and MediaStreamFrameController. The current code will change quite drastically due to strong feedback; instead of passing around IDs the actual objects will be sent instead. Also some refactoring is done right now to move functionality to WebCore/platform.

(In reply to comment #9) > This patch completely removes the functionality that shuts down everything when a frame is moved/deleted or the page goes away before the frame etc. This is handled in the MediaStreamController and MediaStreamFrameController. > We kept UserMediaController from our branch instead of MediaStreamController and MediaStreamFrameController since the majority of the code in the latter two deal with relaying calls from the MediaStream API and PeerConnection to the WebKit page client (our controller only deals with functionality related to getUserMedia in this design). Although Navigator, UserMediaController and UserMediaClient have teardown procedures to cancel outstanding requests (to dismiss any open media selectors), e.g. when you navigate away from a page, you may be right that it is not sufficient in some special cases. Do you think that the approach with two controllers will be needed in this design as well?

Comment on attachment 107159 [details] patch for discussion (can be applied) View in context: https://bugs.webkit.org/attachment.cgi?id=107159&action=review This patch is too large to review, but I like the approach of moving a bunch of the code into WebCore/platform. Renaming p2p to mediastream also seems beneficial, but a slightly separable concern. I also like that you're using ActiveDOMObject rather than the Frame- and Page-level controller objects. That's good improvement to the design. > Source/WebCore/platform/mediastream/PeerHandler.h:76 > +class PeerHandlerClient { These classes in WebCore/platform shouldn't be called Clients. In WebKit, the client exist to talk with "up" to the WebKit Client, not "down" to the platform.

(In reply to comment #11) > (From update of attachment 107159 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107159&action=review > > This patch is too large to review, but I like the approach of moving a bunch of the code into WebCore/platform. Thanks for taking the time to look at the patch even though it's large. I knew that it's too large to be reviewed and that's why I labeled it "patch for discussion" (without review-flag and ChangeLog). The idea is to get people on board with the proposed changes and then submit smaller patches. > Renaming p2p to mediastream also seems beneficial, but a slightly separable concern. We're not simply renaming p2p to mediastream, but also gathering various MediaStream-related files from WebCore/dom, WebCore/page and WebCore/platform in this new folder. > > Source/WebCore/platform/mediastream/PeerHandler.h:76 > > +class PeerHandlerClient { > > These classes in WebCore/platform shouldn't be called Clients. In WebKit, the client exist to talk with "up" to the WebKit Client, not "down" to the platform. The PeerHandlerClient interface is used by PeerHandler to talk "up" to PeerConnection much like HTMLMediaElement is a MediaPlayerClient. However, we don't have a strong opinion about the name.

Closing as this is no longer relevant.