A preliminary review of bug 162147 asked for a restructuring of the MediaConstraint classes: MediaConstraint should not be reference counted, MediaTrackConstraintSetMap should not use a HashMap, clean up MediaConstraint inheritance.
Created attachment 289859 [details] Proposed patch.
Comment on attachment 289859 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=289859&action=review > Source/WebCore/platform/mediastream/MediaConstraints.h:49 > + static MediaConstraint& create(const AtomicString&); This is really weird. Why not just make the constructor public? > Source/WebCore/platform/mediastream/MediaConstraints.h:55 > + MediaConstraint(const MediaConstraint& other) > + { > + m_name = other.m_name; > + m_constraintType = other.m_constraintType; > + m_dataType = other.m_dataType; > + } Won't this copy-constructor just be auto-generated if you left this out? > Source/WebCore/platform/mediastream/MediaConstraints.h:242 > + NumericConstraint(const NumericConstraint& other) > + : MediaConstraint(other) > { > + if (other.m_min) > + m_min = other.m_min; > + if (other.m_max) > + m_max = other.m_max; > + if (other.m_exact) > + m_exact = other.m_exact; > + if (other.m_ideal) > + m_ideal = other.m_ideal; > + } Ditto. > Source/WebCore/platform/mediastream/MediaConstraints.h:282 > - static Ref<IntConstraint> create(const String& name, MediaConstraintType type) { return adoptRef(*new IntConstraint(name, type)); } > + static IntConstraint& create(const AtomicString& name, MediaConstraintType type) { return *new IntConstraint(name, type); } > + IntConstraint(const IntConstraint& other) > + : NumericConstraint<int>(other) > + { > + } Ditto. > Source/WebCore/platform/mediastream/MediaConstraints.h:302 > - static Ref<DoubleConstraint> create(const String& name, MediaConstraintType type) { return adoptRef(*new DoubleConstraint(name, type)); } > + static DoubleConstraint& create(const AtomicString& name, MediaConstraintType type) { return *new DoubleConstraint(name, type); } > + DoubleConstraint(const DoubleConstraint& other) > + : NumericConstraint<double>(other) > + { > + } Ditto. > Source/WebCore/platform/mediastream/MediaConstraints.h:318 > + static BooleanConstraint& create(const AtomicString& name, MediaConstraintType type) { return *new BooleanConstraint(name, type); } Ditto. > Source/WebCore/platform/mediastream/MediaConstraints.h:326 > + BooleanConstraint(const BooleanConstraint& other) > + : MediaConstraint(other) > + { > + if (other.m_exact) > + m_exact = other.m_exact; > + if (other.m_ideal) > + m_ideal = other.m_ideal; > + } Ditto. > Source/WebCore/platform/mediastream/MediaConstraints.h:389 > + static StringConstraint& create(const AtomicString& name, MediaConstraintType type) { return *new StringConstraint(name, type); } Ditto. > Source/WebCore/platform/mediastream/MediaConstraints.h:397 > + StringConstraint(const StringConstraint& other) > + : MediaConstraint(other) > + { > + if (!other.m_exact.isEmpty()) > + m_exact = other.m_exact; > + if (!other.m_ideal.isEmpty()) > + m_ideal = other.m_ideal; > + } It seems like a lot more work to initialize m_exact (e.g.) as empty, and then do an operator= on it, rather than just call the Vector copy-constructor. > Source/WebCore/platform/mediastream/MediaConstraints.h:502 > + class iterator { > + public: > + iterator() > + : m_constraint(nullptr) > + , m_index(0) > + { > + } > + > + iterator(const FlattenedConstraint* constraint, size_t index) > + : m_constraint(constraint) > + , m_index(index) > + { > + } > + > + MediaConstraint& operator*() const > + { > + return m_constraint->m_variants.at(m_index).constraint(); > + } > > - const_iterator begin() const { return m_constraints.begin(); } > - const_iterator end() const { return m_constraints.end(); } > + iterator& operator++() > + { > + m_index++; > + return *this; > + } > + > + bool operator==(const iterator& other) const { return m_index == other.m_index; } > + bool operator!=(const iterator& other) const { return !(*this == other); } So, you're going to have to be very, very careful with this iterator. Because adding or removing items from the m_variants list doesn't explicitly invalidate any iterators, you can git into a situation where you walk over uninitialized memory. Say, you've got something like this: for (auto iter = flattened.begin(); iter != flattened.end(); ++iter) { if (someCondition) flattened.merge(someValue); } If you're on the last item in flattened, and the merge operation deletes a row, then the next loop comparison will be iter (index=5) != end (index=4). And it will keep on walking! One cheap way to catch this kind of thing in debug builds would be for the FlattenedConstraint and it's iterator to both have a "generation" ivar. And every time you modify m_variants, you bump up the generation on FlattenedConstraint. The operator*() method on the constraint would ASSERT that its generation matches the FlattenedConstraint. The whole generation logic could be debug only. > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:490 > + advancedConstraint.forEach([&](const MediaConstraint& constraint) { Woo! I didn't know we added forEach to Vector!
Created attachment 289972 [details] Updated patch.
Created attachment 289977 [details] Updated patch.
Committed r206445: https://trac.webkit.org/r206445