Added an option to use ConicGradient API as a preference and removed the
corresponding expected FAILing test, over @ntim's implementation.
Details
Details
- Reviewers
emilio - Commits
- rMOZILLACENTRALcdd65df84242: Bug 1627014 - Implement CanvasRenderingContext2D.createConicGradient r=emilio
- Bugzilla Bug ID
- 1627014
Diff Detail
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Branch
- default
Event Timeline
Comment Actions
Code analysis found 5 defects in the diff 382087:
- 2 defects found by clang-tidy
- 3 defects found by clang-format
You can run this analysis locally with:
- ./mach clang-format -s -p dom/canvas/BasicRenderingContext2D.h dom/canvas/CanvasRenderingContext2D.h dom/canvas/CanvasRenderingContext2D.cpp (C/C++)
- ./mach static-analysis check --outgoing (C/C++)
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
You can view these defects on the code-review frontend and on Treeherder.
Comment Actions
Thanks for the patch!
Note that this has been posted with the wrong bug number. I'll fix it on Phabricator, but you may want to fix it locally as well, in case you push any newer changes.
Comment Actions
Looks fine at first sight, though Emilio is a much better reviewer here. You may want to address the C++ lint changes the code review bot suggested.
Have you tested if the offscreen canvas test passes ? You can do this by changing the metadata as you just did for the other test, then running ./mach wpt html/canvas/offscreen/fill-and-stroke-styles/2d.gradient.conic.html.
(re: testing-exception-elsewhere: the tests were contributed by chromium on WPT)
| testing/web-platform/meta/html/canvas/element/fill-and-stroke-styles/2d.gradient.conic.html.ini | ||
|---|---|---|
| 3 | This should probably be the metadata, just in case. | |
Comment Actions
Hey @ntim ! I did check the offscreen canvas test using ./mach wpt html/canvas/offscreen/fill-and-stroke-styles/2d.gradient.conic.html but it seems that it fails.
Emilio was actually mentoring me on this bug, and he made sure I do all that :D
Sorry about the bug number, made that mistake twice now :/ . Oh btw, how should I change my current branch with git so that the bug number is correct in that branch too?
I'll look into the style changes right away! Thanks :)
PS: Tagged you & @emilio both because you wrote the implementation, and thought it was appropriate : |
This comment was removed by aryanagal98.
Comment Actions
Code analysis found 2 defects in the diff 382119:
- 2 defects found by clang-tidy
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
If you see a problem in this automated review, please report it here.
You can view these defects on the code-review frontend and on Treeherder.
Comment Actions
Since it still needs some spec discussion, I think we should make this nightly-only for now. But with the changes suggested below this looks great, thanks!
| dom/canvas/CanvasRenderingContext2D.cpp | ||
|---|---|---|
| 197 | Can these be const? So, const Float mAngle;, const Point mCenter;? | |
| 2090 | This can be return MakeAndAddRef<CanvasConicGradient>(this, aAngle, Point(aCx, aCy));, I think | |
| 3575 | Let's update this assertion message too. | |
| modules/libpref/init/StaticPrefList.yaml | ||
| 1205 | It seems this needs still a bit of spec work, because https://github.com/whatwg/html/issues/5431 still hasn't come to a conclusion on what the behavior should be for inf / NaN / etc. I think it should be easy to solve, but meanwhile let's do value: @IS_NIGHTLY_BUILD@ | |
| testing/web-platform/meta/html/canvas/element/fill-and-stroke-styles/2d.gradient.conic.html.ini | ||
| 3 | As long as it's enabled by default I don't think it matters much, but sure. But since we probably want some spec text before shipping it to release, let's do this. | |
Comment Actions
Sent an intent email on your behalf: https://groups.google.com/g/mozilla.dev.platform/c/Pp1N_GCqY0M/m/OZHiTqfmBgAJ
Comment Actions
Code analysis found 2 defects in the diff 382128:
- 2 defects found by clang-tidy
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
If you see a problem in this automated review, please report it here.
You can view these defects on the code-review frontend and on Treeherder.
Comment Actions
One minor change so the test works if the pref is set to false.
| testing/web-platform/meta/html/canvas/element/fill-and-stroke-styles/2d.gradient.conic.html.ini | ||
|---|---|---|
| 3 | Does this work? I'm pretty sure this should just be at the be top level, under [2d.gradient.conic.html], and the [Conic gradient function exists] line can be removed. | |
Comment Actions
Code analysis found 2 defects in the diff 382141:
- 2 defects found by clang-tidy
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
If you see a problem in this automated review, please report it here.
You can view these defects on the code-review frontend and on Treeherder.