Bug 60260 – [Chromium] Implement the embedders for the HTML5 Track List objects

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Leandro Graciá Gil]

Implement in WebKit Chromium the WebTrackList, WebExclusiveTrackList and WebMultipleTrackList embedder objects for their WebCore counterparts.

[Leandro Graciá Gil]

Created attachment 92398 [details]
Patch

[Tony Gentilcore]

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

[WebKit Review Bot]

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

[Tommy Widenflycht]

Created attachment 95564 [details]
Patch

[Tommy Widenflycht]

Created attachment 96411 [details]
Patch

[Tommy Widenflycht]

Changed platform to All/All

[Tony Gentilcore]

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

[Tommy Widenflycht]

Created attachment 97260 [details]
Patch

[Darin Fisher (:fishd, Google)]

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!

[Tommy Widenflycht]

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!

[Darin Fisher (:fishd, Google)]

(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.

[Tommy Widenflycht]

(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.

[Tommy Widenflycht]

Created attachment 102908 [details]
Patch

[Leandro Graciá Gil]

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.

[Tommy Widenflycht]

Created attachment 102912 [details]
Patch

[Tommy Widenflycht]

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.

[Darin Fisher (:fishd, Google)]

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.

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 103223 [details]
Patch

[Darin Fisher (:fishd, Google)]

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.

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 103345 [details]
Patch

[Darin Fisher (:fishd, Google)]

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.

[WebKit Review Bot]

Comment on attachment 103345 [details]
Patch

Clearing flags on attachment: 103345

Committed r92989: <http://trac.webkit.org/changeset/92989>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.