Bug 60260

Created attachment 92398 [details] Patch

Adding fishd, who like to review all changes to the Chromium/WebKit API.

Attachment 92398 [details] did not pass chromium-ews: Output: http://queues.webkit.org/results/8556921

Created attachment 95564 [details] Patch

Created attachment 96411 [details] Patch

Changed platform to All/All

Darin, would you like to review this one since it is a new chromium API?

Created attachment 97260 [details] Patch

Can you tell me more about the design here? It seems like you need to bridge to some code that will live in Chromium. Do you have a Chromium CL for the other side that I can also see? Any other background you can provide about this feature would be helpful too. Thanks!

Well, we don't really have any design doc for this feature, except the whatwg spec <http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#multipletracklist> and <http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#obtaining-local-multimedia-content> The design idea is to do as little as possible in webkit and to delegate as much as possible to the port. We don't have a corresponding chromium CL yet, but would like to has this in first. (In reply to comment #9) > Can you tell me more about the design here? It seems like you need to bridge to some code that will live in Chromium. Do you have a Chromium CL for the other side that I can also see? Any other background you can provide about this feature would be helpful too. Thanks!

(In reply to comment #10) > Well, we don't really have any design doc for this feature, except the whatwg spec <http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#multipletracklist> and <http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#obtaining-local-multimedia-content> > > The design idea is to do as little as possible in webkit and to delegate as much as possible to the port. > > We don't have a corresponding chromium CL yet, but would like to has this in first. This is usually the wrong approach. It is helpful to keep as much of the implementation of the web platform, especially the tricky bits, in WebKit. Now, we have some reasons why we delegate out to Chromium. For example, if the implementation of a platform component requires IPC escalation to the Chromium browser process, then it can't be implemented in WebKit. It may also be the case that you are implementing this based on libraries in the Chromium repository. It would help if you could explain why you feel the need to split complexity between two code bases.

(In reply to comment #11) > (In reply to comment #10) > > Well, we don't really have any design doc for this feature, except the whatwg spec <http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#multipletracklist> and <http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#obtaining-local-multimedia-content> > > > > The design idea is to do as little as possible in webkit and to delegate as much as possible to the port. > > > > We don't have a corresponding chromium CL yet, but would like to has this in first. > > This is usually the wrong approach. It is helpful to keep as much of the implementation of the web platform, especially the tricky bits, in WebKit. Now, we have some reasons why we delegate out to Chromium. For example, if the implementation of a platform component requires IPC escalation to the Chromium browser process, then it can't be implemented in WebKit. It may also be the case that you are implementing this based on libraries in the Chromium repository. > > It would help if you could explain why you feel the need to split complexity between two code bases. Then I understand you better. We have a good reason for the divide and that is the webrtc open source library that was recently added to chromium. That together with upcoming changes to libjingle and other chromium work consists of the complete corresponding chromium implementation. Really there are very little code that can be moved from the chromium side to webkit, and that move will introduce more complexity. However I have been contacted by the Ericsson developers and they propose to add some sort of proxy functionality in WebKit so that ports that wants to do everything in WebKit can do so equally easily. I am very much in favour of such a solution and will meet with them to discuss the plan asap everyone are back from vacations. Right now however I would like to start submitting our current bindings. And FYI I have removed the review? from this patch since the WebCore API has changed, and are waiting for feedback from you.

Created attachment 102908 [details] Patch

Comment on attachment 102908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102908&action=review > Source/WebKit/chromium/public/WebMediaStreamTrackList.h:33 > +#include "WebString.h" This can be removed as it's not required either in the h or cpp files. > Source/WebKit/chromium/public/WebMediaStreamTrackList.h:34 > +#include "WebVector.h" Same for this.

Created attachment 102912 [details] Patch

Comment on attachment 102908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102908&action=review >> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:33 >> +#include "WebString.h" > > This can be removed as it's not required either in the h or cpp files. Done. >> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:34 >> +#include "WebVector.h" > > Same for this. Done.

Comment on attachment 102912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102912&action=review > Source/WebKit/chromium/public/WebMediaStreamTrack.h:32 > +#include "WebString.h" WebString can just be forward declared. > Source/WebKit/chromium/public/WebMediaStreamTrack.h:50 > + WebMediaStreamTrack(WTF::PassRefPtr<WebCore::MediaStreamTrack>); nit: parameter should be 'const Foo&' > Source/WebKit/chromium/public/WebMediaStreamTrack.h:58 > +typedef WebVector<WebMediaStreamTrack> WebTrackVector; nit: the name of this type should be WebMediaStreamTrackVector. also, we normally create a unique header file for each top-level type, so this should go into a WebMediaStreamTrackVector.h all by its lonesome self. but i have to wonder... what is the difference between a WebMediaStreamTrackVector and a WebMediaStreamTrackList? why do we need two container types for WebMediaStreamTrack objects? > Source/WebKit/chromium/public/WebMediaStreamTrackList.h:49 > + WebMediaStreamTrackList(WTF::PassRefPtr<WebCore::MediaStreamTrackList>); nit: parameter should be 'const Foo&' > Source/WebKit/chromium/src/WebMediaStreamTrackList.cpp:42 > + m_private = WebCore::MediaStreamTrackList::create(tracks); nit: please add a 'using namespace WebCore' at the top of this file and remove the WebCore:: prefixes.

Comment on attachment 102912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102912&action=review >> Source/WebKit/chromium/public/WebMediaStreamTrack.h:32 >> +#include "WebString.h" > > WebString can just be forward declared. Done. >> Source/WebKit/chromium/public/WebMediaStreamTrack.h:50 >> + WebMediaStreamTrack(WTF::PassRefPtr<WebCore::MediaStreamTrack>); > > nit: parameter should be 'const Foo&' Done. >> Source/WebKit/chromium/public/WebMediaStreamTrack.h:58 >> +typedef WebVector<WebMediaStreamTrack> WebTrackVector; > > nit: the name of this type should be WebMediaStreamTrackVector. also, we normally create a unique header file for each top-level type, so this should go into a WebMediaStreamTrackVector.h all by its lonesome self. > > but i have to wonder... what is the difference between a WebMediaStreamTrackVector and a WebMediaStreamTrackList? why do we need two container types for WebMediaStreamTrack objects? This was just to prettify the constructor and usage of WebMediaStreamTrackList. Removed. >> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:49 >> + WebMediaStreamTrackList(WTF::PassRefPtr<WebCore::MediaStreamTrackList>); > > nit: parameter should be 'const Foo&' Done. >> Source/WebKit/chromium/src/WebMediaStreamTrackList.cpp:42 >> + m_private = WebCore::MediaStreamTrackList::create(tracks); > > nit: please add a 'using namespace WebCore' at the top of this file and remove the WebCore:: prefixes. Done.

Created attachment 103223 [details] Patch

Comment on attachment 103223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103223&action=review > Source/WebKit/chromium/public/WebMediaStreamTrack.h:32 > +#include "WebVector.h" nit: this include is not necessary. > Source/WebKit/chromium/public/WebMediaStreamTrack.h:47 > + WEBKIT_EXPORT void set(const WebString& id, const WebString& kind, const WebString& label); since this performs and allocation, i'd probably name the method 'initialize()'. that'd be more consistent with other WebKit APIs too. perhaps you should have an isNull() function too? see for example WebHistoryItem, which similarly wraps a reference counted WebCore class. > Source/WebKit/chromium/public/WebMediaStreamTrackList.h:31 > +#include "WebMediaStreamTrack.h" WebMediaStreamTrack can just be forward declared, right? > Source/WebKit/chromium/public/WebMediaStreamTrackList.h:33 > + this one needs the WebVector include, or at least a forward declaration. forward declaration is more common in WebKit APIs. template <typename T> class WebVector; > Source/WebKit/chromium/public/WebMediaStreamTrackList.h:45 > + WEBKIT_EXPORT void set(const WebVector<WebMediaStreamTrack>& webTracks); nit: same suggestion. this method should probably be named initialize() and there should be an isNull() method.

Comment on attachment 103223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103223&action=review >> Source/WebKit/chromium/public/WebMediaStreamTrack.h:32 >> +#include "WebVector.h" > > nit: this include is not necessary. Done. >> Source/WebKit/chromium/public/WebMediaStreamTrack.h:47 >> + WEBKIT_EXPORT void set(const WebString& id, const WebString& kind, const WebString& label); > > since this performs and allocation, i'd probably name the method 'initialize()'. that'd be more consistent with other WebKit APIs too. > > perhaps you should have an isNull() function too? see for example WebHistoryItem, which similarly wraps a reference counted WebCore class. Done. >> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:31 >> +#include "WebMediaStreamTrack.h" > > WebMediaStreamTrack can just be forward declared, right? Done. >> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:33 >> + > > this one needs the WebVector include, or at least a forward declaration. forward declaration is more common in WebKit APIs. > > template <typename T> class WebVector; Done. >> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:45 >> + WEBKIT_EXPORT void set(const WebVector<WebMediaStreamTrack>& webTracks); > > nit: same suggestion. this method should probably be named initialize() and there should be an isNull() method. Done.

Created attachment 103345 [details] Patch

Comment on attachment 103345 [details] Patch R=me I still feel a bit uncomfortable with this change since it is adding unused code. The big picture isn't at all clear to me. Perhaps when you add a patch that makes use of these new classes, things will start to make more sense to me. I might end up suggesting a very different approach at that point.

Comment on attachment 103345 [details] Patch Clearing flags on attachment: 103345 Committed r92989: <http://trac.webkit.org/changeset/92989>

All reviewed patches have been landed. Closing bug.