Details
- Reviewers
gw nical emilio - Commits
- rMOZILLACENTRAL62a1a471e01c: Bug 1614890 - Implement conic-gradient for WebRender graphics backend. r=gw…
rMOZILLACENTRAL8f55490fb539: Bug 1614890 - Implement conic-gradient for WebRender graphics backend. r=gw…
rMOZILLACENTRAL53650b8686a6: Bug 1614890 - Implement conic-gradient for WebRender graphics backend. r=gw… - Bugzilla Bug ID
- 1614890
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Code analysis found 9 defects in the diff 226257:
- 7 defects found by clang-format
- 2 defects found by clang-tidy
You can run this analysis locally with:
- ./mach clang-format -s -p gfx/webrender_bindings/WebRenderAPI.h gfx/webrender_bindings/WebRenderAPI.cpp (C/C++)
- ./mach static-analysis check gfx/webrender_bindings/WebRenderAPI.cpp (C/C++)
For your convenience, here is a patch that fixes all the source-test-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.
| gfx/webrender_bindings/WebRenderAPI.cpp | ||
|---|---|---|
| 1206 | Warning: Narrowing conversion from 'double' to 'float' [clang-tidy: cppcoreguidelines-narrowing-conversions] | |
| 1339 | Warning: Narrowing conversion from 'double' to 'float' [clang-tidy: cppcoreguidelines-narrowing-conversions] | |
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
Code analysis found 9 defects in the diff 226277:
- 7 defects found by clang-format
- 2 defects found by clang-tidy
You can run this analysis locally with:
- ./mach clang-format -s -p gfx/webrender_bindings/WebRenderAPI.cpp gfx/webrender_bindings/WebRenderAPI.h (C/C++)
- ./mach static-analysis check gfx/webrender_bindings/WebRenderAPI.cpp (C/C++)
For your convenience, here is a patch that fixes all the source-test-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.
| gfx/webrender_bindings/WebRenderAPI.cpp | ||
|---|---|---|
| 1206 | Warning: Narrowing conversion from 'double' to 'float' [clang-tidy: cppcoreguidelines-narrowing-conversions] | |
| 1339 | Warning: Narrowing conversion from 'double' to 'float' [clang-tidy: cppcoreguidelines-narrowing-conversions] | |
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
Amazing work, from a quick read this looks pretty good to me! I'll take a detailed look later today, and maybe we'll get Nical to take a quick look over the C++ and brush bits.
Does this result in any Gecko reftests becoming unexpected PASS results?
One thing I'd like to do in future with the gradient code is make them all work like the linear fast path. That is, the gradient is rendered once into the texture cache and persisted there, and then each frame that pre-rendered gradient is drawn with the brush_image shader. This gives us better performance and batching during the scene draw, especially for large gradients. However, we can do that as a follow up in future (when we will also do this for radial gradients etc), there's no need to block an initial implementation because of that.
This looks good to me for the rust parts. Added Nical to take a look at the C++ and brush parts. Added a few comments, mostly related to test coverage and future optimization, nothing major.
| gfx/wr/webrender/src/scene_builder_thread.rs | ||
|---|---|---|
| 20 | nit: Seems like this shouldn't be needed, since there's no other changes in the file that reference it? | |
| gfx/wr/wrench/reftests/gradient/conic-simple.yaml | ||
| 8 | Could we extend this test to have multiple gradients, each with a different angle? And perhaps some with multiple stops, just to give us a bit more test coverage here? It'd be good to have some test cases for the conic gradient + border case too. If you prefer, extending the test suite as a follow up patch is fine. | |
Great stuff! just a few nits to fix just like Glenn. r=me otherwise
| gfx/webrender_bindings/WebRenderAPI.cpp | ||
|---|---|---|
| 1201 | Please make aAngle a float to match what's actually happening under the hood. | |
| gfx/wr/webrender/src/renderer.rs | ||
| 158 | nit: let's use another color so that we can differential conic from radial gradients in the builtin profiler. | |
| gfx/wr/webrender/src/scene_builder_thread.rs | ||
|---|---|---|
| 20 | Seems like it doesn't compile with this import removed, so I'll leave this in for now. FWIW, LinearGradient and RadialGradient are also not referenced directly in this file. It's possible that another file depends on it ? | |
| gfx/wr/wrench/reftests/gradient/conic-simple.yaml | ||
| 8 | I've added some extra test cases for a different angle and one with multiple stops. I'll file a new bug to extend the test suite further. | |
Code analysis found 12 defects in the diff 226516:
- 12 defects found by clang-format
You can run this analysis locally with:
- ./mach clang-format -s -p gfx/webrender_bindings/WebRenderAPI.cpp gfx/webrender_bindings/WebRenderAPI.h (C/C++)
For your convenience, here is a patch that fixes all the source-test-clang-format defects (use it in your repository with hg import or git apply -p0).
The analysis task source-test-coverity-coverity failed, but we could not detect any issue.
Please check this task manually.
If you see a problem in this automated review, please report it here.
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
Code analysis found 12 defects in the diff 226519:
- 12 defects found by clang-format
You can run this analysis locally with:
- ./mach clang-format -s -p gfx/webrender_bindings/WebRenderAPI.h gfx/webrender_bindings/WebRenderAPI.cpp (C/C++)
For your convenience, here is a patch that fixes all the source-test-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.
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
Looks like this is almost ready to merge. Could you apply the formatting patch referenced in https://phabricator.services.mozilla.com/D61599#1894721, and then this should be good, I think.
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
| gfx/wr/webrender/src/prim_store/gradient.rs | ||
|---|---|---|
| 572 | You should probably comment the unit this angle is in... radians I guess? | |
| gfx/wr/webrender_api/src/api.rs | ||
|---|---|---|
| 1162 | You forgot this part, this is why your patch is crashing while reporting memory. | |
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.