Bug 160606 - Implement constrainable type ConstrainDoubleRange
Summary: Implement constrainable type ConstrainDoubleRange
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Local Build
Hardware: All Other
: P2 Normal
Assignee: George Ruan
URL:
Keywords: InRadar
Depends on:
Blocks: 160524
  Show dependency treegraph
 
Reported: 2016-08-05 13:46 PDT by George Ruan
Modified: 2017-08-30 11:47 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.39 KB, patch)
2016-08-05 14:07 PDT, George Ruan
no flags Details | Formatted Diff | Diff
Patch (2.91 KB, patch)
2016-08-05 15:45 PDT, George Ruan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Radar WebKit Bug Importer 2016-08-05 13:47:52 PDT
<rdar://problem/27725318>
Comment 2 George Ruan 2016-08-05 14:07:06 PDT
Created attachment 285449 [details]
Patch
Comment 3 Chris Dumez 2016-08-05 14:14:20 PDT
Comment on attachment 285449 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285449&action=review

> Source/WebCore/Modules/mediastream/ConstrainDoubleRange.idl:27
> +Conditional=MEDIA_STREAM,

Should be indented.

> Source/WebCore/Modules/mediastream/ConstrainDoubleRange.idl:28
> +] dictionary ConstrainDoubleRange : DoubleRange {

Can you check the generated bindings, I doubt we support dictionary inheritance currently.

> Source/WebCore/Modules/mediastream/DoubleRange.idl:28
> +] dictionary DoubleRange {

I am not aware that we support dictionaries defined in their own IDL file. AFAIK, the dictionary needs to be defined in the IDL of the interface where it is used (e.g. MediaDevices.idl).

> Source/WebCore/Modules/mediastream/MediaDevices.h:58
> +struct ConstrainDoubleRange : DoubleRange {

Shouldn't this be public inheritance?
Comment 4 Chris Dumez 2016-08-05 14:17:20 PDT
Comment on attachment 285449 [details]
Patch

I don't think it makes sense to add IDL files for which we do not generate any code.
Comment 5 Chris Dumez 2016-08-05 14:18:41 PDT
(In reply to comment #4)
> Comment on attachment 285449 [details]
> Patch
> 
> I don't think it makes sense to add IDL files for which we do not generate
> any code.

FYI, Source/WebCore/DerivedSources.make is where we add the IDL files so that we actually generate code for them. However, in your case, you probably don't need additional IDL files if you move those dictionaries to the existing IDL file where they are used.
Comment 6 George Ruan 2016-08-05 15:45:47 PDT
Created attachment 285457 [details]
Patch
Comment 7 Chris Dumez 2016-08-05 15:53:06 PDT
Comment on attachment 285457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285457&action=review

> Source/WebCore/ChangeLog:10
> +        patch.

While this patch seems correct, I am not sure how helpful this is. This adds support for a ConstrainDoubleRange dictionary. However, this type is not used directly.
I looked at the specification and there is no API taking a ConstrainDoubleRange. However, there is a MediaTrackConstraintSet dictionary with a member of type ConstrainDouble.

ContrainDouble is defined as so:
typedef (double or ConstrainDoubleRange) ConstrainDouble;

However, since we don't support union types, you'll either need to add support for union types first or more likely use the following:
typedef any ConstrainDouble;

And do the conversion of this member on native side. In which case the ConstrainDoubleRange type you're adding would not end up being used.