Bug 160524 – ⛱: Implement Types for Constrainable Properties

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Media

[George Ruan]

Currently, the class CapabilityValueOrRange only supports long and double types along with each type only supporting min, max, and exact values for Media Capabilities and Media Constraints.

According to https://w3c.github.io/mediacapture-main/#idl-def-constraindomstring, constrainable properties (and so also constraints) should be able to handle long, boolean, double, and String types along with having each type support a min, max, exact, and ideal member value.

[Radar WebKit Bug Importer]

<rdar://problem/27685132>

[George Ruan]

Created attachment 285269 [details]
Patch

[George Ruan]

Created attachment 285286 [details]
Patch

[Alex Christensen]

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.

[Eric Carlson]

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.

[George Ruan]

(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.

[George Ruan]

Created attachment 285338 [details]
Patch

[George Ruan]

Created attachment 285341 [details]
Patch

[Eric Carlson]

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.

[George Ruan]

Created attachment 285351 [details]
Patch

[George Ruan]

Created attachment 285357 [details]
Patch

[George Ruan]

Created attachment 285365 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 285365 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 285365 [details]:

editing/spelling/spellcheck-async.html bug 160571 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 285365 [details]
Patch

Clearing flags on attachment: 285365

Committed r204147: <http://trac.webkit.org/changeset/204147>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 160602