[iOS] Fix ShouldTriggerHelpUI Being Called Multiple Times [chromium/src : main]
Joemer Ramos (Gerrit)
Joemer Ramos voted and added 3 comments![]( "Open in Gerrit")
Votes added by Joemer Ramos
| Commit-Queue | +1 |
3 comments
I found the issue, during migration, one of the timer variables weren't being used properly. This **doesn't** break M144 but we do need to cherry pick this into M145.
weak_ptr_factory_.InvalidateWeakPtrs();Joemer RamosI see a lot of uses of the `weak_ptr_factory_` in this class. This won't mess up any other uses?
Joemer Ramos@ada...@google.com do you have any thoughts here? I thought about this too. Is there a way around this? Maybe I should create a separate weak pointer factory for `AskGeminiChip`?
Disregard
if (previous_main_frame_url_ != main_frame_url) {Joemer RamosIIUC, your theory is that this callback is occurring multiple times, which calls the FET multiple times as a result. Is that right?
This condition is meant to handle that, which would return early if the URL has changed since the request was made
Joemer Ramos`previous_main_frame_url_ = current_url;` only happens in `DidFinishNavigation()`. Is it possible that this condition is not doing what we want? i.e. we wouldn't return early if the eligible site that the user navigates to doesn't reach a state which would call `DidFinishNavigation()`. So:
1. User goes to eligible site A
2. DidFinishNavigation is called
3. Callback A is kicked off
4. User goes to eligible site B but hasn't finished navigating yet.
5. Callback A returns (passing the URL early return check)
6. FET trigger happens and IPH is shown
7. Callback B is kicked off
8. Callback B returns (passing the URL early return check)
9. FET trigger happens and hits DCHECK as the IPH has not dismissedSomething along these lines + what I commented [here](https://chromium-review.googlesource.com/c/chromium/src/+/7301235/comments/f10df804_13bcbd13) in a discussion with Robbie. I suppose that with Robbie's comment of how this DCHECK gets hit, maybe we can check if the promo timer isn't null which implies that the chip is currently showing WDYT? I think in general, we need to avoid this:
```
And kIPHiOSGeminiContextualCueChip's FET rules have nothing that
would prevent a 2nd call from returning false (i.e there's no session_rate
rule, the trigger rule allows for multiple displays in 1 day).So it's your responsibility to make sure that you don't
call ShouldTriggerHelpUI while the IPH is currently being displayed.
```
Disregard
Related details
- Adam Arcaro
- Robbie Gibson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 1 comment![]( "Open in Gerrit")
I found the issue, during migration, one of the timer variables weren't being used properly. This **doesn't** break M144 but we do need to cherry pick this into M145.
Nevermind, M145 branches Jan 12, so this can just normally roll in