Bug 90819 - Update Media Source to the new OO design
Summary: Update Media Source to the new OO design
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anna Cavender
URL:
Keywords:
Depends on: 91649 91771 91773 91775 93285 93302 93303 93406
Blocks: 80583
  Show dependency treegraph
 
Reported: 2012-07-09 13:56 PDT by Anna Cavender
Modified: 2017-07-18 08:29 PDT (History)
13 users (show)

See Also:


Attachments
Patch for discussion/reference only - not intended for review/commit (126.29 KB, patch)
2012-07-13 12:36 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Updating to ToT (126.27 KB, patch)
2012-07-13 13:13 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
revised with acolwell suggestions (129.51 KB, patch)
2012-07-17 10:13 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
update based on feedback (147.43 KB, patch)
2012-07-19 11:25 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (293.71 KB, application/zip)
2012-07-19 13:28 PDT, WebKit Review Bot
no flags Details
win build files and layout test expectations (153.57 KB, patch)
2012-07-19 14:30 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (308.96 KB, application/zip)
2012-07-19 15:49 PDT, WebKit Review Bot
no flags Details
another try at windows build files (163.13 KB, patch)
2012-07-19 16:12 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
fix typo in vcproj file - using bots to check windows build (163.22 KB, patch)
2012-07-20 10:31 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
testing windows build (164.66 KB, patch)
2012-07-20 11:30 PDT, Anna Cavender
annacc: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Cavender 2012-07-09 13:56:18 PDT
The Media Source spec is changing to a more Object Oriented design and introducing separate objects: MediaSource, SourceBuffer, and SourceBufferList:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=17082

This uber-bug will track the WebKit changes needed to implement those changes.
Comment 1 Anna Cavender 2012-07-13 12:36:11 PDT
Created attachment 152320 [details]
Patch for discussion/reference only - not intended for review/commit
Comment 2 Anna Cavender 2012-07-13 12:58:04 PDT
Comment on attachment 152320 [details]
Patch for discussion/reference only - not intended for review/commit

I have a few questions inline.  Also, what is the best way handle the webkit- prefix for the new objects?

View in context: https://bugs.webkit.org/attachment.cgi?id=152320&action=review

> Source/WebCore/Modules/mediasource/MediaSource.h:100
> +    MediaPlayer* m_player; // <-- should this be a RefPtr?

Should we go ahead and make MediaPlayer RefCounted so that this can be a RefPtr (and also make HTMLMediaElement's m_player a RefPtr instead of an OwnPtr)?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:13971
> +				B1EF1A3515A3A1F4001682A6 /* MediaSource.cpp */,

I have never figured out how to properly add new files to xcode project files and I'm sure this is not the right way.  Pointers please!

> Source/WebCore/html/HTMLMediaElement.cpp:4287
> +        m_mediaSource->setMediaPlayer(m_player.get());

This seems a little suspect.  Other ideas how to handle this case?

> Source/WebCore/html/HTMLMediaElement.h:599
> +    RefPtr<MediaSource> m_mediaSource;

Should this be OwnPtr?

> LayoutTests/http/tests/media/media-source/media-source.js:280
> +        if (videoTag.networkState != HTMLMediaElement.NETWORK_LOADING &&

This check was added to ensure that the video element is in an appropriate state when the webkitsoureopen event is fired.  Because webkitsourceopen and loadstart are now on two different objects, we can't guarantee the order with which they will be fired. So, I've removed waitForEvent(loadstart) from all of the media-source LayoutTests and added this additional check here.
Comment 3 Anna Cavender 2012-07-13 13:13:18 PDT
Created attachment 152322 [details]
Updating to ToT
Comment 4 Aaron Colwell 2012-07-13 19:12:42 PDT
Comment on attachment 152322 [details]
Updating to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=152322&action=review

Here are my initial comments on the C++ changes. Overall I think this is looking pretty good. I'll review LayoutTest changes next week.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:61
> +        return SourceBufferList::create(scriptExecutionContext()).get();

Why create a new SourceBufferList in this case instead of just returning m_sourceBuffers? Say someone wants to attach event handlers to the list before the source is opened. I think if they try that they'll never get the events when the source is opened and new SourceBuffers are added.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:115
> +    if (!m_sourceBuffers->remove(buffer)) {

You should probably add a clear() method on SourceBuffer to clear the reference to this MediaSource so that it can't be used once it is removed.

> Source/WebCore/Modules/mediasource/MediaSource.idl:45
> +        // readonly attribute SourceBufferList activeSourceBuffers;

For now we could assume that if the MediaPlayer accepts the new source it is considered active. Once we have the ability to select tracks, we could add the infrastructure to signal when SourceBuffers should be added/removed from this list.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:44
> +    , m_type(type)

It doesn't look like you use m_type anywhere so I think it can be removed.

> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:79
> +        createAndFireEvent(eventNames().webkitremovesourcebufferEvent);

This won't cause JavaScript to run while in this loop right?

> Source/WebCore/html/HTMLMediaElement.cpp:149
>  static const char* mediaSourceURLProtocol = "x-media-source";

I think this patch should also rip out the old code. I don't think there is a good reason to keep both versions of the API in place at the same time.

> Source/WebCore/html/HTMLMediaElement.cpp:937
> +        m_mediaSource = adoptRef(MediaSourceRegistry::registry().lookupMediaSource(url.string()));

If m_mediaSource is already set, will this cause a closed event to fire on the old object? At a minimum I think you'll need to have the old object release its reference to MediaPlayer so the old source can't mess with playback.
Comment 5 Anna Cavender 2012-07-17 10:08:32 PDT
Comment on attachment 152322 [details]
Updating to ToT

Thanks for the speedy feedback!!

View in context: https://bugs.webkit.org/attachment.cgi?id=152322&action=review

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:61
>> +        return SourceBufferList::create(scriptExecutionContext()).get();
> 
> Why create a new SourceBufferList in this case instead of just returning m_sourceBuffers? Say someone wants to attach event handlers to the list before the source is opened. I think if they try that they'll never get the events when the source is opened and new SourceBuffers are added.

Good call.  Fixed.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:115
>> +    if (!m_sourceBuffers->remove(buffer)) {
> 
> You should probably add a clear() method on SourceBuffer to clear the reference to this MediaSource so that it can't be used once it is removed.

Very good point.  I originally thought the RefPtr would take care of that but forgot that JS could have a handle on the SourceBuffer object.  Thanks!  I put the call to clear in SourceBufferList::remove()

>> Source/WebCore/Modules/mediasource/MediaSource.idl:45
>> +        // readonly attribute SourceBufferList activeSourceBuffers;
> 
> For now we could assume that if the MediaPlayer accepts the new source it is considered active. Once we have the ability to select tracks, we could add the infrastructure to signal when SourceBuffers should be added/removed from this list.

OK.  I'll make activeSourceBuffers just return sourceBuffers for now.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:44
>> +    , m_type(type)
> 
> It doesn't look like you use m_type anywhere so I think it can be removed.

Done.

>> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:79
>> +        createAndFireEvent(eventNames().webkitremovesourcebufferEvent);
> 
> This won't cause JavaScript to run while in this loop right?

I'm not sure what you mean.  The events should be dispatched one by one during the loop.

>> Source/WebCore/html/HTMLMediaElement.cpp:149
>>  static const char* mediaSourceURLProtocol = "x-media-source";
> 
> I think this patch should also rip out the old code. I don't think there is a good reason to keep both versions of the API in place at the same time.

OK.  Done.

>> Source/WebCore/html/HTMLMediaElement.cpp:937
>> +        m_mediaSource = adoptRef(MediaSourceRegistry::registry().lookupMediaSource(url.string()));
> 
> If m_mediaSource is already set, will this cause a closed event to fire on the old object? At a minimum I think you'll need to have the old object release its reference to MediaPlayer so the old source can't mess with playback.

Good thinking.  Added a check here and set an already existing m_mediaSource to "closed", which will release its reference to the MediaPlayer.
Comment 6 Anna Cavender 2012-07-17 10:13:12 PDT
Created attachment 152776 [details]
revised with acolwell suggestions
Comment 7 Build Bot 2012-07-17 10:42:40 PDT
Comment on attachment 152776 [details]
revised with acolwell suggestions

Attachment 152776 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13279185
Comment 8 WebKit Review Bot 2012-07-17 10:44:46 PDT
Comment on attachment 152776 [details]
revised with acolwell suggestions

Attachment 152776 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13275207
Comment 9 Aaron Colwell 2012-07-17 11:29:42 PDT
Comment on attachment 152776 [details]
revised with acolwell suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=152776&action=review

This is looking pretty good to me. eric.carlson probably needs to start chiming in at this point.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:66
> +    return m_sourceBuffers.get();

Eventhough these lists will contain the same objects right now, I think we should still have different list objects so that you can't add an event handler to sourceBuffers() and remove it from activeSourceBuffers(). I believe that would be possible with this code.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:96
> +    RefPtr<SourceBuffer> buffer = SourceBuffer::create(String::number(++m_nextSourceBufferId));

Could this be moved into the MediaPlayer::Ok case and the MediaSource be passed to SourceBuffer::create() so we don't need setMediaSource()?

> Source/WebCore/Modules/mediasource/MediaSource.cpp:119
> +

I think you need a return 0 here.

> Source/WebCore/Modules/mediasource/MediaSource.idl:57
> +        attribute EventListener onwebkitsourceopen;

This and the 2 below should be removed. onwebkitsourceXXX attributes were removed from the spec. I think the DEFINE_ATTRIBUTE_EVENT_LISTENER() can be removed from the .h file as well.

> Source/WebCore/Modules/mediasource/SourceBufferList.h:60
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitaddsourcebuffer);

I believe these can be removed since the onxxx attributes were removed from the spec.

> Source/WebCore/Modules/mediasource/SourceBufferList.idl:43
> +        attribute EventListener onwebkitaddsourcebuffer;

please remove.

> Source/WebCore/html/HTMLMediaElement.cpp:933
> +            mediaLoadingFailed(MediaPlayer::FormatError);

I don't think this is quite right. It is possible to have blob: URLs that are not MediaSource related. WebRTC local camera preview is one example. I believe the correct thing to do is just drop this else block. You might want to test with other types of blob: URLs just to make sure.

> LayoutTests/http/tests/media/media-source/media-source.js:3
> +var sourceBuffer = null;

I think sourceBuffer should be a member of SegmentHelper and all references converted to this.sourceBuffer. I also think you should be able to remove this.SourceID now since the API doesn't use source IDs.

> LayoutTests/http/tests/media/media-source/media-source.js:128
>      this.videoTag.webkitSourceAddId(this.SourceID, this.segmentInfo.type);

Can this addSourceId() method go away?

> LayoutTests/http/tests/media/media-source/media-source.js:148
> +    if (!sourceBuffer.append) {

I think this should be !this.sourceBuffer since this check is just here to make sure that the MediaSource API is present. sourceBuffer should be null if that was the case.

> LayoutTests/http/tests/media/media-source/video-media-source-abort.html:20
> +                    sourceBuffer.append(partialInitSegment);

I think calls like this should be converted to segmentHelper.sourceBuffer.append(). same goes for abort()
Comment 10 Gyuyoung Kim 2012-07-17 18:03:30 PDT
Comment on attachment 152776 [details]
revised with acolwell suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=152776&action=review

In my humble opinion, there are a little nits. Please take a look them.

> Source/WebCore/ChangeLog:3
> +        Convert Media Source to new object-oriented API

Bug title is different from this bug.

> Source/WebCore/GNUmakefile.list.am:1173
> +  Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp \

Nit : wrong indentation.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:236
> +     if (!m_player->sourceAppend(id, data->data(), data->length())) {

Nit : wrong indentation.

> Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2011 -> 2012 ?

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:35
> +#include "SourceBuffer.h"

It seems header file of implementation file is just placed below #include "config.h" generally.

> Source/WebCore/Modules/mediasource/SourceBuffer.h:67
> +    SourceBuffer(const String& id);

Don't you need to use *explicit* keyword as well ?

> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:35
> +#include "SourceBufferList.h"

ditto.

> Source/WebCore/Modules/mediasource/SourceBufferList.h:35
> +

Nit : There is an unneeded line.

> Source/WebCore/dom/EventTarget.h:60
> +    class MediaSource;

Nit : wrong alphabetical order.
Comment 11 Eric Carlson 2012-07-18 11:23:48 PDT
Comment on attachment 152776 [details]
revised with acolwell suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=152776&action=review

> Source/WebCore/ChangeLog:22
> +        New Objects:
> +        * Modules/mediasource/MediaSource.cpp: Added.

I don't think it is necessary to list the method names of newly added files.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:65
> +    // TODO(): support track selection

We usually use "FIXME:". I think it is also worth filing a bug and listing the number in the comment.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:72
> +    // 1. If type is null or an empty then throw an INVALID_ACCESS_ERR exception and
> +    // abort these steps.

If this is a comment from the spec, please make that clear by including a link to the appropriate section of the spec. Here and throughout the rest of the patch.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:149
> +    // 4. Remove track information from audioTracks, videoTracks, and textTracks for all tracks associated with sourceBuffer and fire a simple event named change on the modified lists.
> +
> +    // 5. If sourceBuffer is in activeSourceBuffers, then remove it from that list and fire a removesourcebuffer event on that object.

Please include FIXMEs with bug numbers for unimplemented steps.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:157
> +void MediaSource::setState(const String& state)

Why "setState" instead of "setReadyState"?

> Source/WebCore/Modules/mediasource/MediaSource.cpp:164
> +    String oldState = m_readyState;
> +    m_readyState = state;
> +
> +    if (m_readyState == oldState)
> +        return;

Nit: why update m_readyState if it does not change?

> Source/WebCore/Modules/mediasource/MediaSource.cpp:169
> +        dispatchEvent(Event::create(eventNames().webkitsourcecloseEvent, false, true));

Does the spec really call for synchronous event dispatch? If so, you are going to have to add a bunch of code to guard against bad things happening when an event listener does something causes reentrancy.

> Source/WebCore/Modules/mediasource/MediaSource.idl:50
> +        //enum State { "closed", "open", "ended" };
> +        //readonly attribute State readyState;

Why are these commented out?

> Source/WebCore/Modules/mediasource/MediaSource.idl:53
> +        // enum EndOfStreamError { "network", "decode" };

Ditto.

> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:81
> +    for (size_t i = 0; i < m_list.size(); ++i)
> +        createAndFireEvent(eventNames().webkitremovesourcebufferEvent);
> +    m_list.clear();

Is there any reason to not call In SourceBufferList::remove? If so, it seems like the behavior here should at least be equivalent to what it does: remove the buffer from the list, call buffer->clear(), and then fire the event. Currently this will fire the 'removesourcebuffer' events while the buffers are still in the list.

> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:89
> +    EventTarget::dispatchEvent(event);

Synchronous dispatch?

> Source/WebCore/html/DOMURL.cpp:68
> +    // Since WebWorkers cannot obtain MediaSource objects, we should be on the main thread.
> +    ASSERT(isMainThread());

Shouldn't this be the first thing checked?

> Source/WebCore/html/HTMLMediaElement.cpp:922
> +    // If this is a media source URL, fetch the media source from the blob url.

This comment just describes what the code does. It should be removed.
Comment 12 Anna Cavender 2012-07-19 11:24:29 PDT
Comment on attachment 152776 [details]
revised with acolwell suggestions

Thanks everyone for the feedback!  I'll upload a patch to address your suggestions, just for your reference, but I'll be splitting this patch into 3 smaller ones coming up shortly.

View in context: https://bugs.webkit.org/attachment.cgi?id=152776&action=review

>> Source/WebCore/ChangeLog:3
>> +        Convert Media Source to new object-oriented API
> 
> Bug title is different from this bug.

Done.

>> Source/WebCore/ChangeLog:22
>> +        * Modules/mediasource/MediaSource.cpp: Added.
> 
> I don't think it is necessary to list the method names of newly added files.

Done.

>> Source/WebCore/GNUmakefile.list.am:1173
>> +  Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp \
> 
> Nit : wrong indentation.

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:65
>> +    // TODO(): support track selection
> 
> We usually use "FIXME:". I think it is also worth filing a bug and listing the number in the comment.

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:66
>> +    return m_sourceBuffers.get();
> 
> Eventhough these lists will contain the same objects right now, I think we should still have different list objects so that you can't add an event handler to sourceBuffers() and remove it from activeSourceBuffers(). I believe that would be possible with this code.

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:72
>> +    // abort these steps.
> 
> If this is a comment from the spec, please make that clear by including a link to the appropriate section of the spec. Here and throughout the rest of the patch.

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:96
>> +    RefPtr<SourceBuffer> buffer = SourceBuffer::create(String::number(++m_nextSourceBufferId));
> 
> Could this be moved into the MediaPlayer::Ok case and the MediaSource be passed to SourceBuffer::create() so we don't need setMediaSource()?

Sure.  Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:119
>> +
> 
> I think you need a return 0 here.

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:149
>> +    // 5. If sourceBuffer is in activeSourceBuffers, then remove it from that list and fire a removesourcebuffer event on that object.
> 
> Please include FIXMEs with bug numbers for unimplemented steps.

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:157
>> +void MediaSource::setState(const String& state)
> 
> Why "setState" instead of "setReadyState"?

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:164
>> +        return;
> 
> Nit: why update m_readyState if it does not change?

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:169
>> +        dispatchEvent(Event::create(eventNames().webkitsourcecloseEvent, false, true));
> 
> Does the spec really call for synchronous event dispatch? If so, you are going to have to add a bunch of code to guard against bad things happening when an event listener does something causes reentrancy.

No, that was a mistake.  I will change all dispatching to be non-cancelable.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:236
>> +     if (!m_player->sourceAppend(id, data->data(), data->length())) {
> 
> Nit : wrong indentation.

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.idl:50
>> +        //readonly attribute State readyState;
> 
> Why are these commented out?

I just wanted to document what is in the spec even though it won't appear in the IDL.  I'll remove them.

>> Source/WebCore/Modules/mediasource/MediaSource.idl:53
>> +        // enum EndOfStreamError { "network", "decode" };
> 
> Ditto.

Ditto.

>> Source/WebCore/Modules/mediasource/MediaSource.idl:57
>> +        attribute EventListener onwebkitsourceopen;
> 
> This and the 2 below should be removed. onwebkitsourceXXX attributes were removed from the spec. I think the DEFINE_ATTRIBUTE_EVENT_LISTENER() can be removed from the .h file as well.

Oops thanks.  Done.

>> Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> 2011 -> 2012 ?

Done.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:35
>> +#include "SourceBuffer.h"
> 
> It seems header file of implementation file is just placed below #include "config.h" generally.

Done.

>> Source/WebCore/Modules/mediasource/SourceBuffer.h:67
>> +    SourceBuffer(const String& id);
> 
> Don't you need to use *explicit* keyword as well ?

Yes, but I've added a another parameter to the constructor, so not needed anymore.

>> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:35
>> +#include "SourceBufferList.h"
> 
> ditto.

done.

>> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:81
>> +    m_list.clear();
> 
> Is there any reason to not call In SourceBufferList::remove? If so, it seems like the behavior here should at least be equivalent to what it does: remove the buffer from the list, call buffer->clear(), and then fire the event. Currently this will fire the 'removesourcebuffer' events while the buffers are still in the list.

Good catch, calling remove instead.

>> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:89
>> +    EventTarget::dispatchEvent(event);
> 
> Synchronous dispatch?

I think I've addressed this by making the event non-cancelable.  Is that correct?

>> Source/WebCore/Modules/mediasource/SourceBufferList.h:35
>> +
> 
> Nit : There is an unneeded line.

Done.

>> Source/WebCore/Modules/mediasource/SourceBufferList.h:60
>> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitaddsourcebuffer);
> 
> I believe these can be removed since the onxxx attributes were removed from the spec.

Done.

>> Source/WebCore/Modules/mediasource/SourceBufferList.idl:43
>> +        attribute EventListener onwebkitaddsourcebuffer;
> 
> please remove.

Done.

>> Source/WebCore/dom/EventTarget.h:60
>> +    class MediaSource;
> 
> Nit : wrong alphabetical order.

Done.

>> Source/WebCore/html/DOMURL.cpp:68
>> +    ASSERT(isMainThread());
> 
> Shouldn't this be the first thing checked?

Sure.  Done.

>> Source/WebCore/html/HTMLMediaElement.cpp:922
>> +    // If this is a media source URL, fetch the media source from the blob url.
> 
> This comment just describes what the code does. It should be removed.

Gone.

>> Source/WebCore/html/HTMLMediaElement.cpp:933
>> +            mediaLoadingFailed(MediaPlayer::FormatError);
> 
> I don't think this is quite right. It is possible to have blob: URLs that are not MediaSource related. WebRTC local camera preview is one example. I believe the correct thing to do is just drop this else block. You might want to test with other types of blob: URLs just to make sure.

Good point, thanks.

>> LayoutTests/http/tests/media/media-source/media-source.js:3
>> +var sourceBuffer = null;
> 
> I think sourceBuffer should be a member of SegmentHelper and all references converted to this.sourceBuffer. I also think you should be able to remove this.SourceID now since the API doesn't use source IDs.

Done.

>> LayoutTests/http/tests/media/media-source/media-source.js:128
>>      this.videoTag.webkitSourceAddId(this.SourceID, this.segmentInfo.type);
> 
> Can this addSourceId() method go away?

Yep.  Gone.

>> LayoutTests/http/tests/media/media-source/media-source.js:148
>> +    if (!sourceBuffer.append) {
> 
> I think this should be !this.sourceBuffer since this check is just here to make sure that the MediaSource API is present. sourceBuffer should be null if that was the case.

Done.

>> LayoutTests/http/tests/media/media-source/video-media-source-abort.html:20
>> +                    sourceBuffer.append(partialInitSegment);
> 
> I think calls like this should be converted to segmentHelper.sourceBuffer.append(). same goes for abort()

Done.
Comment 13 Anna Cavender 2012-07-19 11:25:54 PDT
Created attachment 153313 [details]
update based on feedback
Comment 14 Build Bot 2012-07-19 12:00:03 PDT
Comment on attachment 153313 [details]
update based on feedback

Attachment 153313 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13277940
Comment 15 WebKit Review Bot 2012-07-19 13:28:47 PDT
Comment on attachment 153313 [details]
update based on feedback

Attachment 153313 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13280945

New failing tests:
http/tests/media/media-source/video-media-source-objects.html
Comment 16 WebKit Review Bot 2012-07-19 13:28:51 PDT
Created attachment 153335 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 17 Anna Cavender 2012-07-19 14:30:33 PDT
Created attachment 153345 [details]
win build files and layout test expectations
Comment 18 Build Bot 2012-07-19 14:58:50 PDT
Comment on attachment 153345 [details]
win build files and layout test expectations

Attachment 153345 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13284990
Comment 19 WebKit Review Bot 2012-07-19 15:49:12 PDT
Comment on attachment 153345 [details]
win build files and layout test expectations

Attachment 153345 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13272986

New failing tests:
http/tests/messaging/cross-domain-message-event-dispatch.html
Comment 20 WebKit Review Bot 2012-07-19 15:49:18 PDT
Created attachment 153366 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 21 Anna Cavender 2012-07-19 16:12:42 PDT
Created attachment 153368 [details]
another try at windows build files
Comment 22 Build Bot 2012-07-19 16:46:50 PDT
Comment on attachment 153368 [details]
another try at windows build files

Attachment 153368 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13304108
Comment 23 Anna Cavender 2012-07-20 10:31:41 PDT
Created attachment 153536 [details]
fix typo in vcproj file - using bots to check windows build
Comment 24 Anna Cavender 2012-07-20 11:30:14 PDT
Created attachment 153550 [details]
testing windows build

lets see if this one applies