-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add Chromium data for CanvasRenderingContext2D #7465
Add Chromium data for CanvasRenderingContext2D #7465
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing surprises me here but I might be a bit rusty. Anything I need to know when reviewing mdn-bcd-collector PRs?
Welcome back, @Elchi3, glad to see you again! Basically, the mdn-bcd-collector project is a feature detection system that Philip and I have been working on, something like what Confluence does. Overall, reviewing is the same process as other PRs! |
@Elchi3 some tips for these PRs. I use #6862 for them, since that gives a compact of view of what has changed. The majority of issues I look for and catch are false negatives, due to a variety of reasons:
False negatives aren't only a problem when setting something to true, but are always necessary to consider unless the tests return true since the very first version of a browser, since an observed change between version N and N+1 could be a false negative for version N. However, I spend a lot less time looking for these types of errors when a set of changes together seem to make sense. Beyond false negatives, there are also some limitations of the update scripts themselves, some of them documented in foolip/mdn-bcd-collector#571. In all of my reviewing so far, however, I haven't spotted a case where this was the root cause for some incorrect data change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://mdn-bcd-collector.appspot.com/tests/api/CanvasRenderingContext2D tests using document.createElement('canvas')
so false negatives are quite unlikely here.
api.CanvasRenderingContext2D.drawImage.SVGImageElement_source_image is the only behavioral entry touched here, and the rest all look sensible to me, nothing that looks especially suspect.
@@ -1581,10 +1581,10 @@ | |||
"version_added": null | |||
}, | |||
"opera": { | |||
"version_added": null | |||
"version_added": "46" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is changing the "SVGImageElement_source_image" entry, but it looks like it's just doing mirroring. We don't generate tests for this entry.
Support for CanvasPattern is implied by support for ctx.createPattern(), since that's how a CanvasPattern instance is created. Similarly, support for CanvasGradient is implied by support for either createLinearGradient() or createRadialGradient(). The support data for CanvasRenderingContext2D looks reliable based on the PRs that updated it to the current state: mdn#7465 mdn#8666 Update CanvasGradient and CanvasPattern to match. The versions now match for all browsers except Opera, where some ≤12.1 versions were left alone rather than trying to confirm the correct versions.
Support for CanvasPattern is implied by support for ctx.createPattern(), since that's how a CanvasPattern instance is created. Similarly, support for CanvasGradient is implied by support for either createLinearGradient() or createRadialGradient(). The support data for CanvasRenderingContext2D looks reliable based on the PRs that updated it to the current state: #7465 #8666 Update CanvasGradient and CanvasPattern to match. The versions now match for all browsers except Opera, where some ≤12.1 versions were left alone rather than trying to confirm the correct versions.
This PR adds real values for Chrome and its derivatives for the CanvasRenderingContext2D API using results from the mdn-bcd-collector project, along with a tiny bit of mirroring (which was all confirmed with mdn-bcd-collector results).