Bug 160524

<rdar://problem/27685132>

Created attachment 285269 [details] Patch

Created attachment 285286 [details] Patch

Comment on attachment 285286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285286&action=review Can this be tested? > Source/WebCore/platform/mediastream/MediaConstraints.cpp:2 > + * Copyright (C) 2012 Google Inc. All rights reserved. probably not necessary. It doesn't look like you moved this from anywhere. It looks like you just copied the copyright after adding Apple to the other file.

Comment on attachment 285286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285286&action=review > Source/WebCore/platform/mediastream/MediaConstraints.cpp:42 > +RefPtr<BaseConstraint> BaseConstraint::create(String name) const String& name > Source/WebCore/platform/mediastream/MediaConstraints.cpp:68 > +Ref<LongConstraint> LongConstraint::create(String name) const String& > Source/WebCore/platform/mediastream/MediaConstraints.cpp:105 > + LOG(Media, "LongConstraint::initializeWithDictionary(%p) - failed to get dictionary value for key", this); Please log the key as well, e.g.: LOG(Media, "LongConstraint::initializeWithDictionary(%p) - failed to get dictionary value for key '%s'", this, key.utf8().data()); > Source/WebCore/platform/mediastream/MediaConstraints.cpp:120 > +Ref<DoubleConstraint> DoubleConstraint::create(String name) const String& > Source/WebCore/platform/mediastream/MediaConstraints.cpp:157 > + LOG(Media, "DoubleConstraint::initializeWithDictionary(%p) - failed to get dictionary value for key.", this); Log the key. > Source/WebCore/platform/mediastream/MediaConstraints.cpp:172 > +Ref<BooleanConstraint> BooleanConstraint::create(String name) const String& > Source/WebCore/platform/mediastream/MediaConstraints.cpp:197 > + LOG(Media, "BooleanConstraint::initializeWithDictionary(%p) - failed to get dictionary value for key.", this); Log the name. > Source/WebCore/platform/mediastream/MediaConstraints.cpp:208 > +Ref<StringConstraint> StringConstraint::create(String name) const String& > Source/WebCore/platform/mediastream/MediaConstraints.cpp:231 > + LOG(Media, "StringConstraint::initializeWithDictionary(%p) - failed to get dictionary value for key.", this); Log the key. > Source/WebCore/platform/mediastream/MediaConstraints.h:51 > + virtual ~BaseConstraint() { /* No-op */ }; Nit: the comment isn't necessary. > Source/WebCore/platform/mediastream/MediaConstraints.h:54 > + virtual void initializeWithDictionary(const Dictionary&) = 0; Does this need to be public? > Source/WebCore/platform/mediastream/MediaConstraints.h:101 > + static Ref<LongConstraint> create(String name, const Dictionary&); I don't see this in the patch.

(In reply to comment #4) > Comment on attachment 285286 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285286&action=review > > Can this be tested? Not in its current state, since nothing is hooked up. It will be tested once parsing for constraints in getUserMedia is done! > > > Source/WebCore/platform/mediastream/MediaConstraints.cpp:2 > > + * Copyright (C) 2012 Google Inc. All rights reserved. > > probably not necessary. It doesn't look like you moved this from anywhere. > It looks like you just copied the copyright after adding Apple to the other > file.

Created attachment 285338 [details] Patch

Created attachment 285341 [details] Patch

Comment on attachment 285341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285341&action=review > Source/WebCore/ChangeLog:9 > + No new tests, no new functionality. I would say something like: "This isn't testable yet, but tests will be added in a future patch." > Source/WebCore/platform/mediastream/MediaConstraints.cpp:75 > +RefPtr<BaseConstraint> BaseConstraint::createEmptyDerivedConstraint(const String& name) > +{ > + const RealtimeMediaSourceSupportedConstraints& supportedConstraints = RealtimeMediaSourceCenter::singleton().supportedConstraints(); > + MediaConstraintType constraintType = supportedConstraints.constraintFromName(name); > + > + switch (constraintType) { > + case MediaConstraintType::Width: > + case MediaConstraintType::Height: > + case MediaConstraintType::SampleRate: > + case MediaConstraintType::SampleSize: > + return LongConstraint::create(name); > + case MediaConstraintType::AspectRatio: > + case MediaConstraintType::FrameRate: > + case MediaConstraintType::Volume: > + return DoubleConstraint::create(name); > + case MediaConstraintType::EchoCancellation: > + return BooleanConstraint::create(name); > + case MediaConstraintType::FacingMode: > + case MediaConstraintType::DeviceId: > + case MediaConstraintType::GroupId: > + return StringConstraint::create(name); > + case MediaConstraintType::Unknown: > + return nullptr; > + } > +} Unless this will be used by other methods, I would put it inline in create. > Source/WebCore/platform/mediastream/MediaConstraints.cpp:126 > + setIdeal(value); > + } Future engineers will thank you if you add something like: LOG(Media, "LongConstraint::initializeWithDictionary(%p) - ignoring invalid key/value pair: '%s' : %f", this, key.utf8().data(), value); > Source/WebCore/platform/mediastream/MediaConstraints.cpp:178 > + setIdeal(value); > + } Ditto. > Source/WebCore/platform/mediastream/MediaConstraints.cpp:215 > + } > +} Ditto. > Source/WebCore/platform/mediastream/MediaConstraints.cpp:248 > + setIdeal(value); > + } Ditto.

Created attachment 285351 [details] Patch

Created attachment 285357 [details] Patch

Created attachment 285365 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 285365 [details]: The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 285365 [details]: editing/spelling/spellcheck-async.html bug 160571 (authors: g.czajkowski@samsung.com and mark.lam@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 285365 [details] Patch Clearing flags on attachment: 285365 Committed r204147: <http://trac.webkit.org/changeset/204147>

All reviewed patches have been landed. Closing bug.

This doesn't look right to me. Rather than passing Dictionaries into c++ land, this conversion should be happening in the bindings. And we certainly shouldn't be doing String -> Number conversions like this. It should all happen at the JSValue level.

Re-opened since this is blocked by bug 160602