1187315 - Rename dom/webidl/Constraints.webidl to something less generic

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Paul Adenot (:padenot)]

>  * | bz is confused about how http://hg.mozilla.org/mozilla-central/rev/c0b31eca151e managed to land without
>    | DOM peer review.  :(
> bz | we should get the hook fixed
> bz | because the name of this file is nuts.  ;)
> bz | Anyway
> bz | r=me in bug, but please add a link to the spec and get this file renamed to something sane.  ;)

Comment 1

[Maire Reavy [:mreavy]]

P1 because bz would have required this if he had reviewed before the file was added.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks, and whoops on the DOM review! Sorry about that. I understand from backscroll that it landed one day before the hooks.

For background, this file exists because of some limitation in our webidl compiler with unions and/or typedefs, that types need to be in different files from each other. That's why the file isn't named after an interface in a spec.

I wonder if this limitation still exists? If not, we may be able to move these definitions to the files they're used in.

Comment 3

[Paul Adenot (:padenot)]

Boris, maybe you know if this (comment 2) limitation still applies ? Maybe we can directly remove the file if it's been fixed.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Still an issue (I tried moving this to MediaTrackConstraintSet.webidl):

> 2:00.97 TypeError: Dictionary contains a union that contains a dictionary in the same WebIDL file.  This won't compile.  Move the inner dictionary to a different file.
> 2:00.97 /Users/Jan/moz/mozilla-central/dom/webidl/MediaTrackConstraintSet.webidl line 65:45
> 2:00.97 typedef (DOMString or sequence<DOMString> or ConstrainDOMStringParameters) ConstrainDOMString;
> 2:00.97                                              ^
> 2:00.97 /Users/Jan/moz/mozilla-central/dom/webidl/MediaTrackConstraintSet.webidl line 46:0
> 2:00.97 dictionary ConstrainDOMStringParameters {
> 2:00.97 ^

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1187315 - Refactor out Constraints.webidl

Bug 1187315 - Refactor out Constraints.webidl

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8639295 [details]
MozReview Request: Bug 1187315 - Refactor out Constraints.webidl

Bug 1187315 - Refactor out Constraints.webidl

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(I've filed Bug 1187923 on the inability to see what was removed in Review Board.)

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Hope this helps http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Constraints.webidl?rev=a1829935d87e

I'm on m-c so I'll unbitrot to get https://hg.mozilla.org/integration/mozilla-inbound/rev/2964352ce228#l6.12 later today

Comment 9

[Paul Adenot (:padenot)]

Jan-Ivar is doing this.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8639295 [details]
MozReview Request: Bug 1187315 - Refactor out Constraints.webidl

> Boris, maybe you know if this (comment 2) limitation still applies ?

It does.  The limitation is quite simple: if you have the sort of setup that triggers the typeerror in comment 4 there is no way to write the C++ declarations sanely given our setup for unions, in general.  It might be possible to do it if the union is only used in this one IDL file, but as soon as any other IDL file used the same union, things would break.

In any case, my complaint wasn't about the file _existing_, nor about it being named after something that's not an interface.  My complaint was that the name is too generic is all.

Given that reviewboard is not showing me the actual diff... mind attaching that diff here so I can review it?

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Refactor out Constraints.webidl

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> In any case, my complaint wasn't about the file _existing_, nor about it
> being named after something that's not an interface.  My complaint was that
> the name is too generic is all.

There's always more to clean up. I found that SupportedVideoConstraints was unused, and that we had one more file indirection than we needed. This seems cleaner to me. Let me know what you think.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8639365 [details] [diff] [review]
Refactor out Constraints.webidl

r=me

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Refactor out Constraints.webidl (2) r=bz

Rebased (to get https://hg.mozilla.org/integration/mozilla-inbound/rev/2964352ce228#l6.12 )

Carrying forward r=bz.

Comment 15

[Carsten Book [:Tomcat]]

can we get a try run here, thanks!

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

There's one in Review Board from the first patch (hidden):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=888d88616654

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8639295 [details]
MozReview Request: Bug 1187315 - Refactor out Constraints.webidl

Bug 1187315 - Refactor out Constraints.webidl

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Whoops, never got checked in. Rebasing and re-running try.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14207bf8e5b8

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14207bf8e5b8