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

RESOLVED FIXED in Firefox 43

Status

()

P1
normal
Rank:
15
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: padenot, Assigned: jib)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
>  * | 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.  ;)
(Reporter)

Updated

3 years ago
Assignee: nobody → padenot
P1 because bz would have required this if he had reviewed before the file was added.
backlog: --- → webRTC+
Rank: 15
Priority: -- → P1
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.
(Reporter)

Comment 3

3 years ago
Boris, maybe you know if this (comment 2) limitation still applies ? Maybe we can directly remove the file if it's been fixed.
Flags: needinfo?(bzbarsky)
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 ^
Created attachment 8639295 [details]
MozReview Request: Bug 1187315 - Refactor out Constraints.webidl

Bug 1187315 - Refactor out Constraints.webidl
Attachment #8639295 - Flags: review?(bzbarsky)
Comment on attachment 8639295 [details]
MozReview Request: Bug 1187315 - Refactor out Constraints.webidl

Bug 1187315 - Refactor out Constraints.webidl
(I've filed Bug 1187923 on the inability to see what was removed in Review Board.)
(Reporter)

Comment 9

3 years ago
Jan-Ivar is doing this.
Assignee: padenot → jib
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?
Flags: needinfo?(bzbarsky)
Attachment #8639295 - Flags: review?(bzbarsky)
Created attachment 8639365 [details] [diff] [review]
Refactor out Constraints.webidl
Attachment #8639295 - Attachment is obsolete: true
Attachment #8639365 - Flags: review?(bzbarsky)
(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 on attachment 8639365 [details] [diff] [review]
Refactor out Constraints.webidl

r=me
Attachment #8639365 - Flags: review?(bzbarsky) → review+
Created attachment 8640309 [details] [diff] [review]
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.
Attachment #8639365 - Attachment is obsolete: true
Attachment #8640309 - Flags: review+
Keywords: checkin-needed
can we get a try run here, thanks!
Flags: needinfo?(jib)
Keywords: checkin-needed
There's one in Review Board from the first patch (hidden):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=888d88616654
Flags: needinfo?(jib)
Comment on attachment 8639295 [details]
MozReview Request: Bug 1187315 - Refactor out Constraints.webidl

Bug 1187315 - Refactor out Constraints.webidl
Attachment #8639295 - Attachment is obsolete: false
Attachment #8639295 - Flags: review?(bzbarsky)
Attachment #8639295 - Flags: review?(bzbarsky) → review+
Attachment #8640309 - Attachment is obsolete: true
Whoops, never got checked in. Rebasing and re-running try.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/14207bf8e5b8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.