Bug 160533

<rdar://problem/27688483>

Created attachment 285814 [details] WIP

Comment on attachment 285814 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=285814&action=review I realize is a WIP, but I added a few comments that might be helpful as you continue to work on it. > Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:157 > +bool CaptureDeviceManager::sessionSupportsConstraint(const CaptureSessionInfo*, RealtimeMediaSource::Type type, RefPtr<MediaConstraint> constraint) Can this be passed as a const reference? > Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:175 > + return isSupportedFrameRate(static_cast<LongConstraint*>(constraint.get())); Are we always certain that constraint is a LongConstraint? > Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:189 > +bool CaptureDeviceManager::isSupportedFrameRate(RefPtr<LongConstraint> constraint) const Can this be passed as a const reference? > Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:191 > + bool isSupported = true; Shouldn't this be initialized to false? > Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:200 > + isSupported &= 0 < constraint->exact() && constraint->exact() <= 60; shouldn't this be "isSupported = ..." because this is a required constraint? > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:70 > +static void initializeStringConstraintIdealWithList(Ref<StringConstraint>& constraint, const ArrayValue& list) > +{ > + size_t size; > + if (!list.length(size)) > + return; > + > + for (size_t i = 0; i < size; ++i) { > + String idealValue; > + if (list.get(i, idealValue)) > + constraint->appendIdeal(idealValue); > + } > +} > + > +static void initializeStringConstraintExactWithList(Ref<StringConstraint>& constraint, const ArrayValue& list) > +{ > + size_t size; > + if (!list.length(size)) > + return; > + > + for (size_t i = 0; i < size; ++i) { > + String exactValue; > + if (list.get(i, exactValue)) > + constraint->appendExact(exactValue); > + } > +} You should be able to combine these with something like this: static void initializeStringConstraintWithList(Ref<StringConstraint>& constraint, void (StringConstraint::*addValue)(const String&), const ArrayValue& list) { size_t size; if (!list.length(size)) return; for (size_t i = 0; i < size; ++i) { String value; if (list.get(i, value)) (constraint->*addValue)(value); } } > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:72 > +static RefPtr<StringConstraint> initializeStringConstraint(const Dictionary& mediaTrackConstraintSet, const String& name, bool isMandatoryConstraintSet) Nit: an enum would be better than a bool here, e.g. something like "enum ConstraintType { Mandatory, Optional } " > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:80 > + initializeStringConstraintExactWithList(constraint, exactArrayValue); and: initializeStringConstraintExactWithList(constraint, &StringConstraint::appendExact, exactArrayValue); > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:88 > + initializeStringConstraintIdealWithList(constraint, idealArrayValue); and: initializeStringConstraintExactWithList(constraint, &StringConstraint::appendIdeal, exactArrayValue); > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:95 > + if (constraint->isEmpty()) { > + LOG(Media, "initializeMediaConstraint() - ignoring string constraint '%s' with dictionary value since it has no valid or supported key/value pairs.", name.utf8().data()); Odd indentation. > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:104 > + if (mediaTrackConstraintSet.get(name, arrayValue) && !arrayValue.isUndefinedOrNull()) > + initializeStringConstraintIdealWithList(constraint, arrayValue); > + > + String value; > + if (!mediaTrackConstraintSet.get(name, value)) { Do you want an "else" between these? > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:119 > +static RefPtr<BooleanConstraint> initializeBooleanConstraint(const Dictionary& mediaTrackConstraintSet, const String& name, bool isMandatoryConstraintSet) This function and its siblings allocate and return constraints, so something like "create" would be a better prefix. > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:236 > + Nit: all of the InitializeXXXConstraint functions would be easier to follow if you used early returns and less indentation. > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:314 > + advancedConstraints.append(map); Nit: you should add the map after parsing in case the dictionary doesn't have any valid keys. > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:319 > + parseMediaTrackConstraintSetForKey(mediaTrackConstraintSet, localKey, advancedConstraints.at(i), false, sourceType); Nit: why not pass the map directly? > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:351 > + MediaTrackConstraintSetMap audioMandatoryConstraints; > + Vector<MediaTrackConstraintSetMap> audioAdvancedConstraints; Nit: swapping the type and "audio" would make this easier to read, e.g. "mandatoryAudioConstraints" etc. > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:359 > + mediaStreamConstraints.get("audio", areAudioConstraintsValid); Nit: is "areAudioConstraintsValid" really the best name, isn't the bool in this case whether or not audio is being requested? > Source/WebCore/platform/mediastream/MediaConstraints.h:43 > +class Dictionary; This isn't necessary. > Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:280 > + RefPtr<LongConstraint> widthConstraint = static_cast<LongConstraint*>(constraint.get()); How do we know it is a LongConstraint? > Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:281 > + return session->bestSessionPresetForVideoDimensions(widthConstraint->exact(), 0) != emptyString(); What if this is a min or max constraint? > Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:286 > + return session->bestSessionPresetForVideoDimensions(0, heightConstraint->exact()) != emptyString(); Ditto. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:187 > + RefPtr<LongConstraint> widthConstraintValue = static_cast<LongConstraint*>(mandatoryConstraints.get(widthConstraintName)); > + RefPtr<LongConstraint> heightConstraintValue = static_cast<LongConstraint*>(mandatoryConstraints.get(heightConstraintName)); Instead of casting here and elsewhere, can you make MediaTrackConstraintSetMap a class with getter and setter methods for the known constraints instead of a just a HashMap?

Comment on attachment 285814 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=285814&action=review >> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:175 >> + return isSupportedFrameRate(static_cast<LongConstraint*>(constraint.get())); > > Are we always certain that constraint is a LongConstraint? Here, yes. It's nested in a switch case which let's us know that the constraint is of type FrameRate which according to spec should be a long constraint. Since we create the MediaConstraint based on the type of constraint, this should always be a LongConstraint. I'll put an assert. >> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:191 >> + bool isSupported = true; > > Shouldn't this be initialized to false? No, I'm using a 'bitwise and' operation, so if any of the below checks ends up being false, we will return false. Otherwise we'll return true. That being said, I realize that the logic is backwards in the below checks. >> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:200 >> + isSupported &= 0 < constraint->exact() && constraint->exact() <= 60; > > shouldn't this be "isSupported = ..." because this is a required constraint? See above. >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:72 >> +static RefPtr<StringConstraint> initializeStringConstraint(const Dictionary& mediaTrackConstraintSet, const String& name, bool isMandatoryConstraintSet) > > Nit: an enum would be better than a bool here, e.g. something like "enum ConstraintType { Mandatory, Optional } " I was thinking the same thing =] >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:95 >> + LOG(Media, "initializeMediaConstraint() - ignoring string constraint '%s' with dictionary value since it has no valid or supported key/value pairs.", name.utf8().data()); > > Odd indentation. Woops! >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:119 >> +static RefPtr<BooleanConstraint> initializeBooleanConstraint(const Dictionary& mediaTrackConstraintSet, const String& name, bool isMandatoryConstraintSet) > > This function and its siblings allocate and return constraints, so something like "create" would be a better prefix. Sure. >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:236 >> + > > Nit: all of the InitializeXXXConstraint functions would be easier to follow if you used early returns and less indentation. Not sure if this is possible, since the conditional statements are used as fallthroughs. If readability is an issue I can create functions for the outermost if and else case. >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:314 >> + advancedConstraints.append(map); > > Nit: you should add the map after parsing in case the dictionary doesn't have any valid keys. Good point. >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:351 >> + Vector<MediaTrackConstraintSetMap> audioAdvancedConstraints; > > Nit: swapping the type and "audio" would make this easier to read, e.g. "mandatoryAudioConstraints" etc. Sure. >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:359 >> + mediaStreamConstraints.get("audio", areAudioConstraintsValid); > > Nit: is "areAudioConstraintsValid" really the best name, isn't the bool in this case whether or not audio is being requested? Sure. >> Source/WebCore/platform/mediastream/MediaConstraints.h:43 >> +class Dictionary; > > This isn't necessary. Good catch. >> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:280 >> + RefPtr<LongConstraint> widthConstraint = static_cast<LongConstraint*>(constraint.get()); > > How do we know it is a LongConstraint? We know that constraints of type MediaConstraintType::Width should always be a LongConstraint, since we construct the derived classes of MediaConstraints according to the name of the constraint itself. In this case, the width constraint is always a LongConstraint. I'll put an assert as well. >> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:281 >> + return session->bestSessionPresetForVideoDimensions(widthConstraint->exact(), 0) != emptyString(); > > What if this is a min or max constraint? It could very well be. But let's save that for a future bug. This change was to silence the compiler errors for now. >> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:286 >> + return session->bestSessionPresetForVideoDimensions(0, heightConstraint->exact()) != emptyString(); > > Ditto. Ditto! >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:187 >> + RefPtr<LongConstraint> heightConstraintValue = static_cast<LongConstraint*>(mandatoryConstraints.get(heightConstraintName)); > > Instead of casting here and elsewhere, can you make MediaTrackConstraintSetMap a class with getter and setter methods for the known constraints instead of a just a HashMap? Sounds like a good idea.

Created attachment 285883 [details] Patch

Created attachment 285885 [details] Patch

Created attachment 285888 [details] Patch

Comment on attachment 285888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285888&action=review > Source/WebCore/platform/mediastream/MediaConstraints.h:97 > + void setHasMin(bool value) { m_hasMin = value; } > + void setHasMax(bool value) { m_hasMax = value; } > + void setHasExact(bool value) { m_hasExact = value; } > + void setHasIdeal(bool value) { m_hasIdeal = value; } Nit: these can all be protected.

Comment on attachment 285888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285888&action=review > Source/WebCore/Modules/mediastream/CaptureDeviceInfo.h:52 > + virtual String bestSessionPresetForVideoDimensions(long /* width */, long /* height */) const { return emptyString(); } We use 'int' in C++ when Web IDL uses 'long'. A long in Web IDL is defined as a 32 bit integer. Therefore, an int is enough. Using long is unnecessary. Also, a long can end up being a 64 bit integers on 64bit Linux for e.g.

Comment on attachment 285888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285888&action=review > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:183 > + auto mandatoryConstraints = constraints->mandatoryConstraints(); Did you mean to copy the return value? If not, you may want to use auto&. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:184 > + auto advancedConstraints = constraints->advancedConstraints(); Did you mean to copy the return value? If not, you may want to use auto&. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:191 > + long width; Again, if this is defined as a long in the Web IDL, you want to use int here, not long. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:195 > + long height; Again, if this is defined as a long in the Web IDL, you want to use int here, not long. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:200 > NSString *preset = AVCaptureSessionInfo(session()).bestSessionPresetForVideoDimensions(width, height); It looks to me that it is possible that either width or height is used uninitialized here. You only check that either is set and you did not provide a default value. Note that you could use Optional<int> type for those variables to make things a bit clearer and less error prone. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:211 > + double frameRate; Ditto, could use Optional<double> instead of 2 separate variables. > Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:70 > +void RealtimeMediaSourceCenterMac::validateRequestConstraints(MediaStreamCreationClient* client, MediaConstraints* audioConstraints, MediaConstraints* videoConstraints) You dereference audioConstaints / videoContraints unconditionally in this method. If they cannot be null, then pass them by reference, not raw pointer. If they can be null, please add null checks. > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.h:56 > + void validateRequestConstraints(MediaStreamCreationClient*, MediaConstraints* audioConstraints, MediaConstraints* videoConstraints) override; override -> final since the class is final as per coding style. > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:40 > +static bool isLongMediaConstraintSatisfiable(const Ref<MediaConstraint>& constraint) ditto in this function about long vs int typing. Also, it looks like constraints could be passed as a MediaContraints& type. > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:60 > +static bool isDoubleMediaConstraintSatisfiable(const Ref<MediaConstraint>& constraint) MediaContraint& ? > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:80 > +static bool isBooleanMediaConstraintSatisfiable(const Ref<MediaConstraint>& constraint) MediaContraint& ? > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:89 > +static bool isStringMediaConstraintSatisfiable(const Ref<MediaConstraint>& constraint) MediaContraint& ? > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:94 > + if (notFound != constraintValue.find("invalid")) Please reverse the conditions, this looks weird. Also, you likely want ASCIILiteral("invalid"). > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:102 > +static bool isSatisfiable(RealtimeMediaSource::Type type, RefPtr<MediaConstraint> constraint) MediaContraint& or const MediaConstraint& ? > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:104 > + const RealtimeMediaSourceSupportedConstraints& supportedConstraints = RealtimeMediaSourceCenter::singleton().supportedConstraints(); the type is too long, please use auto&. > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:142 > +String MediaConstraintsMock::verifyConstraints(RealtimeMediaSource::Type type, MediaConstraints* constraints) MediaConstraints& or const MediaConstraints& Also looks like this could return a const String&? > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:144 > + auto mandatoryConstraints = constraints->mandatoryConstraints(); auto& ? > Source/WebCore/platform/mock/MockRealtimeMediaSourceCenter.cpp:70 > +void MockRealtimeMediaSourceCenter::validateRequestConstraints(MediaStreamCreationClient* client, MediaConstraints* audioConstraints, MediaConstraints* videoConstraints) Should be passed by reference if they cannot be null. Or please add null checks below.

Comment on attachment 285888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285888&action=review > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.cpp:67 > + return m_mandatoryConstraints; Looks like this could be inlined? > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.cpp:72 > + return m_advancedConstraints; Looks like this could be inlined? > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:45 > class MediaConstraintsImpl : public MediaConstraints { Could this class be final? > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:60 > + : m_mandatoryConstraints(mandatoryConstraints) m_mandatoryConstraints(WTFMove(mandatoryConstraints)) > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:61 > + , m_advancedConstraints(advancedConstraints) WTFMove() > Source/WebCore/Modules/mediastream/MediaDevices.cpp:66 > +void MediaDevices::getUserMedia(Ref<MediaConstraintsImpl>& audioConstraints, Ref<MediaConstraintsImpl>& videoConstraints, Promise&& promise, ExceptionCode& ec) const Ref<MediaConstraintsImpl>&& ? > Source/WebCore/Modules/mediastream/MediaDevices.cpp:68 > + UserMediaRequest::start(document(), audioConstraints, videoConstraints, WTFMove(promise), ec); WTFMove() for constraints. > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:390 > + impl.getUserMedia(audioConstraints, videoConstraints, DeferredWrapper(&state, castedThis->globalObject(), promiseDeferred), ec); WTFMove(audioConstraints) and same for videoConstraints?

Comment on attachment 285888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285888&action=review > Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:193 > +bool CaptureDeviceManager::isSupportedFrameRate(const Ref<MediaConstraint>& constraint) const MediaConstraint& or const MediaConstraint& > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:57 > +void UserMediaRequest::start(Document* document, Ref<MediaConstraintsImpl>& audioConstraints, Ref<MediaConstraintsImpl>& videoConstraints, MediaDevices::Promise&& promise, ExceptionCode& ec) Ref<MediaConstraintsImpl>&& > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:107 > + RealtimeMediaSourceCenter::singleton().validateRequestConstraints(this, m_audioConstraints.get(), m_videoConstraints.get()); Can m_audioConstraints / m_videoConstraints be null? If not, please consider updating data members to use Ref<> type instead of RefPtr<>. > Source/WebCore/Modules/mediastream/UserMediaRequest.h:58 > + static void start(Document*, Ref<MediaConstraintsImpl>& audioConstraints, Ref<MediaConstraintsImpl>& videoConstraints, MediaDevices::Promise&&, ExceptionCode&); Ref<MediaConstraintsImpl>&& > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:46 > +enum ConstraintSetType { Mandatory, Advanced }; enum class? > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:63 > + Ref<StringConstraint> constraint = StringConstraint::create(name); auto > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:89 > + return constraint.ptr(); return WTFMove(constraint); > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:96 > + return constraint.ptr(); return WTFMove(constraint); > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:107 > + return constraint.ptr(); return WTFMove(constraint); > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:117 > + Ref<BooleanConstraint> constraint = BooleanConstraint::create(name); auto > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:135 > + return constraint.ptr(); WTFMove() > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:146 > + return constraint.ptr(); WTFMove() > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:156 > + Ref<DoubleConstraint> constraint = DoubleConstraint::create(name); auto > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:182 > + return constraint.ptr(); WTFMove() > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:193 > + return constraint.ptr(); WTFMove() > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:203 > + Ref<LongConstraint> constraint = LongConstraint::create(name); auto > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:229 > + return constraint.ptr(); WTFMove() > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:240 > + return constraint.ptr(); WTFMove() > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:250 > + const RealtimeMediaSourceSupportedConstraints& supportedConstraints = RealtimeMediaSourceCenter::singleton().supportedConstraints(); auto& > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:300 > + map.add(name, mediaConstraint); map.add(name, WTFMove(mediaConstraint)); > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:332 > + advancedConstraints.append(map); WTFMove(map) > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:361 > + auto mediaStreamConstraints = Dictionary(&state, state.argument(0)); It looks like the parameter is mandatory in the specification? If so, then we should throw when missing. In which case, you would need to add this code before: if (UNLIKELY(state.argumentCount() < 1)) return state.vm().throwException(&state, createNotEnoughArgumentsError(&state)); Later on, you can then use uncheckedArgument(0) instead of argument(0). > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:374 > + Ref<MediaConstraintsImpl> audioConstraints = MediaConstraintsImpl::create(WTFMove(mandatoryAudioConstraints), WTFMove(advancedAudioConstraints), areAudioConstraintsValid); auto > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:387 > + Ref<MediaConstraintsImpl> videoConstraints = MediaConstraintsImpl::create(WTFMove(mandatoryVideoConstraints), WTFMove(advancedVideoConstraints), areVideoConstraintsValid); auto > Source/WebCore/platform/mediastream/MediaConstraints.cpp:43 > + const RealtimeMediaSourceSupportedConstraints& supportedConstraints = RealtimeMediaSourceCenter::singleton().supportedConstraints(); auto& > Source/WebCore/platform/mediastream/MediaConstraints.cpp:67 > +bool MediaConstraint::getMin(long&) const long vs int. > Source/WebCore/platform/mediastream/MediaConstraints.h:111 > +class LongConstraint final : public NumericConstraint { long vs int > Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:83 > + bool isSupportedFrameRate(const Ref<MediaConstraint>&) const override; const MediaConstraint&

Created attachment 285939 [details] WIP

Created attachment 285947 [details] Patch

Created attachment 285949 [details] Patch

Created attachment 285952 [details] Patch

Created attachment 285957 [details] Patch

Created attachment 285974 [details] Patch

Created attachment 285998 [details] Patch

Created attachment 286003 [details] Updated patch Add missing "#if ENABLE(MEDIA_STREAM)" to fix the build on some platforms.

Created attachment 286023 [details] Updated patch.

Comment on attachment 286023 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=286023&action=review I'll do one more pass to be safe. > Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:76 > + auto mandatoryConstraints = constraints.mandatoryConstraints(); Do we really want to copy the return value here? I doubt it.

Comment on attachment 286023 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=286023&action=review > Source/WebCore/ChangeLog:100 > + (WebCore::LongConstraint::setIdeal): Sets ideal value of constraint. The ChangeLog still refers to LongConstraint. > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:53 > + const MediaTrackConstraintSetMap& mandatoryConstraints() const override { return m_mandatoryConstraints; } Should be final as per coding style. > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:54 > + const Vector<MediaTrackConstraintSetMap>& advancedConstraints() const override { return m_advancedConstraints; } Should be final as per coding style. > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:55 > + bool isValid() const override { return m_isValid; } Should be final as per coding style. > Source/WebCore/Modules/mediastream/MediaDevices.idl:40 > + [PrivateIdentifier, RaisesException, ImplementedAs=getUserMedia, Custom] Promise getUserMediaFromJS(Dictionary options); I am not 100% clear why this is called getUserMediaFromJS and not getUserMedia as in the spec. > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:69 > + if (!dictionaryValue.get("exact", exactArrayValue) || exactArrayValue.isUndefinedOrNull()) Why is this a !Dictionary::get() ? Aren't we supposed to update constraint if we manage to get the value from the JS Dictionary (not the opposite)? > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:73 > + if (!dictionaryValue.get("exact", exactStringValue)) Why is this a !Dictionary::get() ? > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:81 > + if (!dictionaryValue.get("ideal", idealStringValue)) Why is this a !Dictionary::get() ? > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:209 > + auto constraint = LongConstraint::create(name); Why do we still have LongConstaint? Should probably be a IntConstraint or IntegerConstraint or IntegralConstraint. > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:361 > + auto castedThis = jsDynamicCast<JSMediaDevices*>(thisValue); I believe all the code at the beginning if this function has been copied from generated bindings and should not be duplicated here. At the point this method is called, the generated bindings should have already done those checks and |this| is already a JSMediaDevices* object. > Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:365 > + auto& impl = castedThis->wrapped(); ... So everything before this line should not be needed. > Source/WebCore/platform/mediastream/MediaConstraints.h:111 > +class LongConstraint final : public NumericConstraint { I don't think this should be called LongConstraint as I mentioned before. > Source/WebCore/platform/mediastream/MediaConstraints.h:197 > + bool getExact(Vector<String>&) const override; override -> final in all final classes, as per coding style. (same for other final classes in this file. > Source/WebCore/platform/mediastream/MediaConstraints.h:213 > +typedef HashMap<String, RefPtr<MediaConstraint>> MediaTrackConstraintSetMap; We prefer using vs typedef nowadays: using MediaTrackConstraintSetMap = HashMap<String, RefPtr<MediaConstraint>>; > Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:285 > + return session->bestSessionPresetForVideoDimensions(exact, 0) != emptyString(); return !session->bestSessionPresetForVideoDimensions(exact, 0).isEmpty(); > Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:294 > + return session->bestSessionPresetForVideoDimensions(0, exact) != emptyString(); return !session->bestSessionPresetForVideoDimensions(0, exact).isEmpty(); > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:198 > + if (width && height) { There is a behavior change. I don't know if this is intentional or not. I believe we used to treat missing as 0 when only one was provided. > Source/WebCore/platform/mock/MediaConstraintsMock.cpp:40 > +static bool isLongMediaConstraintSatisfiable(const MediaConstraint& constraint) "Long"

Comment on attachment 286023 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=286023&action=review >> Source/WebCore/ChangeLog:100 >> + (WebCore::LongConstraint::setIdeal): Sets ideal value of constraint. > > The ChangeLog still refers to LongConstraint. Fixed. >> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:76 >> + auto mandatoryConstraints = constraints.mandatoryConstraints(); > > Do we really want to copy the return value here? I doubt it. mandatoryConstraints() returns a "const MediaTrackConstraintSetMap&", so it won't make a copy will it? This is one of the reasons I hate "auto"! >> Source/WebCore/Modules/mediastream/MediaDevices.idl:40 >> + [PrivateIdentifier, RaisesException, ImplementedAs=getUserMedia, Custom] Promise getUserMediaFromJS(Dictionary options); > > I am not 100% clear why this is called getUserMediaFromJS and not getUserMedia as in the spec. Because "getUserMedia" is a JSBuiltin, and calls this private method, so because it is called from JS it is named "getUserMediaFromJS" >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:69 >> + if (!dictionaryValue.get("exact", exactArrayValue) || exactArrayValue.isUndefinedOrNull()) > > Why is this a !Dictionary::get() ? Aren't we supposed to update constraint if we manage to get the value from the JS Dictionary (not the opposite)? Good catch, fixed. >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:73 >> + if (!dictionaryValue.get("exact", exactStringValue)) > > Why is this a !Dictionary::get() ? Fixed. >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:81 >> + if (!dictionaryValue.get("ideal", idealStringValue)) > > Why is this a !Dictionary::get() ? Fixed. >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:209 >> + auto constraint = LongConstraint::create(name); > > Why do we still have LongConstaint? Should probably be a IntConstraint or IntegerConstraint or IntegralConstraint. Although I discussed leaving the name the same with you, I later decided that your suggestion makes sense. Fixed. >> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:361 >> + auto castedThis = jsDynamicCast<JSMediaDevices*>(thisValue); > > I believe all the code at the beginning if this function has been copied from generated bindings and should not be duplicated here. At the point this method is called, the generated bindings should have already done those checks and |this| is already a JSMediaDevices* object. Fixed. >> Source/WebCore/platform/mediastream/MediaConstraints.h:111 >> +class LongConstraint final : public NumericConstraint { > > I don't think this should be called LongConstraint as I mentioned before. Fixed. >> Source/WebCore/platform/mediastream/MediaConstraints.h:197 >> + bool getExact(Vector<String>&) const override; > > override -> final in all final classes, as per coding style. (same for other final classes in this file. Fixed. >> Source/WebCore/platform/mediastream/MediaConstraints.h:213 >> +typedef HashMap<String, RefPtr<MediaConstraint>> MediaTrackConstraintSetMap; > > We prefer using vs typedef nowadays: > using MediaTrackConstraintSetMap = HashMap<String, RefPtr<MediaConstraint>>; Fixed. >> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:285 >> + return session->bestSessionPresetForVideoDimensions(exact, 0) != emptyString(); > > return !session->bestSessionPresetForVideoDimensions(exact, 0).isEmpty(); Fixed. >> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:294 >> + return session->bestSessionPresetForVideoDimensions(0, exact) != emptyString(); > > return !session->bestSessionPresetForVideoDimensions(0, exact).isEmpty(); Fixed. >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:198 >> + if (width && height) { > > There is a behavior change. I don't know if this is intentional or not. I believe we used to treat missing as 0 when only one was provided. The previous code was wrong. >> Source/WebCore/platform/mock/MediaConstraintsMock.cpp:40 >> +static bool isLongMediaConstraintSatisfiable(const MediaConstraint& constraint) > > "Long" Fixed.

Created attachment 286094 [details] Updated patch.

Comment on attachment 286094 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=286094&action=review > Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:76 > + auto mandatoryConstraints = constraints.mandatoryConstraints(); still copying the return value?

Comment on attachment 286094 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=286094&action=review >> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:76 >> + auto mandatoryConstraints = constraints.mandatoryConstraints(); > > still copying the return value? As noted in my reply to your previous review: > mandatoryConstraints() returns a "const MediaTrackConstraintSetMap&", so it won't make a copy will it? > > This is one of the reasons I hate "auto"!

Comment on attachment 286094 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=286094&action=review >>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:76 >>> + auto mandatoryConstraints = constraints.mandatoryConstraints(); >> >> still copying the return value? > > As noted in my reply to your previous review: No, I do not believe your statement is accurate. My understanding is that it will copy unless you use auto&. see http://en.cppreference.com/w/cpp/language/template_argument_deduction#Other_contexts which refers to template argument deduction: http://en.cppreference.com/w/cpp/language/template_argument_deduction (sorry, I missed your comments to my previous review)

Comment on attachment 286094 [details] Updated patch. anyway, r=me with the auto& fixed.

Created attachment 286175 [details] Patch for landing.

Comment on attachment 286175 [details] Patch for landing. Attachment 286175 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1881799 New failing tests: fast/mediastream/enumerating-crash.html

Created attachment 286178 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Created attachment 286183 [details] Patch for landing.

Created attachment 286184 [details] Patch for landing.

Comment on attachment 286184 [details] Patch for landing. Clearing flags on attachment: 286184 Committed r204516: <http://trac.webkit.org/changeset/204516>

All reviewed patches have been landed. Closing bug.