Propagate AvatarIconType from ProfileAttributesEntry::GetAvatarIcon() [chromium/src : main]
qian zhuoyu (Gerrit)
qian zhuoyu added 1 comment![]( "Open in Gerrit")
Hi Nicolas,
This change is from the comment of last commit (https://chromium-review.googlesource.com/c/chromium/src/+/7587374/comment/b15464f6_2326cf94/).
Could you please have a look at it?
Thank you!
Related details
- Nicolas Dossou-Gbété
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas Dossou-Gbété (Gerrit)
Nicolas Dossou-Gbété voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nicolas Dossou-Gbété
| Code-Review | +1 |
3 comments
Thanks for the quick follow-up!
gfx::Image ProfileAttributesEntry::GetAvatarIcon(opening a can of worms maybe but... @rs...@google.com or @dro...@chromium.org I see that the functions called here are returning pointers or references, and generally keep the image itself in a map in a resource bundle... so we'd be making copies of the image by returning it here right? Is that an issue?
if (image)nit: add braces please, and ditto in other places in this function where we use brace-less `if`. ([styleguide](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#:~:text=Use%20%7B%7D%20on%20all%20conditionals/loops)). Or we could also do
```suggestion
if (const gfx::Image* image = GetGAIAPicture()) {
```
Related details
- David Roger
- Ryan Sultanem
- qian zhuoyu
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. |
qian zhuoyu (Gerrit)
qian zhuoyu added 3 comments![]( "Open in Gerrit")
gfx::Image ProfileAttributesEntry::GetAvatarIcon(opening a can of worms maybe but... @rs...@google.com or @dro...@chromium.org I see that the functions called here are returning pointers or references, and generally keep the image itself in a map in a resource bundle... so we'd be making copies of the image by returning it here right? Is that an issue?
Good question! gfx::Image uses a refcounted backing store internally (ImageSkia → ImageSkiaStorage, which inherits from base::RefCountedThreadSafe), so I believe copying a gfx::Image just bumps the refcount rather than duplicating any pixel data. The cost should be trivial.
In my opinion this shouldn't be a concern here — GetAvatarIcon() was already returning gfx::Image by value before this change, so this isn't new behavior we're introducing.
nit: add braces please, and ditto in other places in this function where we use brace-less `if`. ([styleguide](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#:~:text=Use%20%7B%7D%20on%20all%20conditionals/loops)). Or we could also do
```suggestion
if (const gfx::Image* image = GetGAIAPicture()) {
```
- David Roger
- Nicolas Dossou-Gbété
- Ryan Sultanem
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. |