Apply favicon theming consistently in bookmark menus [chromium/src : main]
qian zhuoyu (Gerrit)
qian zhuoyu voted and added 1 comment![]( "Open in Gerrit")
Votes added by qian zhuoyu
| Commit-Queue | +1 |
1 comment
Hello Darryl,
Can you please have a look at this change?
Thank you!
Related details
- Darryl James
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
1 comment
- qian zhuoyu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
DCHECK(node->is_url());There might be an issue here since the node could be a folder or URL, I would remove the DCHECK and check both paths before returning
If folder
return folder favicon
else
return themed favicon or regular favicon for url node
Related details
- 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 1 comment![]( "Open in Gerrit")
DCHECK(node->is_url());There might be an issue here since the node could be a folder or URL, I would remove the DCHECK and check both paths before returning
If folder
return folder favicon
else
return themed favicon or regular favicon for url node
Thanks for the review! I believe folders won't reach this point because:
GetFaviconForNode() is only called from BuildMenuForURLAt() (which only handles URL nodes) and BookmarkNodeFaviconChanged() (which only fires for URL nodes since folders don't have favicons).
Even if a folder were passed, model->GetFavicon(node) would return an empty image, causing an early return before the DCHECK.
The DCHECK serves as a safety assertion to catch any incorrect usage in debug builds. If you prefer, I could add a comment explaining this, what do you think?
Related details
- Darryl James
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. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
DCHECK(node->is_url());qian zhuoyuThere might be an issue here since the node could be a folder or URL, I would remove the DCHECK and check both paths before returning
If folder
return folder favicon
else
return themed favicon or regular favicon for url node
Thanks for the review! I believe folders won't reach this point because:
GetFaviconForNode() is only called from BuildMenuForURLAt() (which only handles URL nodes) and BookmarkNodeFaviconChanged() (which only fires for URL nodes since folders don't have favicons).
Even if a folder were passed, model->GetFavicon(node) would return an empty image, causing an early return before the DCHECK.
The DCHECK serves as a safety assertion to catch any incorrect usage in debug builds. If you prefer, I could add a comment explaining this, what do you think?
Got it - yes, in this case I recommend adding a comment explaining why the node cannot be a folder or other non-url node. Thanks for the context!
Related details
- 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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by qian zhuoyu
| Commit-Queue | +1 |
1 comment
Hi Alison,
Could you please have a look at this change?
Thanks!
Related details
- Alison Gale
- Darryl James
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 1 comment![]( "Open in Gerrit")
qian zhuoyuThere might be an issue here since the node could be a folder or URL, I would remove the DCHECK and check both paths before returning
If folder
return folder favicon
else
return themed favicon or regular favicon for url node
Darryl JamesThanks for the review! I believe folders won't reach this point because:
GetFaviconForNode() is only called from BuildMenuForURLAt() (which only handles URL nodes) and BookmarkNodeFaviconChanged() (which only fires for URL nodes since folders don't have favicons).
Even if a folder were passed, model->GetFavicon(node) would return an empty image, causing an early return before the DCHECK.
The DCHECK serves as a safety assertion to catch any incorrect usage in debug builds. If you prefer, I could add a comment explaining this, what do you think?
Got it - yes, in this case I recommend adding a comment explaining why the node cannot be a folder or other non-url node. Thanks for the context!
Done
Related details
- Alison Gale
- Darryl James
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |