Bug 60260 - [Chromium] Implement the embedders for the HTML5 Track List objects
Summary: [Chromium] Implement the embedders for the HTML5 Track List objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on: 60184
Blocks: 56459
  Show dependency treegraph
 
Reported: 2011-05-05 03:03 PDT by Leandro Graciá Gil
Modified: 2011-08-12 13:46 PDT (History)
7 users (show)

See Also:


Attachments
Patch (19.20 KB, patch)
2011-05-05 03:07 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (19.20 KB, patch)
2011-06-01 01:39 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (19.20 KB, patch)
2011-06-08 05:54 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (19.20 KB, patch)
2011-06-15 01:53 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (12.66 KB, patch)
2011-08-04 07:01 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (12.61 KB, patch)
2011-08-04 07:16 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (12.67 KB, patch)
2011-08-08 01:54 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (12.84 KB, patch)
2011-08-09 04:34 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2011-05-05 03:03:23 PDT
Implement in WebKit Chromium the WebTrackList, WebExclusiveTrackList and WebMultipleTrackList embedder objects for their WebCore counterparts.
Comment 1 Leandro Graciá Gil 2011-05-05 03:07:03 PDT
Created attachment 92398 [details]
Patch
Comment 2 Tony Gentilcore 2011-05-05 03:37:19 PDT
Adding fishd, who like to review all changes to the Chromium/WebKit API.
Comment 3 WebKit Review Bot 2011-05-05 03:53:06 PDT
Attachment 92398 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8556921
Comment 4 Tommy Widenflycht 2011-06-01 01:39:54 PDT
Created attachment 95564 [details]
Patch
Comment 5 Tommy Widenflycht 2011-06-08 05:54:36 PDT
Created attachment 96411 [details]
Patch
Comment 6 Tommy Widenflycht 2011-06-08 06:11:34 PDT
Changed platform to All/All
Comment 7 Tony Gentilcore 2011-06-08 10:03:41 PDT
Darin, would you like to review this one since it is a new chromium API?
Comment 8 Tommy Widenflycht 2011-06-15 01:53:53 PDT
Created attachment 97260 [details]
Patch
Comment 9 Darin Fisher (:fishd, Google) 2011-06-15 09:05:34 PDT
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!
Comment 10 Tommy Widenflycht 2011-06-20 04:59:23 PDT
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!
Comment 11 Darin Fisher (:fishd, Google) 2011-06-23 16:25:58 PDT
(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.
Comment 12 Tommy Widenflycht 2011-07-19 08:03:03 PDT
(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.
Comment 13 Tommy Widenflycht 2011-08-04 07:01:25 PDT
Created attachment 102908 [details]
Patch
Comment 14 Leandro Graciá Gil 2011-08-04 07:12:33 PDT
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.
Comment 15 Tommy Widenflycht 2011-08-04 07:16:36 PDT
Created attachment 102912 [details]
Patch
Comment 16 Tommy Widenflycht 2011-08-04 07:17:30 PDT
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 17 Darin Fisher (:fishd, Google) 2011-08-05 08:44:57 PDT
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 18 Tommy Widenflycht 2011-08-08 01:52:24 PDT
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.
Comment 19 Tommy Widenflycht 2011-08-08 01:54:54 PDT
Created attachment 103223 [details]
Patch
Comment 20 Darin Fisher (:fishd, Google) 2011-08-08 12:42:56 PDT
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 21 Tommy Widenflycht 2011-08-09 04:33:05 PDT
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.
Comment 22 Tommy Widenflycht 2011-08-09 04:34:02 PDT
Created attachment 103345 [details]
Patch
Comment 23 Darin Fisher (:fishd, Google) 2011-08-12 12:27:27 PDT
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 24 WebKit Review Bot 2011-08-12 13:46:29 PDT
Comment on attachment 103345 [details]
Patch

Clearing flags on attachment: 103345

Committed r92989: <http://trac.webkit.org/changeset/92989>
Comment 25 WebKit Review Bot 2011-08-12 13:46:35 PDT
All reviewed patches have been landed.  Closing bug.