-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename ImageBitmapOptions dictionary to ImageBitmapOptionsInit. #4248
Conversation
@grorg @jdashg @RafaelCintron this is what was discussed at the WebGL face-to-face meeting, but without introducing a new ImageBitmapOptions interface type. While writing that up, it became clear that it was redundant. This is now a small addition providing the key feature detection mechanism that was discussed. |
@kenrussell, I don't think adding a platform API to feature-detect a dictionary has been a general approach. @annevk, does firefox have a plan to change https://dxr.mozilla.org/mozilla-central/source/dom/webidl/WindowOrWorkerGlobalScope.webidl#38 to what's defined in the spec? |
@jdashg is the Mozilla employee who's been working most closely on this. On today's WebGL conference call a different approach was suggested involving making ImageBitmapOptions an interface, providing a new constructor taking a dictionary, and changing createImageBitmap to take either that interface, or "any" - which would be the existing dictionary. This would avoid duplicating the dictionary. I'll prototype this in Chromium to see if it can work, and will update this PR. (One way or another, the lack of ability to do feature detection on dictionaries defined in various web specs is highly problematic. If the approach currently suggested here is palatable, perhaps we should do it as an example for other APIs to follow.) |
@jungkees , even if all browsers implement the currently speced members, we end up in the same problem if we ever add a new setting to the dictionary. WebGL solved a similar problem with WebGLContextAttributes where the API returns a dictionary containing the settings the browser honored. Settings absent from the returned dictionary are not supported. I look forward to seeing the result of @kenrussell 's investigation. |
This is a particularly unusual method of dictionary feature detection, which I do not think we should include. Similarly, switching from dictionaries to interfaces is also an unprecedented way of doing feature detection for options objects, which I would not encourage. There is a canonical pattern for such feature detection, as discussed in whatwg/webidl#107. It's just ugly. I can understand the desire for something less ugly. I am not convinced it's a high priority to make a less-ugly version, and get that implemented and shipped everywhere. But if the ImageBitmap implementers do, then I'd suggest they use one of the established patterns we have, such as |
@domenic Neither of those two options works for the ImageBitmap use case. The browser is required to ignore unsupported dictionary members, so it would have to return true from "canCreateImageBitmap" if the passed dictionary contains attributes the browser doesn't know how to handle. The point of the proposed createImageBitmapOptions is to filter out those unhandled attributes so that the user knows exactly what the browser supports. It is not workable to require invoking createImageBitmap and checking the result; it's a costly operation, often involving decoding of large images, and applications need to decide which of various alternatives to use ahead of time. Given the prevalence of this problem on the web platform, we could start a new convention like "filterImageBitmapOptions", "filterUnsupportedImageBitmapOptions", or similar, if that is more palatable. Otherwise, we'll reify ImageBitmapOptions into an interface in a backward-compatible way and solve the problem only for this API. It would be better to address this longstanding web platform problem more generally, though. |
This is up to the spec to define; it's not an immutable law. The easiest way would be to take an object and check its properties, for example. |
But you can invoke it with a cheap image and see which getters the browser invoked and use that knowledge going forward, no? You could even throw from the last getter the browser has to check as the order is defined, meaning it would throw at the IDL layer before you even hit any of the image components in play. |
An alternative that could be possible is to follow the pattern in use by The difference here is that the I don't think it would be that strange to add the |
Are there any other blockers here? |
Besides the fundamental design disagreements discussed upthread, you mean? Nobody seems to have responded to those, so that's probably the most important blocker. |
I apologize for not coming back to this pull request and will do so soon. Wanted to prototype the alternatives in Chromium before updating it, and higher-priority issues keep coming up. |
This is a prototype taking into account the feedback from whatwg/html#4248 . Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . It basically adds a new interface to hold imagebitmapoptions while preserving the same old functionality with an imagebitmapoptionsinit. Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . It basically adds a new interface to hold imagebitmapoptions while preserving the same old functionality with an imagebitmapoptionsinit. Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . It basically adds a new interface to hold imagebitmapoptions while preserving the same old functionality with an imagebitmapoptionsinit. Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . It basically adds a new interface to hold imagebitmapoptions while preserving the same old functionality with an imagebitmapoptionsinit. Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . It basically adds a new interface to hold imagebitmapoptions while preserving the same old functionality with an imagebitmapoptionsinit. Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . It basically adds a new interface to hold imagebitmapoptions while preserving the same old functionality with an imagebitmapoptionsinit. Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
This is a prototype taking into account the feedback from whatwg/html#4248 . It basically adds a new interface to hold imagebitmapoptions while preserving the same old functionality with an imagebitmapoptionsinit. Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
04af5f3
to
d3e38fe
Compare
Reviewers: @Juanmihd from the Chrome team has successfully prototyped this revised API change: It is backward-compatible, and is potentially generalizable into a Web IDL construct (perhaps an extended attribute to be applied to dictionaries) which would auto-generate both a JavaScript dictionary and interface for a Web IDL dictionary. Could you please review this revision? Thank you! |
Could you explain what has changed since the previous review and why the outstanding feedback, e.g., #4248 (comment), no longer applies? |
@annevk the members of the WebGL working group - representing Apple, Google, Microsoft and Mozilla - agree unanimously that a better feature detection mechanism is needed than hooking an ImageBitmapOptions dictionary, creating a small ImageBitmap, and seeing which getters of the dictionary were called. This pull request refactors ImageBitmapOptions into a dual interface/dictionary pair, ImageBitmapOptions and ImageBitmapOptionsInit, following @travisleithead 's suggestion above regarding the Fetch API's Request and RequestInit pattern. It would be technically possible to add the ImageBitmapOptions members to the existing ImageBitmap interface itself, but I think this would lead to developer confusion. For example, specifying resizeWidth and resizeHeight in ImageBitmapOptions changes the width and height attributes of the resulting ImageBitmap, and adding them as additional members would be redundant. ImageBitmaps can be created from other ImageBitmaps, so policy questions about whether the premultiplyAlpha attribute should reflect the most recently passed one, or some union of all of them, would arise. I think leaving ImageBitmap as a mostly-opaque handle, and reflecting the user agent's supported creation options, is the more desirable approach. Could this please be re-reviewed? Thanks. |
Can those members of the working group please present their arguments to the editors of HTML? Or at least, answer the arguments that the editors of HTML have previously put forward? |
@domenic what specific arguments would you like presented? I thought I answered the earlier questions and points in my message above. This newly uploaded pull request eliminates the ad-hoc feature detection mechanism proposed earlier in this thread with one that is general, and which could be extended to other web APIs if desired. |
If you are proposing this as a general mechanism that discussion ought to be had with the Web IDL community. Thus far there's been no agreement there and adding a class-per-dictionary doesn't seem great. Also, Fetch doesn't establish precedent here as the |
@annevk certainly not proposing to add a class per dictionary throughout the HTML specification - only suggesting that the approach now taken in this pull request is generalizable. Could this revised pull request please be reviewed? |
The only answer I saw to #4248 (comment) was "agree unanimously that a better feature detection mechanism is needed". No technical arguments were presented to support this position. Additionally, the arguments that this needs a general solution are being answered in two ways: one, with a claim that the proposed solution is general; but two, with the claim that the proposed solution is not proposed to be used generally. We need to get consensus within the Web IDL community on a general solution before accepting such a design. |
In response to the 3 repetitions of "Could this revised pull request please be reviewed?", I'll note that that is what we are doing. Pull request review for additions to the HTML Standard doesn't consist solely of finding typos line by line. It primarily consists of evaluating the addition presented as to whether it fits in with the architecture of the web platform and the HTML Standard itself. |
Personally I'm open to suggestions. We've identified a pretty bad developer pain-point, and have provided a proposal to address it. If we don't know how to move this forward, we need your help solving this developer need. Ultimately, it's not actually the WebGL WG's problem to solve, it's HTML's problem that we're asking for a solution for. We're just trying to jump-start progress here. |
@domenic I apologize, I misunderstood. It sounded like none of the HTML editors had looked at the body of this pull request. The current state of the art of detecting supported dictionary members described in whatwg/webidl#107 is complex and error prone. One suggestion above - throwing on the "last" getter invoked against the dictionary, in order to avoid having to provide a valid image to Does this provide enough evidence that an improvement should be made, at least for the ImageBitmap APIs? This comment above attempts to motivate the design decision to reify ImageBitmapOptions itself, rather than reflecting its properties on the created ImageBitmap. What are your thoughts on that aspect? Let's put aside my statements that this is a general solution - since the more general Web IDL question of reifying dictionaries in JavaScript hasn't been solved in 3+ years, I'd like to not gate this pull request on solving that problem. I do however hope that this PR might inform a way forward for dictionary feature detection in other web APIs. |
Yes, there are some downsides, but it's not as complicated as you make it out to be and you have to understand that there's a lot of APIs with similar oddities and we should really solve for them holistically. Meanwhile I'd recommend developers to use something along these lines, which seems to work pretty well (I'll note that Firefox doesn't seem to support the options argument currently): async function isImageBitmapOptionSupported(feature) {
try {
await self.createImageBitmap(new ImageData(1,1), { get [feature]() { throw 1 } });
} catch (e) {
if (e === 1) {
return true;
}
}
return false;
}
isImageBitmapOptionSupported("imageOrientation").then(isSupported => ...); |
@annevk it's disappointing to me that the HTML spec editors are pushing back on an improvement to one of the APIs, requested by the de facto API owners, because we're not solving the problem for the entire HTML specification. The feature detection that web authors must do in this case is unpleasant. They'll need to add asynchronous feature detection for all ImageBitmap options before initializing their libraries / apps / etc. Our working group has received multiple requests from web authors to improve the ability to feature detect in this area of the API. Would it help if they voiced their opinions on this pull request? My concrete suggestion for generalizing our solution would be to add some new extended attribute to Web IDL - perhaps [ReifyDictionary] - which, for any dictionary to which it is applied, would generate both that dictionary, and an associated interface which can be constructed from that dictionary, in the ECMAScript binding. Essentially, the same transformation that has been done to ImageBitmapOptions in this pull request. I realize similar approaches have been considered before, but perhaps not this specific incarnation. |
Given the longevity of web standards, I think having a pretty high bar for new primitives is good, especially when there are workarounds available. I'd recommend contributing to whatwg/webidl#107. A number of implementers do seem interested in doing something and more slightly problematic cases might tip it in the direction of something getting done. |
I'm relatively satisfied that this is polyfillable now: |
This is a prototype taking into account the feedback from whatwg/html#4248 . It basically adds a new interface to hold imagebitmapoptions while preserving the same old functionality with an imagebitmapoptionsinit. Bug: 983173 Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
I recently implemented a createImageBitmap monkey-patch and I did face this detection problem. IMHO, what could be done at the specs level (or maybe at Web IDL's?) is
In current Chrome's implementation of the ImageBitmapOptions, all the properties are accessed synchronously, so the example in #4248 (comment) can even be rewritten as a synchronous method, which is actually very important since we need the source's buffer to be copied synchronously (think for instance about a webgl context without Regarding the second bullet, I think in particular about CSS's ScrollOptions case (which faces the same detection issue), where unfortunately we can't use the property-bag-trap for the |
@Kaiido 1 follows from IDL. 2 relies on implementers not adding members without backing implementation and they have done that thus far. |
Oh right I see it now in https://heycam.github.io/webidl/#es-dictionary So my point still stands that the property-bag-trap should be enough.
Unfortunately not all no, as I said, at least Safari will try to access the |
IDL is probably a better place, but the problem is deeper than that as they shouldn't have the IDL member in the code if it's not backed by an implementation. (I.e., the loop in IDL shouldn't be able to reach unsupported members.) |
I think the trap should be sufficient to feature-detect whether the key is supported, but if only a subset of all possible values are supported, we're still out of luck. |
Specifically, I would consider a browser that does a key lookup without supporting that key to be defective, and recommend filing a bug report with that browser. |
Given that there was no support for making this change, I'm closing this pull request. |
Add ImageBitmapOptions interface which can be constructed from
ImageBitmapOptionsInit.
The ImageBitmapOptions interface allows easy detection of which
ImageBitmap features the user agent supports, especially as
ImageBitmapOptionsInit evolves.
Representatives from Apple, Google, Microsoft and Mozilla have agreed
at recent WebGL working group meetings to add this feature detection
mechanism. The pattern could be generalized as a Web IDL construct for
any web-exposed dictionaries where easy feature detection is desired.
💥 Error: Wattsi server error 💥
PR Preview failed to build. (Last tried on Jan 15, 2021, 7:59 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.