[ios] Fix metrics recorded by WebStateListMetricsBrowserAgent [chromium/src : main]
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Commit-Queue | +1 |
2 comments
alexandrasp: can you do a first pass review?
marq: can you review after alexandrasp and then send to CQ+2 if this lgtm to you both?
Please fix this WARNING reported by Large Change: This change adds 476 lines of production code. Consider splitting this change in...
This change adds 476 lines of production code. Consider splitting this change into smaller ones.
Small changes get reviewed faster and more thoroughly, are less likely to introduce bugs, and are easier to roll back if needed.
You can bypass this warning by adding 'BYPASS_LARGE_CHANGE_WARNING: <reason>' to the change description. (Google-internal: See go/large-change-nudge)
Related details
- Alexandra Pereira
- Mark Cogan
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Cogan (Gerrit)
Mark Cogan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Cogan
| Code-Review | +1 |
1 comment
LGTM, although I'm wondering why profile-scoping (rather than app-scoping) is correct here? Is it because we care about the active tabs at the time of the crash which, for each scene, will be per-profile?
Related details
- Alexandra Pereira
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexandra Pereira (Gerrit)
Alexandra Pereira added 7 comments![]( "Open in Gerrit")
LGTM, although I'm wondering why profile-scoping (rather than app-scoping) is correct here? Is it because we care about the active tabs at the time of the crash which, for each scene, will be per-profile?
IIUC, it is exactly because we want it to be per-profile. It will make it easier and the right way to have profile-aware crash keys.
Hi Sylvain, thanks for the CL! I left some comments. I also wanted to ask: is there a reason we don't have any tests here? Would it be possible to cover this with unit tests?
const std::vector<web::WebState*>& restored_web_states) final;don't we need to have web namespace?
const std::vector<web::WebState*>& restored_web_states) final;isn't it necessary to import vector?
#include "base/memory/raw_ref.h"do we need base/memory/raw_ref.h?
CGFloat red = 1.0;are we able to use CGFLoat without importing UIKit?
helpers_.insert(std::make_pair(type, std::make_unique<Helper>(type)));isn't it necessary to import utility ?
Related details
- Sylvain Defresne
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |