Remove MediaSourcePrivate/SourceBufferPrivate

Remove MediaSourcePrivate/SourceBufferPrivate

These layers are just proxies for the Web* variants.

BUG=317353
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161859

[philipj_slow, 2013-11-10 16:19:42]

PTAL, with particular attention to lifetime and ownership, sine I could mess
that up without causing any tests to fail.

[acolwell GONE FROM CHROMIUM, 2013-11-11 18:06:54]

LGTM % comments.

Thanks for doing this.

https://codereview.chromium.org/61603006/diff/1/Source/core/html/HTMLMediaSou...
File Source/core/html/HTMLMediaSource.h (right):

https://codereview.chromium.org/61603006/diff/1/Source/core/html/HTMLMediaSou...
Source/core/html/HTMLMediaSource.h:62: virtual void
setWebAndOpen(PassOwnPtr<blink::WebMediaSource>) = 0;
nit: s/setWeb/setWebMediaSource/

https://codereview.chromium.org/61603006/diff/1/Source/modules/mediasource/So...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/61603006/diff/1/Source/modules/mediasource/So...
Source/modules/mediasource/SourceBuffer.cpp:366:
m_webSourceBuffer->removedFromMediaSource();
I believe you can & should clear m_webSourceBuffer after this call.

https://codereview.chromium.org/61603006/diff/1/Source/modules/mediasource/We...
File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):

https://codereview.chromium.org/61603006/diff/1/Source/modules/mediasource/We...
Source/modules/mediasource/WebKitSourceBuffer.cpp:78: WebTimeRanges webRanges =
m_webSourceBuffer->buffered();
WDYT about creating a constructor on TimeRanges that takes a const
WebTimeRanges& so we don't have to duplicate this code.

https://codereview.chromium.org/61603006/diff/1/public/platform/WebMimeRegist...
File public/platform/WebMimeRegistry.h (right):

https://codereview.chromium.org/61603006/diff/1/public/platform/WebMimeRegist...
public/platform/WebMimeRegistry.h:35: #include "WebString.h"
Why is this include needed now?

[philipj_slow, 2013-11-12 09:43:50]

Aaron, can you take another look, in particular at the null check I didn't bring
over from SourceBufferPrivateImpl.cpp?

https://codereview.chromium.org/61603006/diff/1/Source/core/html/HTMLMediaSou...
File Source/core/html/HTMLMediaSource.h (right):

https://codereview.chromium.org/61603006/diff/1/Source/core/html/HTMLMediaSou...
Source/core/html/HTMLMediaSource.h:62: virtual void
setWebAndOpen(PassOwnPtr<blink::WebMediaSource>) = 0;
On 2013/11/11 18:06:55, acolwell wrote:
> nit: s/setWeb/setWebMediaSource/

I was hoping you would say that! Done!

https://codereview.chromium.org/61603006/diff/1/Source/modules/mediasource/So...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/61603006/diff/1/Source/modules/mediasource/So...
Source/modules/mediasource/SourceBuffer.cpp:366:
m_webSourceBuffer->removedFromMediaSource();
On 2013/11/11 18:06:55, acolwell wrote:
> I believe you can & should clear m_webSourceBuffer after this call.

I noticed the .clear() in the code that I removed, but brushed it aside a an
artifact of the glue layer. Thanks for paying attention when I didn't!

https://codereview.chromium.org/61603006/diff/1/Source/modules/mediasource/We...
File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):

https://codereview.chromium.org/61603006/diff/1/Source/modules/mediasource/We...
Source/modules/mediasource/WebKitSourceBuffer.cpp:78: WebTimeRanges webRanges =
m_webSourceBuffer->buffered();
On 2013/11/11 18:06:55, acolwell wrote:
> WDYT about creating a constructor on TimeRanges that takes a const
> WebTimeRanges& so we don't have to duplicate this code.

Yes, I thought about that as well. Done.

https://codereview.chromium.org/61603006/diff/1/Source/web/SourceBufferPrivat...
File Source/web/SourceBufferPrivateImpl.cpp (left):

https://codereview.chromium.org/61603006/diff/1/Source/web/SourceBufferPrivat...
Source/web/SourceBufferPrivateImpl.cpp:75: if (!m_sourceBuffer)
I also ignored this and the null check below when moving things around. Should I
also move this up into SourceBuffer.cpp? To me it looks rather strange that the
null check would be needed here but in e.g.
SourceBuffer::appendBufferAsyncPart(). Were these just paranoid null checks?

https://codereview.chromium.org/61603006/diff/1/public/platform/WebMimeRegist...
File public/platform/WebMimeRegistry.h (right):

https://codereview.chromium.org/61603006/diff/1/public/platform/WebMimeRegist...
public/platform/WebMimeRegistry.h:35: #include "WebString.h"
On 2013/11/11 18:06:55, acolwell wrote:
> Why is this include needed now?

The forward declaration is weird, because the concrete blink::WebString type is
used, not just pointers or references. I got into a situation when because I had
removed some header included before WebMimeRegistry.h, WebString.h was no longer
included first and it didn't compile. However, now it works with this reverted,
so presumably I moved headers more after the fix to make this change
unnecessary. Reverting.

[acolwell GONE FROM CHROMIUM, 2013-11-12 18:25:17]

lgtm 2

https://codereview.chromium.org/61603006/diff/1/Source/web/SourceBufferPrivat...
File Source/web/SourceBufferPrivateImpl.cpp (left):

https://codereview.chromium.org/61603006/diff/1/Source/web/SourceBufferPrivat...
Source/web/SourceBufferPrivateImpl.cpp:75: if (!m_sourceBuffer)
On 2013/11/12 09:43:51, philipj wrote:
> I also ignored this and the null check below when moving things around. Should
I
> also move this up into SourceBuffer.cpp? To me it looks rather strange that
the
> null check would be needed here but in e.g.
> SourceBuffer::appendBufferAsyncPart(). Were these just paranoid null checks?

I don't think the null check is necessary anymore because of the isRemoved()
check in SourceBuffer::setAppendWindowStart(). This was likely just overly
paranoid checking in this layer.

I believe the appendBufferAsyncPart() should never be called when
m_webSourceBuffer is called because abortIfUpdating() is called before the
pointer is cleared. This should prevent m_appendBufferAsyncPartRunner from
calling that method if a call is pending.

https://codereview.chromium.org/61603006/diff/100001/Source/core/html/TimeRan...
File Source/core/html/TimeRangesTest.cpp (right):

https://codereview.chromium.org/61603006/diff/100001/Source/core/html/TimeRan...
Source/core/html/TimeRangesTest.cpp:66: blink::WebTimeRanges
webRanges(static_cast<size_t>(2));
nit: s/static_cast<size_t>(2)/2u/ ?

[philipj_slow, 2013-11-12 19:47:58]

Thanks Aaron, handing over to commit queue now...

https://codereview.chromium.org/61603006/diff/1/Source/web/SourceBufferPrivat...
File Source/web/SourceBufferPrivateImpl.cpp (left):

https://codereview.chromium.org/61603006/diff/1/Source/web/SourceBufferPrivat...
Source/web/SourceBufferPrivateImpl.cpp:75: if (!m_sourceBuffer)
On 2013/11/12 18:25:17, acolwell wrote:
> On 2013/11/12 09:43:51, philipj wrote:
> > I also ignored this and the null check below when moving things around.
Should
> I
> > also move this up into SourceBuffer.cpp? To me it looks rather strange that
> the
> > null check would be needed here but in e.g.
> > SourceBuffer::appendBufferAsyncPart(). Were these just paranoid null checks?
> 
> I don't think the null check is necessary anymore because of the isRemoved()
> check in SourceBuffer::setAppendWindowStart(). This was likely just overly
> paranoid checking in this layer.
> 
> I believe the appendBufferAsyncPart() should never be called when
> m_webSourceBuffer is called because abortIfUpdating() is called before the
> pointer is cleared. This should prevent m_appendBufferAsyncPartRunner from
> calling that method if a call is pending.

Thanks for verifying!

https://codereview.chromium.org/61603006/diff/100001/Source/core/html/TimeRan...
File Source/core/html/TimeRangesTest.cpp (right):

https://codereview.chromium.org/61603006/diff/100001/Source/core/html/TimeRan...
Source/core/html/TimeRangesTest.cpp:66: blink::WebTimeRanges
webRanges(static_cast<size_t>(2));
On 2013/11/12 18:25:17, acolwell wrote:
> nit: s/static_cast<size_t>(2)/2u/ ?

Unfortunately, GCC then doesn't pick the correct constructor:

In file included from
../../third_party/WebKit/public/platform/WebTimeRange.h:34:0,
                 from ../../third_party/WebKit/Source/core/html/TimeRanges.h:30,
                 from
../../third_party/WebKit/Source/core/html/TimeRangesTest.cpp:32:
../../third_party/WebKit/public/platform/WebVector.h: In instantiation of
'blink::WebVector<T>::WebVector(const C&) [with C = unsigned int; T =
blink::WebTimeRange]':
../../third_party/WebKit/Source/core/html/TimeRangesTest.cpp:66:38:   required
from here
../../third_party/WebKit/public/platform/WebVector.h:90:9: error: subscripted
value is neither array nor pointer
../../third_party/WebKit/public/platform/WebVector.h:90:9: error: request for
member 'size' in 'other', which is of non-class type 'const unsigned int'
../../third_party/WebKit/public/platform/WebVector.h:90:9: error: request for
member 'size' in 'other', which is of non-class type 'const unsigned int'

[commit-bot: I haz the power, 2013-11-12 19:48:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/61603006/100001

[commit-bot: I haz the power, 2013-11-12 19:49:02]

Failed to apply patch for Source/modules/mediasource/SourceBuffer.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/mediasource/SourceBuffer.cpp
  Hunk #1 FAILED at 39.
  Hunk #2 succeeded at 74 (offset -1 lines).
  Hunk #3 succeeded at 97 (offset -1 lines).
  Hunk #4 succeeded at 126 (offset -1 lines).
  Hunk #5 succeeded at 166 (offset -1 lines).
  Hunk #6 succeeded at 196 (offset -1 lines).
  Hunk #7 succeeded at 259 (offset -1 lines).
  Hunk #8 succeeded at 354 (offset -1 lines).
  Hunk #9 succeeded at 460 (offset -1 lines).
  Hunk #10 succeeded at 488 (offset -1 lines).
  Hunk #11 succeeded at 527 (offset -1 lines).
  Hunk #12 succeeded at 590 (offset -1 lines).
  Hunk #13 succeeded at 621 (offset -1 lines).
  1 out of 13 hunks FAILED -- saving rejects to file
Source/modules/mediasource/SourceBuffer.cpp.rej

Patch:       Source/modules/mediasource/SourceBuffer.cpp
Index: Source/modules/mediasource/SourceBuffer.cpp
diff --git a/Source/modules/mediasource/SourceBuffer.cpp
b/Source/modules/mediasource/SourceBuffer.cpp
index
3552d4d2fcc1662acf03cc17227226c5e287bfc9..e0f3758cbd8ba472eafadb173e821eed2ffd7e7a
100644
--- a/Source/modules/mediasource/SourceBuffer.cpp
+++ b/Source/modules/mediasource/SourceBuffer.cpp
@@ -39,28 +39,30 @@
 #include "core/fileapi/FileReaderLoader.h"
 #include "core/fileapi/Stream.h"
 #include "core/html/TimeRanges.h"
-#include "core/platform/graphics/SourceBufferPrivate.h"
 #include "modules/mediasource/MediaSource.h"
 #include "platform/Logging.h"
 #include "platform/TraceEvent.h"
+#include "public/platform/WebSourceBuffer.h"
 #include "wtf/ArrayBuffer.h"
 #include "wtf/ArrayBufferView.h"
 #include "wtf/MathExtras.h"
 
 #include <limits>
 
+using blink::WebSourceBuffer;
+
 namespace WebCore {
 
-PassRefPtr<SourceBuffer> SourceBuffer::create(PassOwnPtr<SourceBufferPrivate>
sourceBufferPrivate, MediaSource* source, GenericEventQueue* asyncEventQueue)
+PassRefPtr<SourceBuffer> SourceBuffer::create(PassOwnPtr<WebSourceBuffer>
webSourceBuffer, MediaSource* source, GenericEventQueue* asyncEventQueue)
 {
-    RefPtr<SourceBuffer> sourceBuffer(adoptRef(new
SourceBuffer(sourceBufferPrivate, source, asyncEventQueue)));
+    RefPtr<SourceBuffer> sourceBuffer(adoptRef(new
SourceBuffer(webSourceBuffer, source, asyncEventQueue)));
     sourceBuffer->suspendIfNeeded();
     return sourceBuffer.release();
 }
 
-SourceBuffer::SourceBuffer(PassOwnPtr<SourceBufferPrivate> sourceBufferPrivate,
MediaSource* source, GenericEventQueue* asyncEventQueue)
+SourceBuffer::SourceBuffer(PassOwnPtr<WebSourceBuffer> webSourceBuffer,
MediaSource* source, GenericEventQueue* asyncEventQueue)
     : ActiveDOMObject(source->executionContext())
-    , m_private(sourceBufferPrivate)
+    , m_webSourceBuffer(webSourceBuffer)
     , m_source(source)
     , m_asyncEventQueue(asyncEventQueue)
     , m_updating(false)
@@ -75,7 +77,7 @@ SourceBuffer::SourceBuffer(PassOwnPtr<SourceBufferPrivate>
sourceBufferPrivate,
     , m_streamMaxSize(0)
     , m_appendStreamAsyncPartRunner(this, &SourceBuffer::appendStreamAsyncPart)
 {
-    ASSERT(m_private);
+    ASSERT(m_webSourceBuffer);
     ASSERT(m_source);
     ScriptWrappable::init(this);
 }
@@ -98,7 +100,7 @@ PassRefPtr<TimeRanges> SourceBuffer::buffered(ExceptionState&
es) const
     }
 
     // 2. Return a new static normalized TimeRanges object for the media
segments buffered.
-    return m_private->buffered();
+    return TimeRanges::create(m_webSourceBuffer->buffered());
 }
 
 double SourceBuffer::timestampOffset() const
@@ -127,7 +129,7 @@ void SourceBuffer::setTimestampOffset(double offset,
ExceptionState& es)
     // and abort these steps.
     //
     // FIXME: Add step 6 text when mode attribute is implemented.
-    if (!m_private->setTimestampOffset(offset)) {
+    if (!m_webSourceBuffer->setTimestampOffset(offset)) {
         es.throwUninformativeAndGenericDOMException(InvalidStateError);
         return;
     }
@@ -167,7 +169,7 @@ void SourceBuffer::setAppendWindowStart(double start,
ExceptionState& es)
         return;
     }
 
-    m_private->setAppendWindowStart(start);
+    m_webSourceBuffer->setAppendWindowStart(start);
 
     // 4. Update the attribute to the new value.
     m_appendWindowStart = start;
@@ -197,7 +199,7 @@ void SourceBuffer::setAppendWindowEnd(double end,
ExceptionState& es)
         return;
     }
 
-    m_private->setAppendWindowEnd(end);
+    m_webSourceBuffer->setAppendWindowEnd(end);
 
     // 5. Update the attribute to the new value.
     m_appendWindowEnd = end;
@@ -260,7 +262,7 @@ void SourceBuffer::abort(ExceptionState& es)
     abortIfUpdating();
 
     // 4. Run the reset parser state algorithm.
-    m_private->abort();
+    m_webSourceBuffer->abort();
 
     // 5. Set appendWindowStart to 0.
     setAppendWindowStart(0, es);
@@ -355,7 +357,8 @@ void SourceBuffer::removedFromMediaSource()
 
     abortIfUpdating();
 
-    m_private->removedFromMediaSource();
+    m_webSourceBuffer->removedFromMediaSource();
+    m_webSourceBuffer.clear();
     m_source = 0;
     m_asyncEventQueue = 0;
 }
@@ -460,11 +463,11 @@ void SourceBuffer::appendBufferAsyncPart()
     size_t appendSize = m_pendingAppendData.size();
     if (!appendSize) {
         // Resize buffer for 0 byte appends so we always have a valid pointer.
-        // We need to convey all appends, even 0 byte ones to |m_private| so
-        // that it can clear its end of stream state if necessary.
+        // We need to convey all appends, even 0 byte ones to
|m_webSourceBuffer|
+        // so that it can clear its end of stream state if necessary.
         m_pendingAppendData.resize(1);
     }
-    m_private->append(m_pendingAppendData.data(), appendSize);
+    m_webSourceBuffer->append(m_pendingAppendData.data(), appendSize);
 
     // 3. Set the updating attribute to false.
     m_updating = false;
@@ -488,7 +491,7 @@ void SourceBuffer::removeAsyncPart()
     //
https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source....
 
     // 9. Run the coded frame removal algorithm with start and end as the start
and end of the removal range.
-    m_private->remove(m_pendingRemoveStart, m_pendingRemoveEnd);
+    m_webSourceBuffer->remove(m_pendingRemoveStart, m_pendingRemoveEnd);
 
     // 10. Set the updating attribute to false.
     m_updating = false;
@@ -527,7 +530,7 @@ void SourceBuffer::appendStreamInternal(PassRefPtr<Stream>
stream, ExceptionStat
     //  3. If the readyState attribute of the parent media source is in the
"ended" state then run the following steps: ...
     m_source->openIfInEndedState();
 
-    //  Steps 4-5 of the prepare append algorithm are handled by m_private.
+    // Steps 4-5 of the prepare append algorithm are handled by
m_webSourceBuffer.
 
     // 3. Set the updating attribute to true.
     m_updating = true;
@@ -590,7 +593,7 @@ void SourceBuffer::appendStreamDone(bool success)
     }
 
     // Section 3.5.6 Stream Append Loop
-    // Steps 1-11 are handled by appendStreamAsyncPart(), |m_loader|, and
|m_private|.
+    // Steps 1-11 are handled by appendStreamAsyncPart(), |m_loader|, and
|m_webSourceBuffer|.
     // 12. Loop Done: Set the updating attribute to false.
     m_updating = false;
 
@@ -621,7 +624,7 @@ void SourceBuffer::didReceiveDataForClient(const char* data,
unsigned dataLength
     ASSERT(m_updating);
     ASSERT(m_loader);
 
-    m_private->append(reinterpret_cast<const unsigned char*>(data),
dataLength);
+    m_webSourceBuffer->append(reinterpret_cast<const unsigned char*>(data),
dataLength);
 }
 
 void SourceBuffer::didFinishLoading()

[philipj_slow, 2013-11-12 20:18:42]

Adam, do you approve?

[philipj_slow, 2013-11-12 20:18:42]

Adam, do you approve?

[adamk, 2013-11-12 20:59:19]

lgtm with a few ASSERTS.

I very much approve!

https://codereview.chromium.org/61603006/diff/190001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSourceBase.cpp (right):

https://codereview.chromium.org/61603006/diff/190001/Source/modules/mediasour...
Source/modules/mediasource/MediaSourceBase.cpp:276: case
WebMediaSource::AddStatusNotSupported:
Can you add ASSERT(!webSourceBuffer) here and...

https://codereview.chromium.org/61603006/diff/190001/Source/modules/mediasour...
Source/modules/mediasource/MediaSourceBase.cpp:283: case
WebMediaSource::AddStatusReachedIdLimit:
...here too? That gives us back some of the safety we lost when we got rid of
the OwnPtr

[philipj_slow, 2013-11-12 22:13:46]

That's it, thanks Aaron and Adam!

https://codereview.chromium.org/61603006/diff/190001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSourceBase.cpp (right):

https://codereview.chromium.org/61603006/diff/190001/Source/modules/mediasour...
Source/modules/mediasource/MediaSourceBase.cpp:276: case
WebMediaSource::AddStatusNotSupported:
On 2013/11/12 20:59:19, adamk wrote:
> Can you add ASSERT(!webSourceBuffer) here and...

Done.

https://codereview.chromium.org/61603006/diff/190001/Source/modules/mediasour...
Source/modules/mediasource/MediaSourceBase.cpp:283: case
WebMediaSource::AddStatusReachedIdLimit:
On 2013/11/12 20:59:19, adamk wrote:
> ...here too? That gives us back some of the safety we lost when we got rid of
> the OwnPtr

Done.

[commit-bot: I haz the power, 2013-11-12 22:15:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/61603006/250001

[commit-bot: I haz the power, 2013-11-12 23:53:24]

Change committed as 161859