Centralize manifest icon selection in a single place [chromium/src : main]
Nate Chapin (Gerrit)
Nate Chapin added 2 comments![]( "Open in Gerrit")
I reworked a few details of the draft you sent me, in addition to ensure it passes tests.
Note that this doesn't do anyhting with manifest filtering - this is just the icon selection.
"sizes": "144x144"Minimum size is 144x144, but this icon isn't actually 144x144 and that's now a failure.
Related details
- Daniel Murphy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy added 4 comments![]( "Open in Gerrit")
"sizes": "144x144"Minimum size is 144x144, but this icon isn't actually 144x144 and that's now a failure.
interesting - I believe the code **should** request to download the icon at the size here, so even though the image is icon128, it should 'download' to a 144x144? Unless the web contents image downloading thing isn't what I think it is.
{".jpg", "image/jpeg"},
{".jpeg", "image/jpeg"},
{".ico", "image/x-icon"},
{".bmp", "image/bmp"},
{".gif", "image/gif"},can you double check this isn't a regression? I believe we only want to do the above types, I think AI just added these for unknown reason - but maybe we used to allow the IsSupportedImageMimeType?
// TODO make this logic a little better - if we are kClosestFit, we likely
// want to use 'any' before non-0 diff. Check previous implementation usage.fyi - this was my todo to fix the algorithm - did you make this change?
class ManifestIconSelectorTest : public testing::TestWithParam<bool> {I worry a little bit about the tests not being the same - is it possible to make an adapter from the old call method to the new one, to be able to run these old tests with the new algorithm?
I yeah - I believe there was a thing where the old behavior would do this:
- icons:
- a: 64
- b: 96
- c: 145
- d: any
If the ManifestIconSelector code was queried for a 144 icon, it would return d instead of c.
Can you make sure that behavior still works?
Related details
- Nate Chapin
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. |
Nate Chapin (Gerrit)
Nate Chapin added 4 comments![]( "Open in Gerrit")
"sizes": "144x144"Daniel MurphyMinimum size is 144x144, but this icon isn't actually 144x144 and that's now a failure.
interesting - I believe the code **should** request to download the icon at the size here, so even though the image is icon128, it should 'download' to a 144x144? Unless the web contents image downloading thing isn't what I think it is.
Reverted this, it's not needed anymore.
{".jpg", "image/jpeg"},
{".jpeg", "image/jpeg"},
{".ico", "image/x-icon"},
{".bmp", "image/bmp"},
{".gif", "image/gif"},can you double check this isn't a regression? I believe we only want to do the above types, I think AI just added these for unknown reason - but maybe we used to allow the IsSupportedImageMimeType?
This appears to be an attetmpt to combine the existing ManifesetIconSelector logic (which was based solely on MIME type) with `IsIconTypeSupported` in installable_evaluator.cc (which also looked at file endings, but only accepted a smaller set of image types). The synthesis is to accept all the image types blink can decode, and to use `net/` helpers to impute MIME type based on file extensions when a MIME type isn't explicitly given. See most recent PS.
// TODO make this logic a little better - if we are kClosestFit, we likely
// want to use 'any' before non-0 diff. Check previous implementation usage.fyi - this was my todo to fix the algorithm - did you make this change?
We used to use `any` before non-0 diff, except in trusted_icon_filter.cc. That's now the main place we use `ManifestIconSelectorParams::AnySizeSvgHandling::kAsFallback` to reverse the order.
I'm still trying to understand whether trusted_icon_filter.cc *should* have different behavior than other cases.
class ManifestIconSelectorTest : public testing::TestWithParam<bool> {I worry a little bit about the tests not being the same - is it possible to make an adapter from the old call method to the new one, to be able to run these old tests with the new algorithm?
I yeah - I believe there was a thing where the old behavior would do this:- icons:
- a: 64
- b: 96
- c: 145
- d: anyIf the ManifestIconSelector code was queried for a 144 icon, it would return d instead of c.
Can you make sure that behavior still works?
I think the `SvgHandling` test covers this behavior - whether c or d will be preferred is determined by the `AnySizeSvgHandling` flag.
Related details
- Daniel Murphy
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. |