[canvas-a11y] Add canvas accessibility browser tests and fix bugs. [chromium/src : main]
Ramin Halavati (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Justin Novosad
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad added 5 comments![]( "Open in Gerrit")
#if BUILDFLAG(ENABLE_SCREEN_AI_BROWSERTESTS) && !BUILDFLAG(USE_FAKE_SCREEN_AI)Why is this buildflag combo important? Explain it in a comment.
::testing::Values(true, false),this param is for OCR, right? Since bool values are not self-documenting (unlike enums), it would be good to add a comment to document what this parameter is.
const ctx = canvas.getContext('2d', { willReadFrequently: true });Justify why you're using willReadFrequently: true here. I assume it's to get code coverage for the mojo pixel format issue you fixed, but it would be good to document that. Also it would be good to ensure that there is adequate coverage for canvas that don't have this option, which is the most common case. Also, I'm not sure this flag actually does anything when running on CQ bot. The main effect of this flag is to disable GPU acceleration, which is already disabled in most test suites.
const ctx = canvas.getContext('2d', { willReadFrequently: true });same here
const ctx = canvas.getContext('2d', { willReadFrequently: true });and here
Related details
- Justin Novosad
- Lucas Radaelli
- Ramin Halavati
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ramin Halavati (Gerrit)
Ramin Halavati voted and added 5 comments![]( "Open in Gerrit")
Votes added by Ramin Halavati
| Commit-Queue | +1 |
5 comments
#if BUILDFLAG(ENABLE_SCREEN_AI_BROWSERTESTS) && !BUILDFLAG(USE_FAKE_SCREEN_AI)Why is this buildflag combo important? Explain it in a comment.
Done
this param is for OCR, right? Since bool values are not self-documenting (unlike enums), it would be good to add a comment to document what this parameter is.
Done
const ctx = canvas.getContext('2d', { willReadFrequently: true });Justify why you're using willReadFrequently: true here. I assume it's to get code coverage for the mojo pixel format issue you fixed, but it would be good to document that. Also it would be good to ensure that there is adequate coverage for canvas that don't have this option, which is the most common case. Also, I'm not sure this flag actually does anything when running on CQ bot. The main effect of this flag is to disable GPU acceleration, which is already disabled in most test suites.
Gemini believes that this is only needed for headless browser tests, and I tried the same test cases locally without `withReadFrequently` and it worked. I added a comment, does it make sense?
const ctx = canvas.getContext('2d', { willReadFrequently: true });Ramin Halavatisame here
Done
const ctx = canvas.getContext('2d', { willReadFrequently: true });Ramin Halavatiand here
Done
Related details
- Justin Novosad
- Justin Novosad
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad voted and added 4 comments![]( "Open in Gerrit")
Votes added by Justin Novosad
| Code-Review | +1 |
4 comments
const ctx = canvas.getContext('2d', { willReadFrequently: true });Ramin HalavatiJustify why you're using willReadFrequently: true here. I assume it's to get code coverage for the mojo pixel format issue you fixed, but it would be good to document that. Also it would be good to ensure that there is adequate coverage for canvas that don't have this option, which is the most common case. Also, I'm not sure this flag actually does anything when running on CQ bot. The main effect of this flag is to disable GPU acceleration, which is already disabled in most test suites.
Gemini believes that this is only needed for headless browser tests, and I tried the same test cases locally without `withReadFrequently` and it worked. I added a comment, does it make sense?
I think you should remove it if it's not needed. It's an uncommon option and I'm concerned it could skew coverage.
// to ensure immediate frame presentation in static/headless browserThis is not true. Frame finalization is driven by base::TaskObserver logic, which is independent of the async rendering pipeline and frame presentation flow. GPU acceleration has no impact on the triggering of frame finalization.
if (bitmap.colorType() != kN32_SkColorType) {
SkBitmap n32_bitmap;
if (n32_bitmap.tryAllocPixels(
bitmap.info().makeColorType(kN32_SkColorType)) &&
bitmap.readPixels(n32_bitmap.info(), n32_bitmap.getPixels(),
n32_bitmap.rowBytes(), 0, 0)) {
bitmap = n32_bitmap;
} else {
return;
}
}According to the coverage analysis, this block is lacking test coverage. Would be important to get it covered I think.
Related details
- Justin Novosad
- Lucas Radaelli
- Ramin Halavati
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ramin Halavati (Gerrit)
Ramin Halavati voted and added 4 comments![]( "Open in Gerrit")
Votes added by Ramin Halavati
| Commit-Queue | +1 |
4 comments
const ctx = canvas.getContext('2d', { willReadFrequently: true });Ramin HalavatiJustify why you're using willReadFrequently: true here. I assume it's to get code coverage for the mojo pixel format issue you fixed, but it would be good to document that. Also it would be good to ensure that there is adequate coverage for canvas that don't have this option, which is the most common case. Also, I'm not sure this flag actually does anything when running on CQ bot. The main effect of this flag is to disable GPU acceleration, which is already disabled in most test suites.
Justin NovosadGemini believes that this is only needed for headless browser tests, and I tried the same test cases locally without `withReadFrequently` and it worked. I added a comment, does it make sense?
I think you should remove it if it's not needed. It's an uncommon option and I'm concerned it could skew coverage.
The tests fail if this is removed, but I removed it and opened the HTML using a proper chromium and it works. So as the comment says, we only need it for tests. I don't know if we can somehow else fix the issue with the tests.
// to ensure immediate frame presentation in static/headless browserThis is not true. Frame finalization is driven by base::TaskObserver logic, which is independent of the async rendering pipeline and frame presentation flow. GPU acceleration has no impact on the triggering of frame finalization.
Is it correct now?
if (bitmap.colorType() != kN32_SkColorType) {
SkBitmap n32_bitmap;
if (n32_bitmap.tryAllocPixels(
bitmap.info().makeColorType(kN32_SkColorType)) &&
bitmap.readPixels(n32_bitmap.info(), n32_bitmap.getPixels(),
n32_bitmap.rowBytes(), 0, 0)) {
bitmap = n32_bitmap;
} else {
return;
}
}According to the coverage analysis, this block is lacking test coverage. Would be important to get it covered I think.
It seems that this cannot happen now as the call eventually arrives at `SkImage_Base::onAsLegacyBitmap` which makes an N32 bitmap. However I think having this block to protect future changes would be good. WDYT?
I can also add a dump without crash instead to follow up if it happens in future, but it won't be enough to catch attention.
Related details
- Justin Novosad
- Justin Novosad
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad voted and added 4 comments![]( "Open in Gerrit")
Votes added by Justin Novosad
| Code-Review | +1 |
LGTM
const ctx = canvas.getContext('2d', { willReadFrequently: true });Ramin HalavatiJustify why you're using willReadFrequently: true here. I assume it's to get code coverage for the mojo pixel format issue you fixed, but it would be good to document that. Also it would be good to ensure that there is adequate coverage for canvas that don't have this option, which is the most common case. Also, I'm not sure this flag actually does anything when running on CQ bot. The main effect of this flag is to disable GPU acceleration, which is already disabled in most test suites.
Justin NovosadGemini believes that this is only needed for headless browser tests, and I tried the same test cases locally without `withReadFrequently` and it worked. I added a comment, does it make sense?
Ramin HalavatiI think you should remove it if it's not needed. It's an uncommon option and I'm concerned it could skew coverage.
The tests fail if this is removed, but I removed it and opened the HTML using a proper chromium and it works. So as the comment says, we only need it for tests. I don't know if we can somehow else fix the issue with the tests.
Okay, got it.
// to ensure immediate frame presentation in static/headless browserRamin HalavatiThis is not true. Frame finalization is driven by base::TaskObserver logic, which is independent of the async rendering pipeline and frame presentation flow. GPU acceleration has no impact on the triggering of frame finalization.
Is it correct now?
Okay, this makes more sense. Would be nice to fix the issue at the source, b ut that's outside the scope of this CL.
if (bitmap.colorType() != kN32_SkColorType) {
SkBitmap n32_bitmap;
if (n32_bitmap.tryAllocPixels(
bitmap.info().makeColorType(kN32_SkColorType)) &&
bitmap.readPixels(n32_bitmap.info(), n32_bitmap.getPixels(),
n32_bitmap.rowBytes(), 0, 0)) {
bitmap = n32_bitmap;
} else {
return;
}
}Ramin HalavatiAccording to the coverage analysis, this block is lacking test coverage. Would be important to get it covered I think.
It seems that this cannot happen now as the call eventually arrives at `SkImage_Base::onAsLegacyBitmap` which makes an N32 bitmap. However I think having this block to protect future changes would be good. WDYT?
I can also add a dump without crash instead to follow up if it happens in future, but it won't be enough to catch attention.
If you're confident it cannot happen anymore, this should probably just be a CHECK. IMHO, No need for a dump without crash if test coverage is sufficient to catch regressions in this area.
Related details
- Justin Novosad
- Lucas Radaelli
- Ramin Halavati
Ramin Halavati (Gerrit)
Ramin Halavati voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ramin Halavati
| Commit-Queue | +1 |
1 comment
if (bitmap.colorType() != kN32_SkColorType) {
SkBitmap n32_bitmap;
if (n32_bitmap.tryAllocPixels(
bitmap.info().makeColorType(kN32_SkColorType)) &&
bitmap.readPixels(n32_bitmap.info(), n32_bitmap.getPixels(),
n32_bitmap.rowBytes(), 0, 0)) {
bitmap = n32_bitmap;
} else {
return;
}
}Ramin HalavatiAccording to the coverage analysis, this block is lacking test coverage. Would be important to get it covered I think.
Justin NovosadIt seems that this cannot happen now as the call eventually arrives at `SkImage_Base::onAsLegacyBitmap` which makes an N32 bitmap. However I think having this block to protect future changes would be good. WDYT?
I can also add a dump without crash instead to follow up if it happens in future, but it won't be enough to catch attention.
If you're confident it cannot happen anymore, this should probably just be a CHECK. IMHO, No need for a dump without crash if test coverage is sufficient to catch regressions in this area.
- Justin Novosad
- Justin Novosad
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |