Refactor NavigationInitiatorActivationAndAdStatus to 2 bools [chromium/src : main]
Yao Xiao (Gerrit)
Yao Xiao added 2 comments![]( "Open in Gerrit")
rakina@/dcheng@/jkarlin@: Per Josh's suggestion, I've refactored this CL to use 2 bools. This now affects more files and I've included specific implementation notes in the updated CL description. Please take another look. Thanks!
enum NavigationInitiatorActivationAndAdStatus {Josh KarlinI'd strongly prefer 2 bools to this squashing of rather different concepts (transient activation and ad relatedness). For instance, the code in render_frame_host_impl.cc is quite verbose. I further see that the code then tries to work back from this 4-state enum into boolean values almost everywhere that it's actually used. Let's just use bools.
Yao Xiaoor 2-state enums for type safety
Done. I use 2 bools instead of 2-state enums to match existing patterns.
Related details
- Daniel Cheng
- Josh Karlin
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josh Karlin (Gerrit)
Josh Karlin voted and added 3 comments![]( "Open in Gerrit")
Votes added by Josh Karlin
| Code-Review | +1 |
3 comments
lgtm
// TODO(yaoxia): Implement `started_by_ad` and `has_user_gesture`. ThisCan you create a bug for this?
enum NavigationInitiatorActivationAndAdStatus {Josh KarlinI'd strongly prefer 2 bools to this squashing of rather different concepts (transient activation and ad relatedness). For instance, the code in render_frame_host_impl.cc is quite verbose. I further see that the code then tries to work back from this 4-state enum into boolean values almost everywhere that it's actually used. Let's just use bools.
Yao Xiaoor 2-state enums for type safety
Done. I use 2 bools instead of 2-state enums to match existing patterns.
- Daniel Cheng
- Rakina Zata Amni
- Yao Xiao
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Sorry for missing this CL; I've been out a bit for personal reasons.
@d...@chromium.org, can you help with reviewing this? I'll stamp afterwards.
Related details
- Dominic Farolino
- Rakina Zata Amni
- Yao Xiao
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. |
Charlie Reis (Gerrit)
Charlie Reis added 1 comment![]( "Open in Gerrit")
bool from_frame_proxy,Quick drive-by note: It looks risky to change a bool's meaning without the compiler requiring any changes to callers. If we aren't going to use enums (which seems safer), can this parameter at least be moved elsewhere in the parameter list so that the code won't compile until all the callers have been updated to pass `from_frame_proxy` and not `has_user_gesture`?
Also, the new parameter should be documented.
Yao Xiao (Gerrit)
Yao Xiao added 2 comments![]( "Open in Gerrit")
// TODO(yaoxia): Implement `started_by_ad` and `has_user_gesture`. ThisCan you create a bug for this?
Done. Created crbug.com/470115250 and updated the TODO link.
Quick drive-by note: It looks risky to change a bool's meaning without the compiler requiring any changes to callers. If we aren't going to use enums (which seems safer), can this parameter at least be moved elsewhere in the parameter list so that the code won't compile until all the callers have been updated to pass `from_frame_proxy` and not `has_user_gesture`?
Also, the new parameter should be documented.
Done. I reordered the parameters to force compilation errors at call sites that haven't been updated. Also documented the new param.
Related details
- Charlie Reis
- Dominic Farolino
- Rakina Zata Amni
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. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 2 comments![]( "Open in Gerrit")
/*started_with_transient_activation*/nit: add `=`
enum NavigationInitiatorActivationAndAdStatus {Josh KarlinI'd strongly prefer 2 bools to this squashing of rather different concepts (transient activation and ad relatedness). For instance, the code in render_frame_host_impl.cc is quite verbose. I further see that the code then tries to work back from this 4-state enum into boolean values almost everywhere that it's actually used. Let's just use bools.
Yao Xiaoor 2-state enums for type safety
Josh KarlinDone. I use 2 bools instead of 2-state enums to match existing patterns.
Thanks, this is much better!
Thanks, I think looking at the changed usages of the previous enum, it looks like it has grown out of its initial use of ads-specific case, so decoupling them is the right move. However I think having a new `has_transient_activation` field separate from the existing `CommonNavigationParams` `has_user_gesture` field is likely to be confusing even with the comments, as it isn't obvious if someone needs to think about which one to use.
I think this might be the time to make `has_user_gesture` to be an enum with the {"no / filtered user gesture", "has user gesture, but filter before sending to renderer", "has user_gesture, no need to filter"} states (from the previous [discussion](https://chromium-review.googlesource.com/c/chromium/src/+/4017155/comment/fb7b8b4f_defa6f15/) on this). Or if that is going to affect too many code (likely), maybe a reasonable alternative is to make it very clear that the states are linked with comments but also by naming the new bool `has_user_gesture_unfiltered` or the old one `has_transient_user_activation_filtered` or something like that. WDYT?
Related details
- Charlie Reis
- Dominic Farolino
- Yao Xiao