Drop [UsePointersEvenForNonNullableObjectArguments] from MediaSource interfaces
Created attachment 277035 [details] Patch
Comment on attachment 277035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277035&action=review > Source/WebCore/Modules/mediasource/MediaSource.cpp:522 > + // FIXME: In case type is null, we should probably throw a TypeError. Yes, this is what the spec says: https://w3c.github.io/media-source/#widl-MediaSource-addSourceBuffer-SourceBuffer-DOMString-type Could you please make the fix in this patch? Changing the exception type should be safe compatibility-size. > Source/WebCore/Modules/mediasource/MediaSource.cpp:526 > if (type.isNull() || type.isEmpty()) { if (type.isEmpty()) would suffice Then ec = TypeError; > Source/WebCore/Modules/mediasource/MediaSource.cpp:576 > + return buffer.ptr(); This function should return a RefPtr<> and here we would just return WTFMove(buffer); > Source/WebCore/Modules/mediasource/SourceBuffer.h:90 > + void appendBuffer(ArrayBufferView*, ExceptionCode&); We really need to fix the bindings generator. > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:52 > +void SourceBufferList::add(SourceBuffer& buffer) Should be a Ref<>&& > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:54 > + m_list.append(&buffer); WTFMove(buffer) Also, could you look to see if making m_list a Vector<Ref<SourceBuffer>> could be done in this patch without too much effort? It would be much nicer.
Comment on attachment 277035 [details] Patch I’m going to set review- because I’d like to see another patch with the fixes Chris suggested.
Created attachment 277240 [details] Patch
(In reply to comment #2) > Comment on attachment 277035 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277035&action=review > > > Source/WebCore/Modules/mediasource/MediaSource.cpp:522 > > + // FIXME: In case type is null, we should probably throw a TypeError. > > Yes, this is what the spec says: > https://w3c.github.io/media-source/#widl-MediaSource-addSourceBuffer- > SourceBuffer-DOMString-type > > Could you please make the fix in this patch? Changing the exception type > should be safe compatibility-size. The issue is that binding generator should handle this case (null is not a valid value for DOMString). I fixed that by making type parameter nullable. > > Source/WebCore/Modules/mediasource/MediaSource.cpp:576 > > + return buffer.ptr(); > > This function should return a RefPtr<> and here we would just return > WTFMove(buffer); buffer is now moved before returning. I kept returning the raw pointer. > > Source/WebCore/Modules/mediasource/SourceBuffer.h:90 > > + void appendBuffer(ArrayBufferView*, ExceptionCode&); > > We really need to fix the bindings generator. Agreed. > > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:52 > > +void SourceBufferList::add(SourceBuffer& buffer) > > Should be a Ref<>&& FIXED. > > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:54 > > + m_list.append(&buffer); > > WTFMove(buffer) > > Also, could you look to see if making m_list a Vector<Ref<SourceBuffer>> > could be done in this patch without too much effort? It would be much nicer. Done. There are some HashMap<..., RefPtr<>> and Vector<RefPtr<>> that might also be changed to using Ref<> in mediasource/mediastream if possible.
Comment on attachment 277240 [details] Patch Attachment 277240 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1217916 New failing tests: http/tests/media/media-source/mediasource-addsourcebuffer.html media/media-source/media-source-addsourcebuffer.html
Created attachment 277244 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 277240 [details] Patch Attachment 277240 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1217918 New failing tests: http/tests/media/media-source/mediasource-addsourcebuffer.html media/media-source/media-source-addsourcebuffer.html
Created attachment 277245 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 277240 [details] Patch Attachment 277240 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1217929 New failing tests: http/tests/media/media-source/mediasource-addsourcebuffer.html media/media-source/media-source-addsourcebuffer.html
Created attachment 277246 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 277248 [details] Rebasing some tests
Comment on attachment 277248 [details] Rebasing some tests View in context: https://bugs.webkit.org/attachment.cgi?id=277248&action=review > Source/WTF/wtf/Ref.h:189 > +template<typename T, typename U> inline bool operator==(const Ref<T>& a, const Ref<U>& b) Antti and Andreas do not like this one. They think if there were an operator== it should forward to the underlying type's operator== since this is a reference, not a pointer type.
Comment on attachment 277248 [details] Rebasing some tests View in context: https://bugs.webkit.org/attachment.cgi?id=277248&action=review > Source/WebCore/Modules/mediasource/MediaSource.cpp:575 > + return result; This method should be updated to return a RefPtr<> since we transfer ownership to the caller.
(In reply to comment #14) > Comment on attachment 277248 [details] > Rebasing some tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277248&action=review > > > Source/WebCore/Modules/mediasource/MediaSource.cpp:575 > > + return result; > > This method should be updated to return a RefPtr<> since we transfer > ownership to the caller. Can you explain the issue? A "result" ref is kept by m_sourceBuffers. I wonder whether returning a RefPtr<> will not create unneeded refcounting at the JS binding code? > Antti and Andreas do not like this one. They think if there were an > operator== it should forward to the underlying type's operator== since this > is a reference, not a pointer type. Good point, I'll check this.
Comment on attachment 277248 [details] Rebasing some tests View in context: https://bugs.webkit.org/attachment.cgi?id=277248&action=review >>> Source/WebCore/Modules/mediasource/MediaSource.cpp:575 >>> + return result; >> >> This method should be updated to return a RefPtr<> since we transfer ownership to the caller. > > Can you explain the issue? > A "result" ref is kept by m_sourceBuffers. > I wonder whether returning a RefPtr<> will not create unneeded refcounting at the JS binding code? Hmm, given that m_sourceBuffers refs it, I don't feel strongly about it anymore.
Created attachment 277367 [details] Patch
Attachment 277367 [details] did not pass style-queue: ERROR: Source/WTF/wtf/Ref.h:200: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/Ref.h:205: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Tools/TestWebKitAPI/Tests/WTF/Ref.cpp:165: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Tools/TestWebKitAPI/Tests/WTF/Ref.cpp:170: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Tools/TestWebKitAPI/Tests/WTF/Ref.cpp:175: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Tools/TestWebKitAPI/Tests/WTF/Ref.cpp:180: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 6 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 277367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277367&action=review > Source/WebCore/Modules/mediasource/SourceBuffer.h:90 > + void appendBuffer(ArrayBufferView*, ExceptionCode&); Why does this take a pointer instead of a reference? > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:60 > size_t index = m_list.find(buffer); This does not seem to be what we want. We want to find the pointer, not find the value. And Vector::find will use == to compare Ref<SourceBuffer> with SourceBuffer&, right? That’s bad in two ways. One, it will churn the reference count on the passed in buffer every time it does a comparison. Two, it will compare the SourceBuffer objects rather than comparing the pointers. While this patch does add the correct behavior for Ref==Ref comparisons, I don’t think we actually want to use that algorithm in the code here from Vector::find for both of those reasons. And maybe we should not be adding the Ref==Ref operation at this time. Instead I think we need to write out a loop of our own to do the finding, or add another Vector function that is flexible enough that we can use something other than operator==. > Source/WebCore/Modules/mediasource/SourceBufferList.h:59 > + bool contains(SourceBuffer& buffer) { return m_list.find(buffer) != notFound; } Same issue here.
Comment on attachment 277367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277367&action=review > Source/WTF/wtf/Ref.h:207 > +template<typename T, typename U> inline auto operator==(const Ref<T>& a, const U& b) -> typename std::enable_if<!CanUseEqual<T, U>::value, bool>::type > +{ > + return a.ptr() == &b; > +} This is *not* a good idea. We don’t want Ref to silently do a pointer comparison if there is no == operator available to compare the values of the two objects. This is unprecedented behavior that might be convenient, but is subtle and dangerous. I suggest not adding this to Ref at all, and doing something different to deal with the Vector::find that we want to use to find a pointer in a vector of Ref. >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:60 >> size_t index = m_list.find(buffer); > > This does not seem to be what we want. > > We want to find the pointer, not find the value. And Vector::find will use == to compare Ref<SourceBuffer> with SourceBuffer&, right? That’s bad in two ways. One, it will churn the reference count on the passed in buffer every time it does a comparison. Two, it will compare the SourceBuffer objects rather than comparing the pointers. > > While this patch does add the correct behavior for Ref==Ref comparisons, I don’t think we actually want to use that algorithm in the code here from Vector::find for both of those reasons. And maybe we should not be adding the Ref==Ref operation at this time. > > Instead I think we need to write out a loop of our own to do the finding, or add another Vector function that is flexible enough that we can use something other than operator==. OK, my comment was wrong. Clearly your override of Ref does a pointer comparison with no reference count churn for SourceBuffer&, so the error lies elsewhere.
(In reply to comment #20) > Comment on attachment 277367 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277367&action=review > > > Source/WTF/wtf/Ref.h:207 > > +template<typename T, typename U> inline auto operator==(const Ref<T>& a, const U& b) -> typename std::enable_if<!CanUseEqual<T, U>::value, bool>::type > > +{ > > + return a.ptr() == &b; > > +} > > This is *not* a good idea. We don’t want Ref to silently do a pointer > comparison if there is no == operator available to compare the values of the > two objects. This is unprecedented behavior that might be convenient, but is > subtle and dangerous. > > I suggest not adding this to Ref at all, and doing something different to > deal with the Vector::find that we want to use to find a pointer in a vector > of Ref. > > >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:60 > >> size_t index = m_list.find(buffer); > > > > This does not seem to be what we want. > > > > We want to find the pointer, not find the value. And Vector::find will use == to compare Ref<SourceBuffer> with SourceBuffer&, right? That’s bad in two ways. One, it will churn the reference count on the passed in buffer every time it does a comparison. Two, it will compare the SourceBuffer objects rather than comparing the pointers. > > > > While this patch does add the correct behavior for Ref==Ref comparisons, I don’t think we actually want to use that algorithm in the code here from Vector::find for both of those reasons. And maybe we should not be adding the Ref==Ref operation at this time. > > > > Instead I think we need to write out a loop of our own to do the finding, or add another Vector function that is flexible enough that we can use something other than operator==. > > OK, my comment was wrong. Clearly your override of Ref does a pointer > comparison with no reference count churn for SourceBuffer&, so the error > lies elsewhere. Honestly, short-term, I would just keep using RefPtr<> in this Vector.
(In reply to comment #21) > Honestly, short-term, I would just keep using RefPtr<> in this Vector. Agreed, I think that’s the best thing to do for now.
(In reply to comment #21) > (In reply to comment #20) > > Comment on attachment 277367 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=277367&action=review > > > > > Source/WTF/wtf/Ref.h:207 > > > +template<typename T, typename U> inline auto operator==(const Ref<T>& a, const U& b) -> typename std::enable_if<!CanUseEqual<T, U>::value, bool>::type > > > +{ > > > + return a.ptr() == &b; > > > +} > > > > This is *not* a good idea. We don’t want Ref to silently do a pointer > > comparison if there is no == operator available to compare the values of the > > two objects. This is unprecedented behavior that might be convenient, but is > > subtle and dangerous. > > > > I suggest not adding this to Ref at all, and doing something different to > > deal with the Vector::find that we want to use to find a pointer in a vector > > of Ref. > > > > >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:60 > > >> size_t index = m_list.find(buffer); > > > > > > This does not seem to be what we want. > > > > > > We want to find the pointer, not find the value. And Vector::find will use == to compare Ref<SourceBuffer> with SourceBuffer&, right? That’s bad in two ways. One, it will churn the reference count on the passed in buffer every time it does a comparison. Two, it will compare the SourceBuffer objects rather than comparing the pointers. > > > > > > While this patch does add the correct behavior for Ref==Ref comparisons, I don’t think we actually want to use that algorithm in the code here from Vector::find for both of those reasons. And maybe we should not be adding the Ref==Ref operation at this time. > > > > > > Instead I think we need to write out a loop of our own to do the finding, or add another Vector function that is flexible enough that we can use something other than operator==. > > > > OK, my comment was wrong. Clearly your override of Ref does a pointer > > comparison with no reference count churn for SourceBuffer&, so the error > > lies elsewhere. > > Honestly, short-term, I would just keep using RefPtr<> in this Vector. Longer term, I think VectorTraits are meant for things like these?
(In reply to comment #23) > > Honestly, short-term, I would just keep using RefPtr<> in this Vector. > > Longer term, I think VectorTraits are meant for things like these? I don’t agree. That defines what every Vector does for a given type. In this case I think it’s an issue of we want to do with the particular Vector that’s the issue, not how all Vector<Ref> should work. Unless we let you allocate a Vector with custom traits.
(In reply to comment #24) > (In reply to comment #23) > > > Honestly, short-term, I would just keep using RefPtr<> in this Vector. > > > > Longer term, I think VectorTraits are meant for things like these? > > I don’t agree. That defines what every Vector does for a given type. In this > case I think it’s an issue of we want to do with the particular Vector > that’s the issue, not how all Vector<Ref> should work. Unless we let you > allocate a Vector with custom traits. Oh right, I assumed we would let you allocate a Vector with custom traits (like we do for HashMap), but it looks like you're right.
(In reply to comment #20) > Comment on attachment 277367 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277367&action=review > > > Source/WTF/wtf/Ref.h:207 > > +template<typename T, typename U> inline auto operator==(const Ref<T>& a, const U& b) -> typename std::enable_if<!CanUseEqual<T, U>::value, bool>::type > > +{ > > + return a.ptr() == &b; > > +} > > This is *not* a good idea. We don’t want Ref to silently do a pointer > comparison if there is no == operator available to compare the values of the > two objects. This is unprecedented behavior that might be convenient, but is > subtle and dangerous. > Agreed, Ref should remain a reference and not try to be more intelligent. > Honestly, short-term, I would just keep using RefPtr<> in this Vector. I'll update the patch accordingly.
Created attachment 277463 [details] Patch
Comment on attachment 277463 [details] Patch Clearing flags on attachment: 277463 Committed r200198: <http://trac.webkit.org/changeset/200198>
All reviewed patches have been landed. Closing bug.