Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14876007:
Add Blink side support for SourceBuffer.appendWindowStart & SourceBuffer.appendWindowEnd. (Closed)
|
Created:
7 years, 7 months ago by changbin Modified:
7 years, 4 months ago CC:
blink-reviews, abarth_chromum.org, danakj, feature-media-reviews_chromium.org, Stephen Chennney, jeez Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd Blink side support for SourceBuffer.appendWindowStart & SourceBuffer.appendWindowEnd.
Implement SourceBuffer.appendWindowStart & SourceBuffer.appendWindowEnd for Media Source Extensions Spec.
BUG=229408
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155891
Patch Set 1 #Patch Set 2 : rebase to add Blink side implementation of appendWindowStart and appendWindowEnd #Patch Set 3 : throw DOMException for set appendwindow error #
Total comments: 19
Patch Set 4 : fix nits of SourceBuffer.cpp and appendwindow Layoutest #Patch Set 5 : fix nits #Patch Set 6 : rebase #
Total comments: 3
Patch Set 7 : fix nits #Patch Set 8 : rebase #
Total comments: 5
Patch Set 9 : add comment code #Messages
Total messages: 29 (0 generated)
Hi Aaron, I am interested to implement appendWindow attributes for MSE spec, please kindly take a look at this patch. Changbin
Is this a separate feature which should go through "intent to implement" or is this part of an existing in-progress feature? Do you plan to guard this with a runtime flag?
On 2013/05/09 10:35:46, Eric Seidel wrote: > Is this a separate feature which should go through "intent to implement" or is > this part of an existing in-progress feature? Do you plan to guard this with a > runtime flag? I think this should be part of the effort to implement the unprefixed version of the MediaSource API. I just sent out a CL (https://codereview.chromium.org/14619016/) that moves the prefixed API so it will be easier to implement the unprefixed version. I also intend to send out an "Intent to Implement" for the unprefixed API later today. I'd like to defer this change until the rename CL above has landed and I have a chance to land a followup refactor that lays the groundwork for the unprefixed API. I just don't want to add more functionality to the prefixed API at this point. Does that sound reasonable? If you have Chromium side changes for this feature that modify ChunkDemuxer, WebMediaPlayerImpl etc... I'd be happy to review them in the meantime.
On 2013/05/09 17:29:05, acolwell wrote: > On 2013/05/09 10:35:46, Eric Seidel wrote: > > Is this a separate feature which should go through "intent to implement" or is > > this part of an existing in-progress feature? Do you plan to guard this with > a > > runtime flag? > > I think this should be part of the effort to implement the unprefixed version of > the MediaSource API. I just sent out a CL > (https://codereview.chromium.org/14619016/) that moves the prefixed API so it > will be easier to implement the unprefixed version. I also intend to send out an > "Intent to Implement" for the unprefixed API later today. > > I'd like to defer this change until the rename CL above has landed and I have a > chance to land a followup refactor that lays the groundwork for the unprefixed > API. I just don't want to add more functionality to the prefixed API at this > point. Does that sound reasonable? > > If you have Chromium side changes for this feature that modify ChunkDemuxer, > WebMediaPlayerImpl etc... I'd be happy to review them in the meantime. Thanks, I am now working on Chromium side implementation. Will update this patch after your CL landed.
Rebase code and add layout test for appendwindow attribute. Dear acolwell, I am sorry that didn't spare time to work on implementation of the Chromium side due to other stuff.
Hi acolwell, could you please take a look at this CL?
lgtm % nits https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html (right): https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:16: var mediaType = 'video/webm;codecs="vp8,vorbis"'; nit: Move this to a higher scope and reused the value everywhere else it is needed below. https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:27: mediasource_test(function(test, mediaElement, mediaSource) nit: add a space to fix indent. https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:58: "set appendWindowStart throws an exception when less than 0."); nit: Test appendWindowStart with Number.NEGATIVE_INFINITY, Number.POSITIVE_INFINITY, and Number.NaN since those should not be allowed with a normal double. https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:61: function() { sourceBuffer.appendWindowEnd = "string"; }, nit:s/"string"/Number.NaN/ since the description explicitly indicates you are checking for NaN behavior. https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:73: "set appendWindowStart throws an exception when mediasource object is not associated with an buffer."); nit: s/an buffer/a buffer/ https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:77: "set appendWindowEnd throws an exception when mediasource object is not associated with an buffer."); nit: s/an buffer/a buffer/ https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:80: }, "Test appendwindow throw error when mediasource object is not associated with an sourebuffer."); nit:s/an sourebuffer/a sourcebuffer/ https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:108: assert_equals(sourceBuffer.appendWindowStart, 0, "appendWindowStart is 0 when abort'"); nit:s/when/after an/ https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:110: "appendWindowStart is POSITIVE_INFINITY when abort"); nit:s/when/after an/ https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:116: }, "Test appendWindowStart and appendWindowEnd value if sourceBuffer.abort()."); nit:s/if/after a/ https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... Source/modules/mediasource/SourceBuffer.cpp:130: // Section 3.1 appendWindowStart attribute setter steps. You should probably add code along these lines to enforce the 'restricted double' rules. // Enforce throwing an exception on restricted double values. if (std::isnan(start) || start == std::numeric_limits<double>::infinity() || start == -std::numeric_limits<double>::infinity()) { es.throwDOMException(TypeMismatchError); return; } https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... Source/modules/mediasource/SourceBuffer.cpp:138: // 2. If the updating attribute equals true, then throw an INVALID_STATE_ERR exception and abort these steps. nit: s/INVALID_STATE_ERR/InvalidStateError https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... Source/modules/mediasource/SourceBuffer.cpp:139: if (m_updating) { nit: This could be added to the conditional above. https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... Source/modules/mediasource/SourceBuffer.cpp:172: // 2. If the updating attribute equals true, then throw an INVALID_STATE_ERR exception and abort these steps. nit: s/INVALID_STATE_ERR/InvalidStateError https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... Source/modules/mediasource/SourceBuffer.cpp:173: if (m_updating) { nit: this could be added to the conditional above. https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... Source/modules/mediasource/SourceBuffer.cpp:178: // 3. If the new value equals NaN, then throw an INVALID_ACCESS_ERR and abort these steps. nit: s/INVALID_ACCESS_ERR/InvalidAccessError https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... Source/modules/mediasource/SourceBuffer.cpp:186: if (end <= m_appendWindowStart) { nit: This can be added to the conditional above. https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... Source/modules/mediasource/SourceBuffer.cpp:245: m_appendWindowStart = 0; nit: This doesn't notify m_private about this change. Change this to setAppendWindowStart(0, es) just so all changes to this variable go through the same codepath. https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... Source/modules/mediasource/SourceBuffer.cpp:248: m_appendWindowEnd = std::numeric_limits<double>::infinity(); nit: ditto. Call setAppendWindowEnd(std::numeric_limits<double>::infinity(), es) instead.
Thanks for the comments! Update the patch, please help to review. On 2013/08/06 18:10:33, acolwell wrote: > lgtm % nits > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > File LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html > (right): > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:16: var > mediaType = 'video/webm;codecs="vp8,vorbis"'; > nit: Move this to a higher scope and reused the value everywhere else it is > needed below. > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:27: > mediasource_test(function(test, mediaElement, mediaSource) > nit: add a space to fix indent. > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:58: "set > appendWindowStart throws an exception when less than 0."); > nit: Test appendWindowStart with Number.NEGATIVE_INFINITY, > Number.POSITIVE_INFINITY, and Number.NaN since those should not be allowed with > a normal double. > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:61: > function() { sourceBuffer.appendWindowEnd = "string"; }, > nit:s/"string"/Number.NaN/ since the description explicitly indicates you are > checking for NaN behavior. > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:73: "set > appendWindowStart throws an exception when mediasource object is not associated > with an buffer."); > nit: s/an buffer/a buffer/ > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:77: "set > appendWindowEnd throws an exception when mediasource object is not associated > with an buffer."); > nit: s/an buffer/a buffer/ > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:80: }, > "Test appendwindow throw error when mediasource object is not associated with an > sourebuffer."); > nit:s/an sourebuffer/a sourcebuffer/ > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:108: > assert_equals(sourceBuffer.appendWindowStart, 0, "appendWindowStart is 0 when > abort'"); > nit:s/when/after an/ > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:110: > "appendWindowStart is POSITIVE_INFINITY when abort"); > nit:s/when/after an/ > > https://codereview.chromium.org/14876007/diff/10001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:116: }, > "Test appendWindowStart and appendWindowEnd value if sourceBuffer.abort()."); > nit:s/if/after a/ > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > File Source/modules/mediasource/SourceBuffer.cpp (right): > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > Source/modules/mediasource/SourceBuffer.cpp:130: // Section 3.1 > appendWindowStart attribute setter steps. > You should probably add code along these lines to enforce the 'restricted > double' rules. > > // Enforce throwing an exception on restricted double values. > if (std::isnan(start) || > start == std::numeric_limits<double>::infinity() || > start == -std::numeric_limits<double>::infinity()) { > es.throwDOMException(TypeMismatchError); > return; > } > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > Source/modules/mediasource/SourceBuffer.cpp:138: // 2. If the updating attribute > equals true, then throw an INVALID_STATE_ERR exception and abort these steps. > nit: s/INVALID_STATE_ERR/InvalidStateError > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > Source/modules/mediasource/SourceBuffer.cpp:139: if (m_updating) { > nit: This could be added to the conditional above. > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > Source/modules/mediasource/SourceBuffer.cpp:172: // 2. If the updating attribute > equals true, then throw an INVALID_STATE_ERR exception and abort these steps. > nit: s/INVALID_STATE_ERR/InvalidStateError > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > Source/modules/mediasource/SourceBuffer.cpp:173: if (m_updating) { > nit: this could be added to the conditional above. > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > Source/modules/mediasource/SourceBuffer.cpp:178: // 3. If the new value equals > NaN, then throw an INVALID_ACCESS_ERR and abort these steps. > nit: s/INVALID_ACCESS_ERR/InvalidAccessError > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > Source/modules/mediasource/SourceBuffer.cpp:186: if (end <= m_appendWindowStart) > { > nit: This can be added to the conditional above. > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > Source/modules/mediasource/SourceBuffer.cpp:245: m_appendWindowStart = 0; > nit: This doesn't notify m_private about this change. Change this to > setAppendWindowStart(0, es) just so all changes to this variable go through the > same codepath. > > https://codereview.chromium.org/14876007/diff/10001/Source/modules/mediasourc... > Source/modules/mediasource/SourceBuffer.cpp:248: m_appendWindowEnd = > std::numeric_limits<double>::infinity(); > nit: ditto. Call setAppendWindowEnd(std::numeric_limits<double>::infinity(), es) > instead.
Hi acolwell, since there are many nits for patch #3, could you please help to review patch set #6?
lgtm % nits https://codereview.chromium.org/14876007/diff/22001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html (right): https://codereview.chromium.org/14876007/diff/22001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:79: sourceBuffer.appendBuffer(mediaData); nit: Do you actually need the appendBuffer() here? I think you should be able to remove it since it would be immediately aborted by the next line. https://codereview.chromium.org/14876007/diff/22001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:95: sourceBuffer.appendBuffer(mediaData); nit: You need a 'test.expectEvent(sourceBuffer, "updateend", "sourceBuffer");' before this line so that the waitForExpectedEvents() below actually waits for the append to complete. You are just getting lucky here. https://codereview.chromium.org/14876007/diff/22001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:115: sourceBuffer.appendBuffer(mediaData); nit: You need a 'test.expectEvent(sourceBuffer, "updateend", "sourceBuffer");' before this line too.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/changbin.shao@intel.com/14876007/34001
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 #2 FAILED at 62.
Hunk #3 succeeded at 125 (offset 1 line).
Hunk #4 succeeded at 237 with fuzz 1 (offset 1 line).
1 out of 4 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
3fdce7bcee1f58e8e536c1efd265d9a1d1643fb4..87ca2254cc1b9ed7eb288ded7dd2ce475aad0907
100644
--- a/Source/modules/mediasource/SourceBuffer.cpp
+++ b/Source/modules/mediasource/SourceBuffer.cpp
@@ -41,6 +41,9 @@
#include "modules/mediasource/MediaSource.h"
#include "wtf/ArrayBuffer.h"
#include "wtf/ArrayBufferView.h"
+#include "wtf/MathExtras.h"
+
+#include <limits>
namespace WebCore {
@@ -59,6 +62,8 @@ SourceBuffer::SourceBuffer(PassOwnPtr<SourceBufferPrivate>
sourceBufferPrivate,
, m_updating(false)
, m_timestampOffset(0)
, m_appendBufferTimer(this, &SourceBuffer::appendBufferTimerFired)
+ , m_appendWindowStart(0)
+ , m_appendWindowEnd(std::numeric_limits<double>::infinity())
{
ASSERT(m_private);
ASSERT(m_source);
@@ -121,6 +126,73 @@ void SourceBuffer::setTimestampOffset(double offset,
ExceptionState& es)
m_timestampOffset = offset;
}
+double SourceBuffer::appendWindowStart() const
+{
+ return m_appendWindowStart;
+}
+
+void SourceBuffer::setAppendWindowStart(double start, ExceptionState& es)
+{
+ // Enforce throwing an exception on restricted double values.
+ if (std::isnan(start)
+ || start == std::numeric_limits<double>::infinity()
+ || start == -std::numeric_limits<double>::infinity()) {
+ es.throwDOMException(TypeMismatchError);
+ return;
+ }
+
+ // Section 3.1 appendWindowStart attribute setter steps.
+ // 1. If this object has been removed from the sourceBuffers attribute of
the parent media source then throw an
+ // InvalidStateError exception and abort these steps.
+ // 2. If the updating attribute equals true, then throw an
InvalidStateError exception and abort these steps.
+ if (isRemoved() || m_updating) {
+ es.throwDOMException(InvalidStateError);
+ return;
+ }
+
+ // 3. If the new value is less than 0 or greater than or equal to
appendWindowEnd then throw an InvalidAccessError
+ // exception and abort these steps.
+ if (start < 0 || start >= m_appendWindowEnd) {
+ es.throwDOMException(InvalidAccessError);
+ return;
+ }
+
+ m_private->setAppendWindowStart(start);
+
+ // 4. Update the attribute to the new value.
+ m_appendWindowStart = start;
+}
+
+double SourceBuffer::appendWindowEnd() const
+{
+ return m_appendWindowEnd;
+}
+
+void SourceBuffer::setAppendWindowEnd(double end, ExceptionState& es)
+{
+ // Section 3.1 appendWindowEnd attribute setter steps.
+ // 1. If this object has been removed from the sourceBuffers attribute of
the parent media source then throw an
+ // InvalidStateError exception and abort these steps.
+ // 2. If the updating attribute equals true, then throw an
InvalidStateError exception and abort these steps.
+ if (isRemoved() || m_updating) {
+ es.throwDOMException(InvalidStateError);
+ return;
+ }
+
+ // 3. If the new value equals NaN, then throw an InvalidAccessError and
abort these steps.
+ // 4. If the new value is less than or equal to appendWindowStart then
throw an InvalidAccessError
+ // exception and abort these steps.
+ if (std::isnan(end) || end <= m_appendWindowStart) {
+ es.throwDOMException(InvalidAccessError);
+ return;
+ }
+
+ m_private->setAppendWindowEnd(end);
+
+ // 5. Update the attribute to the new value.
+ m_appendWindowEnd = end;
+}
+
void SourceBuffer::appendBuffer(PassRefPtr<ArrayBuffer> data, ExceptionState&
es)
{
// Section 3.2 appendBuffer()
@@ -166,7 +238,11 @@ void SourceBuffer::abort(ExceptionState& es)
// 4. Run the reset parser state algorithm.
m_private->abort();
- // FIXME(229408) Add steps 5-6 update appendWindowStart & appendWindowEnd.
+ // 5. Set appendWindowStart to 0.
+ setAppendWindowStart(0, es);
+
+ // 6. Set appendWindowEnd to positive Infinity.
+ setAppendWindowEnd(std::numeric_limits<double>::infinity(), es);
}
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/changbin.shao@intel.com/14876007/40001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/changbin.shao@intel.com/14876007/40001
On 2013/08/08 08:43:00, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/changbin.shao%40intel.com/14876007/40001 ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: Source/core/platform/graphics/SourceBufferPrivate.h Source/web/SourceBufferPrivateImpl.cpp Source/web/SourceBufferPrivateImpl.h public/web/WebSourceBuffer.h Hi eseidel & darin, could you please review these files? thanks!
On 2013/08/08 08:43:00, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/changbin.shao%40intel.com/14876007/40001 ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: Source/core/platform/graphics/SourceBufferPrivate.h Source/web/SourceBufferPrivateImpl.cpp Source/web/SourceBufferPrivateImpl.h public/web/WebSourceBuffer.h Hi eseidel & darin, could you please review these files? thanks!
Hi acolwell, Can you help to add OWNERS to review the changes of public/web, Source/web, Source/core?
jamesr: Can I get a quick OWNERS LGTM for the changes in public/web, Source/web, Source/core/platform/graphics please? They are really trivial.
https://codereview.chromium.org/14876007/diff/40001/Source/web/SourceBufferPr... File Source/web/SourceBufferPrivateImpl.h (right): https://codereview.chromium.org/14876007/diff/40001/Source/web/SourceBufferPr... Source/web/SourceBufferPrivateImpl.h:42: class SourceBufferPrivateImpl : public WebCore::SourceBufferPrivate { why does this class exist? WebKit::WebSourceBuffer looks like a very simple interface type that could live in public/platform/, in which case WebCore::SourceBuffer could be implemented directly on top of it. Or maybe the WebCore sourcebuffer type doesn't need to exist at all, depending on what uses it. This extra indirection seems unnecessary
On 2013/08/09 01:00:35, jamesr wrote: > https://codereview.chromium.org/14876007/diff/40001/Source/web/SourceBufferPr... > File Source/web/SourceBufferPrivateImpl.h (right): > > https://codereview.chromium.org/14876007/diff/40001/Source/web/SourceBufferPr... > Source/web/SourceBufferPrivateImpl.h:42: class SourceBufferPrivateImpl : public > WebCore::SourceBufferPrivate { > why does this class exist? WebKit::WebSourceBuffer looks like a very simple > interface type that could live in public/platform/, in which case > WebCore::SourceBuffer could be implemented directly on top of it. Or maybe the > WebCore sourcebuffer type doesn't need to exist at all, depending on what uses > it. This extra indirection seems unnecessary This could just be legacy from the advice I got when I originally implemented these classes. I didn't think code in core/ or modules/ could depend on WebXXX classes. I thought this was to prevent dependencies between the WebKit API and the WebCore APIs. My guess is that this rule must have changed since the fork. I am happy to remove this layer, since I've always found it silly. Is it ok if I do this in a follow up CL? It will likely result in a Chromium-side changes as well and this migration is totally independent of this CL at hand. Is there doc somewhere that outline when something should be in public/web vs public/platform?
public/web/ is the client API used by chromium to talk to Blink. It's implemented by Source/web/ on top of /core/ and /modules/. /web/, /core/, /modules/ are implemented on top of the platform API, defined in public/platform/. SourceBuffer looks like a fairly low-level concept that might make more sense in the platform API, in which case you could use it directly from /core/ and avoid the SourceBufferPrivate indirection. The platform API is older than the notion of modules, although it hasn't been around forever. This patch lgtm with some nits below. https://codereview.chromium.org/14876007/diff/40001/public/web/WebSourceBuffer.h File public/web/WebSourceBuffer.h (right): https://codereview.chromium.org/14876007/diff/40001/public/web/WebSourceBuffe... public/web/WebSourceBuffer.h:34: #include "WebTimeRange.h" is this include used? https://codereview.chromium.org/14876007/diff/40001/public/web/WebSourceBuffe... public/web/WebSourceBuffer.h:47: virtual void setAppendWindowStart(double) = 0; the meaning of the parameter on these functions are not clear from the type. are these timestamps? please clarify with either a parameter name or a comment
On 2013/08/09 21:03:14, jamesr wrote: > public/web/ is the client API used by chromium to talk to Blink. It's > implemented by Source/web/ on top of /core/ and /modules/. /web/, /core/, > /modules/ are implemented on top of the platform API, defined in > public/platform/. SourceBuffer looks like a fairly low-level concept that might > make more sense in the platform API, in which case you could use it directly > from /core/ and avoid the SourceBufferPrivate indirection. The platform API is > older than the notion of modules, although it hasn't been around forever. Thanks for the explanation. It looks like a fair amount of the media code has this wrong then. The current setup is core/html/HTMLMediaElement -> core/platform/graphics/... -> web/ -> web/public/WebXXX. Since most of the functionality is actually Chromium exposing media functionality to Blink it appears these interfaces should actually be in platform/ and public/platform. Looks like I have some fun multi-sided patches in my future. ;)
On 2013/08/09 21:40:47, acolwell wrote: > On 2013/08/09 21:03:14, jamesr wrote: > > public/web/ is the client API used by chromium to talk to Blink. It's > > implemented by Source/web/ on top of /core/ and /modules/. /web/, /core/, > > /modules/ are implemented on top of the platform API, defined in > > public/platform/. SourceBuffer looks like a fairly low-level concept that > might > > make more sense in the platform API, in which case you could use it directly > > from /core/ and avoid the SourceBufferPrivate indirection. The platform API > is > > older than the notion of modules, although it hasn't been around forever. > > Thanks for the explanation. It looks like a fair amount of the media code has > this wrong then. > The current setup is core/html/HTMLMediaElement -> core/platform/graphics/... -> > web/ -> web/public/WebXXX. Since most of the functionality is actually Chromium > exposing media functionality to Blink it appears these interfaces should > actually be in platform/ and public/platform. Looks like I have some fun > multi-sided patches in my future. ;) The core/platform/graphics/ -> web/ dependency is backwards. That's how we routed things before we had the platform API layer and routing in this way was the only way to have a dependency on chromium code, but now we have the platform API to avoid this hop.
On 2013/08/09 21:47:37, jamesr wrote: > On 2013/08/09 21:40:47, acolwell wrote: > > On 2013/08/09 21:03:14, jamesr wrote: > > > public/web/ is the client API used by chromium to talk to Blink. It's > > > implemented by Source/web/ on top of /core/ and /modules/. /web/, /core/, > > > /modules/ are implemented on top of the platform API, defined in > > > public/platform/. SourceBuffer looks like a fairly low-level concept that > > might > > > make more sense in the platform API, in which case you could use it directly > > > from /core/ and avoid the SourceBufferPrivate indirection. The platform API > > is > > > older than the notion of modules, although it hasn't been around forever. > > > > Thanks for the explanation. It looks like a fair amount of the media code has > > this wrong then. > > The current setup is core/html/HTMLMediaElement -> core/platform/graphics/... > -> > > web/ -> web/public/WebXXX. Since most of the functionality is actually > Chromium > > exposing media functionality to Blink it appears these interfaces should > > actually be in platform/ and public/platform. Looks like I have some fun > > multi-sided patches in my future. ;) > > The core/platform/graphics/ -> web/ dependency is backwards. That's how we > routed things before we had the platform API layer and routing in this way was > the only way to have a dependency on chromium code, but now we have the platform > API to avoid this hop. Hi jamesr and acolwell, thanks for your comments. Looks like there should be fair mount of code change.
https://codereview.chromium.org/14876007/diff/40001/public/web/WebSourceBuffer.h File public/web/WebSourceBuffer.h (right): https://codereview.chromium.org/14876007/diff/40001/public/web/WebSourceBuffe... public/web/WebSourceBuffer.h:34: #include "WebTimeRange.h" On 2013/08/09 21:03:14, jamesr wrote: > is this include used? Yes. "virtual WebTimeRanges buffered() = 0;" https://codereview.chromium.org/14876007/diff/40001/public/web/WebSourceBuffe... public/web/WebSourceBuffer.h:47: virtual void setAppendWindowStart(double) = 0; On 2013/08/09 21:03:14, jamesr wrote: > the meaning of the parameter on these functions are not clear from the type. are > these timestamps? please clarify with either a parameter name or a comment Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/changbin.shao@intel.com/14876007/65001
On 2013/08/09 22:59:59, Changbin Shao wrote: > Hi jamesr and acolwell, thanks for your comments. Looks like there should be > fair mount of code change. Hi. Don't worry about fixing the web/ vs platform/ changes. I'll take care of those. Thank you for your work on this patch.
On 2013/08/09 23:10:26, acolwell wrote: > On 2013/08/09 22:59:59, Changbin Shao wrote: > > Hi jamesr and acolwell, thanks for your comments. Looks like there should be > > fair mount of code change. > Hi. Don't worry about fixing the web/ vs platform/ changes. I'll take care of > those. Thank you for your work on this patch. You are welcome:) Your feedback helps me a lot to ramp up media source extensions, both codes and spec.
Message was sent while issue was closed.
Change committed as 155891 |
