feat: Adds gemini eligibility within the tab helper [chromium/src : main]
Adam Arcaro (Gerrit)
Adam Arcaro added 4 comments![]( "Open in Gerrit")
bool is_gemini_eligible_ = false;Let's default to `true` since the great majority of sites are eligible, and Gemini also checks eligibility itself before sending a response. I would rather risk allowing some pages to appear as eligible than preventing eligible pages from being used
if (IsZeroStateSuggestionsEnabled()) {We don't want to depend on the zero state suggestions flag for this anymore, otherwise we'll never get `OnCanApplyZeroStateSuggestionsDecision` without the feature enabled
Can you extract the shared functionality from this function and just flag guard the code that's specific to zero state suggestions? We probably want to generalize `zero_state_suggestions_->url` to track URL checking for multiple features
void BwgTabHelper::OnCanApplyZeroStateSuggestionsDecision(As part of this improvement, we should rename this since it's not just used for zero state suggestions
Related details
- Francis Beauchamp
- Nicolas MacBeth
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. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 3 comments![]( "Open in Gerrit")
bool is_gemini_eligible_ = false;Let's default to `true` since the great majority of sites are eligible, and Gemini also checks eligibility itself before sending a response. I would rather risk allowing some pages to appear as eligible than preventing eligible pages from being used
Agreed!
if (IsZeroStateSuggestionsEnabled()) {We don't want to depend on the zero state suggestions flag for this anymore, otherwise we'll never get `OnCanApplyZeroStateSuggestionsDecision` without the feature enabled
Can you extract the shared functionality from this function and just flag guard the code that's specific to zero state suggestions? We probably want to generalize `zero_state_suggestions_->url` to track URL checking for multiple features
Just to make sure I get this right, you're suggesting that I move `CanApplyOptimization` outside of `IsZeroStateSuggestionsEnabled` with all of the conditionals that lead to it? In other words, leaving just this code behind the flag:
```
weak_ptr_factory_.InvalidateWeakPtrs();
ClearZeroStateSuggestions();
zero_state_suggestions_->url = current_url;
```
I'm not certain what you mean exactly by "We probably want to generalize `zero_state_suggestions_->url` to track URL checking for multiple features". Would you care to elaborate on that?
void BwgTabHelper::OnCanApplyZeroStateSuggestionsDecision(As part of this improvement, we should rename this since it's not just used for zero state suggestions
It isn't just for zero state suggestions, but it also have a strong relationship to it. What I can do is first gate it's content that is specific to zero state suggestion by the `IsZeroStateSuggestionsEnabled()` flag. Then, we can rename for something more generic like `OnGeminiEligibilityDecision` (really not sure on that one). Additionally, `OnCanApplyZeroStateSuggestionsOnDemandDecision` will have to be renamed for `OnGeminiEligibilityOnDemandDecision`.
Thoughts?
Related details
- Adam Arcaro
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. |
Adam Arcaro (Gerrit)
Adam Arcaro added 2 comments![]( "Open in Gerrit")
if (IsZeroStateSuggestionsEnabled()) {Francis BeauchampWe don't want to depend on the zero state suggestions flag for this anymore, otherwise we'll never get `OnCanApplyZeroStateSuggestionsDecision` without the feature enabled
Can you extract the shared functionality from this function and just flag guard the code that's specific to zero state suggestions? We probably want to generalize `zero_state_suggestions_->url` to track URL checking for multiple features
Just to make sure I get this right, you're suggesting that I move `CanApplyOptimization` outside of `IsZeroStateSuggestionsEnabled` with all of the conditionals that lead to it? In other words, leaving just this code behind the flag:
```
weak_ptr_factory_.InvalidateWeakPtrs();
ClearZeroStateSuggestions();
zero_state_suggestions_->url = current_url;
```I'm not certain what you mean exactly by "We probably want to generalize `zero_state_suggestions_->url` to track URL checking for multiple features". Would you care to elaborate on that?
Yes, moving the following behind the zero state flag sgtm! But maybe we should leave the pointer invalidation
```
ClearZeroStateSuggestions();
zero_state_suggestions_->url = current_url;
```
Right now, we use `zero_state_suggestions_->url` to track the last URL to determine whether we need to perform another optimization guide check. We'll now need to track the URL more generally since the optimization guide check isn't just used for ZSS. Let me know if that's clear and we can chat if you have any questions
void BwgTabHelper::OnCanApplyZeroStateSuggestionsDecision(Francis BeauchampAs part of this improvement, we should rename this since it's not just used for zero state suggestions
It isn't just for zero state suggestions, but it also have a strong relationship to it. What I can do is first gate it's content that is specific to zero state suggestion by the `IsZeroStateSuggestionsEnabled()` flag. Then, we can rename for something more generic like `OnGeminiEligibilityDecision` (really not sure on that one). Additionally, `OnCanApplyZeroStateSuggestionsOnDemandDecision` will have to be renamed for `OnGeminiEligibilityOnDemandDecision`.
Thoughts?
Those new names sgtm, since they do indeed just check eligibility for using the page context
Related details
- Francis Beauchamp
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. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 3 comments![]( "Open in Gerrit")
I have refactored the `DidStartNavigation` to take your comment into account. The url held within the `ZeroStateSuggestions` has been removed in favor of the `current_url_` which was recently added to the tab helper. As such, we no longer rely on the ZSS for the eligibility checks, and they remain gated by the url having changed. Now, one side effect which I'm not entirely certain and would like feedback on is the difference between a simple `GetUrl()` and one with `GetWithoutRef()` and what implications it may have on the refactored code. Some new code added and flagged by `IsGeminiCopresenceEnabled()` don't use the `GetWithoutRef` and I'm wondering if it was by design or a simple omission with no consequences.
Francis BeauchampLet's default to `true` since the great majority of sites are eligible, and Gemini also checks eligibility itself before sending a response. I would rather risk allowing some pages to appear as eligible than preventing eligible pages from being used
Agreed!
Done
void BwgTabHelper::OnCanApplyZeroStateSuggestionsDecision(Francis BeauchampAs part of this improvement, we should rename this since it's not just used for zero state suggestions
Adam ArcaroIt isn't just for zero state suggestions, but it also have a strong relationship to it. What I can do is first gate it's content that is specific to zero state suggestion by the `IsZeroStateSuggestionsEnabled()` flag. Then, we can rename for something more generic like `OnGeminiEligibilityDecision` (really not sure on that one). Additionally, `OnCanApplyZeroStateSuggestionsOnDemandDecision` will have to be renamed for `OnGeminiEligibilityOnDemandDecision`.
Thoughts?
Those new names sgtm, since they do indeed just check eligibility for using the page context
- Adam Arcaro
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. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 1 comment![]( "Open in Gerrit")
Francis BeauchampWe don't want to depend on the zero state suggestions flag for this anymore, otherwise we'll never get `OnCanApplyZeroStateSuggestionsDecision` without the feature enabled
Can you extract the shared functionality from this function and just flag guard the code that's specific to zero state suggestions? We probably want to generalize `zero_state_suggestions_->url` to track URL checking for multiple features
Adam ArcaroJust to make sure I get this right, you're suggesting that I move `CanApplyOptimization` outside of `IsZeroStateSuggestionsEnabled` with all of the conditionals that lead to it? In other words, leaving just this code behind the flag:
```
weak_ptr_factory_.InvalidateWeakPtrs();
ClearZeroStateSuggestions();
zero_state_suggestions_->url = current_url;
```I'm not certain what you mean exactly by "We probably want to generalize `zero_state_suggestions_->url` to track URL checking for multiple features". Would you care to elaborate on that?
Yes, moving the following behind the zero state flag sgtm! But maybe we should leave the pointer invalidation
```
ClearZeroStateSuggestions();
zero_state_suggestions_->url = current_url;
```Right now, we use `zero_state_suggestions_->url` to track the last URL to determine whether we need to perform another optimization guide check. We'll now need to track the URL more generally since the optimization guide check isn't just used for ZSS. Let me know if that's clear and we can chat if you have any questions
Done
Related details
- Adam Arcaro
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Arcaro (Gerrit)
Adam Arcaro added 4 comments![]( "Open in Gerrit")
Looking good! Check out a few more comments
const GURL& new_url = navigation_context->GetUrl().GetWithoutRef();In order to better handle single-page applications, we should remove `GetWithoutRef` for the zero-state suggestions check (even though I think this was the existing behavior)
ClearZeroStateSuggestions();This should be behind the `IsZeroStateSuggestionsEnabled` flag
// The URL has changed so the metadata is obsolete.When `IsGeminiCopresenceEnabled` is true, we need to inform the BwgBrowserAgent of any changes to the page context, including eligibility updates. Therefore we should add this block here:
```
if (IsGeminiCopresenceEnabled()) {
for (auto& observer : observers_) {
observer.OnPageContextUpdated(web_state_);
}
}
```
Related details
- Francis Beauchamp
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. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 3 comments![]( "Open in Gerrit")
const GURL& new_url = navigation_context->GetUrl().GetWithoutRef();In order to better handle single-page applications, we should remove `GetWithoutRef` for the zero-state suggestions check (even though I think this was the existing behavior)
Just to make sure we are on the same page; we are to remove the `GetWithoutRef` which will result in all of the tab helper code relying on the URL to include the `#` parts? I would assume that links within a page that simply lead to an anchor within the same page would result in the equality check to return `false`, thus reloading the ZSS and all that (which might be counter intuitive).
ClearZeroStateSuggestions();This should be behind the `IsZeroStateSuggestionsEnabled` flag
Since the function already gates the entirety of its statements (leading to NO-OP), it felt extraneous. Happy to add the double check here if you think it is necessary in case the content of the function changes someday. I personally find that adding checks everywhere is rather verbose and noisy, as opposed to gating the code once within the functions. Do we have a clear preference in our codebase?
// The URL has changed so the metadata is obsolete.When `IsGeminiCopresenceEnabled` is true, we need to inform the BwgBrowserAgent of any changes to the page context, including eligibility updates. Therefore we should add this block here:
```
if (IsGeminiCopresenceEnabled()) {
for (auto& observer : observers_) {
observer.OnPageContextUpdated(web_state_);
}
}
```
I see; it seems like that exact behaviour is already what exists in `DidStartNavigation`. I will extract it in a function and call it from here.
Related details
- Adam Arcaro
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. |
Adam Arcaro (Gerrit)
Adam Arcaro added 3 comments![]( "Open in Gerrit")
const GURL& new_url = navigation_context->GetUrl().GetWithoutRef();Francis BeauchampIn order to better handle single-page applications, we should remove `GetWithoutRef` for the zero-state suggestions check (even though I think this was the existing behavior)
Just to make sure we are on the same page; we are to remove the `GetWithoutRef` which will result in all of the tab helper code relying on the URL to include the `#` parts? I would assume that links within a page that simply lead to an anchor within the same page would result in the equality check to return `false`, thus reloading the ZSS and all that (which might be counter intuitive).
For SPAs, the `#` part isn't always simply an anchor within the page. Use gmail.com as an example - the "inbox" URL is https://mail.google.com/mail/u/0/#inbox, and the "sent" URL is https://mail.google.com/mail/u/0/#sent, which have entirely different content and thus different zero-state suggestions
However, it is true that the Gemini page eligibility check should be consistent for both. We should probably store the full URL, and then add a `GetWithoutRef` check to determine whether to make another eligibility call, but ignore that for the suggestions check
Does that make sense?
ClearZeroStateSuggestions();Francis BeauchampThis should be behind the `IsZeroStateSuggestionsEnabled` flag
Since the function already gates the entirety of its statements (leading to NO-OP), it felt extraneous. Happy to add the double check here if you think it is necessary in case the content of the function changes someday. I personally find that adding checks everywhere is rather verbose and noisy, as opposed to gating the code once within the functions. Do we have a clear preference in our codebase?
For readability, I would suggest either moving the flag check here or just duplicating it. Flag checks are temporary code so I don't feel too strongly, but it's misleading to see an unchecked `ClearZeroStateSuggestions` call on each navigation
Francis BeauchampWhen `IsGeminiCopresenceEnabled` is true, we need to inform the BwgBrowserAgent of any changes to the page context, including eligibility updates. Therefore we should add this block here:
```
if (IsGeminiCopresenceEnabled()) {
for (auto& observer : observers_) {
observer.OnPageContextUpdated(web_state_);
}
}
```
I see; it seems like that exact behaviour is already what exists in `DidStartNavigation`. I will extract it in a function and call it from here.
- Francis Beauchamp
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. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 1 comment![]( "Open in Gerrit")
const GURL& new_url = navigation_context->GetUrl().GetWithoutRef();Francis BeauchampIn order to better handle single-page applications, we should remove `GetWithoutRef` for the zero-state suggestions check (even though I think this was the existing behavior)
Adam ArcaroJust to make sure we are on the same page; we are to remove the `GetWithoutRef` which will result in all of the tab helper code relying on the URL to include the `#` parts? I would assume that links within a page that simply lead to an anchor within the same page would result in the equality check to return `false`, thus reloading the ZSS and all that (which might be counter intuitive).
For SPAs, the `#` part isn't always simply an anchor within the page. Use gmail.com as an example - the "inbox" URL is https://mail.google.com/mail/u/0/#inbox, and the "sent" URL is https://mail.google.com/mail/u/0/#sent, which have entirely different content and thus different zero-state suggestions
However, it is true that the Gemini page eligibility check should be consistent for both. We should probably store the full URL, and then add a `GetWithoutRef` check to determine whether to make another eligibility call, but ignore that for the suggestions check
Does that make sense?
My understanding of this is that you want the rest of the code to ignore the presence of the ref (i.e. use the URL as is), but have the check here as:
```
const GURL& new_url = navigation_context->GetUrl();
if (new_url.GetWithoutRef() != current_url_.GetWithoutRef()) { ... }
```
Is that what you were trying to say?
Related details
- Adam Arcaro
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. |
Adam Arcaro (Gerrit)
Adam Arcaro added 1 comment![]( "Open in Gerrit")
const GURL& new_url = navigation_context->GetUrl().GetWithoutRef();Francis BeauchampIn order to better handle single-page applications, we should remove `GetWithoutRef` for the zero-state suggestions check (even though I think this was the existing behavior)
Adam ArcaroJust to make sure we are on the same page; we are to remove the `GetWithoutRef` which will result in all of the tab helper code relying on the URL to include the `#` parts? I would assume that links within a page that simply lead to an anchor within the same page would result in the equality check to return `false`, thus reloading the ZSS and all that (which might be counter intuitive).
Francis BeauchampFor SPAs, the `#` part isn't always simply an anchor within the page. Use gmail.com as an example - the "inbox" URL is https://mail.google.com/mail/u/0/#inbox, and the "sent" URL is https://mail.google.com/mail/u/0/#sent, which have entirely different content and thus different zero-state suggestions
However, it is true that the Gemini page eligibility check should be consistent for both. We should probably store the full URL, and then add a `GetWithoutRef` check to determine whether to make another eligibility call, but ignore that for the suggestions check
Does that make sense?
My understanding of this is that you want the rest of the code to ignore the presence of the ref (i.e. use the URL as is), but have the check here as:
```
const GURL& new_url = navigation_context->GetUrl();
if (new_url.GetWithoutRef() != current_url_.GetWithoutRef()) { ... }
```Is that what you were trying to say?
- Francis Beauchamp
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. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 4 comments![]( "Open in Gerrit")
// Whether the content has been deemed eligible for gemini usage.nit: `Gemini`
if (web_state_->GetVisibleURL().GetWithoutRef() == current_url_) {when would this not be true? we set the new URL right when navigations start, so this would presumably always be true, no? I think the URL in the ZeroState suggestions struct is there because the ZS suggestions themselves are tied to a URL, and this helps them not became stale. CC: @tarekm...@google.com for input here
is_gemini_eligible_ =
decision == optimization_guide::OptimizationGuideDecision::kTrue;I'm not sure this is true, but am really unsure. Look at the below comment (line #737). I think OptiGuide returns kTrue when it has something to say, and the actual result is computed below -> this confuses me too. (`zero_state_suggestions_->can_apply` is equivalent to `is_gemini_eligible_`, no?)
CC: @tarekm...@google.com who worked on this initially, and @ada...@google.com
Related details
- Francis Beauchamp
- Tarek Makkouk
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. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 1 comment![]( "Open in Gerrit")
if (web_state_->GetVisibleURL().GetWithoutRef() == current_url_) {when would this not be true? we set the new URL right when navigations start, so this would presumably always be true, no? I think the URL in the ZeroState suggestions struct is there because the ZS suggestions themselves are tied to a URL, and this helps them not became stale. CC: @tarekm...@google.com for input here
Yeah that's exactly it. The plan was to have the ZSSs tightly coupled with their corresponding URL, which ideally would always be the current one, but ```web_state_->GetVisibleURL().GetWithoutRef()``` will always have the current URL, so the check was added to ensure we don't push stale suggestions, even if it means we have to go with the default suggestions. In short, it's to ensure we fail elegantly if the ZSSs are out of sync for any reason.
Related details
- Francis Beauchamp
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. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 1 comment![]( "Open in Gerrit")
I was wonderring why are we opting to untie the URL from the ZSS? Couldn't we just keep it to ensure that we don't push stale ZSSs and just use the webstate to more accurately get the current URL?
Related details
- Adam Arcaro
- Francis Beauchamp
- Nicolas MacBeth
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. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 1 comment![]( "Open in Gerrit")
is_gemini_eligible_ =
decision == optimization_guide::OptimizationGuideDecision::kTrue;I'm not sure this is true, but am really unsure. Look at the below comment (line #737). I think OptiGuide returns kTrue when it has something to say, and the actual result is computed below -> this confuses me too. (`zero_state_suggestions_->can_apply` is equivalent to `is_gemini_eligible_`, no?)
CC: @tarekm...@google.com who worked on this initially, and @ada...@google.com
I'm not sure if they are equivalent, so I'll defer to @ada...@google.com on that, but if it is the case, that would imply that theoretically we should never see the default suggestions (i.e., summary, FAQ, "What can Gemini do?") as any page eligible for Gemini would also be eligible for ZSSs. I'm not sure if this is the intended functionality and, from what I can recall, I think I experienced the default suggestions in the current state of the app.
Adam Arcaro (Gerrit)
Adam Arcaro added 1 comment![]( "Open in Gerrit")
is_gemini_eligible_ =
decision == optimization_guide::OptimizationGuideDecision::kTrue;Tarek MakkoukI'm not sure this is true, but am really unsure. Look at the below comment (line #737). I think OptiGuide returns kTrue when it has something to say, and the actual result is computed below -> this confuses me too. (`zero_state_suggestions_->can_apply` is equivalent to `is_gemini_eligible_`, no?)
CC: @tarekm...@google.com who worked on this initially, and @ada...@google.com
I'm not sure if they are equivalent, so I'll defer to @ada...@google.com on that, but if it is the case, that would imply that theoretically we should never see the default suggestions (i.e., summary, FAQ, "What can Gemini do?") as any page eligible for Gemini would also be eligible for ZSSs. I'm not sure if this is the intended functionality and, from what I can recall, I think I experienced the default suggestions in the current state of the app.
It's correct that any page eligible for Gemini should also be eligible for ZSS.
The logic for when static chips should be shown is handled within the SDK (e.g. "Summarize" is always shown, "What can Gemini do?" is shown to new users, etc), and the "Create a FAQ" chip is just a fallback.
Pages without metadata are eligible by default (as per Chrome Intelligence guidance)
Related details
- Francis Beauchamp
- Nicolas MacBeth
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. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 1 comment![]( "Open in Gerrit")
is_gemini_eligible_ =
decision == optimization_guide::OptimizationGuideDecision::kTrue;Tarek MakkoukI'm not sure this is true, but am really unsure. Look at the below comment (line #737). I think OptiGuide returns kTrue when it has something to say, and the actual result is computed below -> this confuses me too. (`zero_state_suggestions_->can_apply` is equivalent to `is_gemini_eligible_`, no?)
CC: @tarekm...@google.com who worked on this initially, and @ada...@google.com
Adam ArcaroI'm not sure if they are equivalent, so I'll defer to @ada...@google.com on that, but if it is the case, that would imply that theoretically we should never see the default suggestions (i.e., summary, FAQ, "What can Gemini do?") as any page eligible for Gemini would also be eligible for ZSSs. I'm not sure if this is the intended functionality and, from what I can recall, I think I experienced the default suggestions in the current state of the app.
It's correct that any page eligible for Gemini should also be eligible for ZSS.
The logic for when static chips should be shown is handled within the SDK (e.g. "Summarize" is always shown, "What can Gemini do?" is shown to new users, etc), and the "Create a FAQ" chip is just a fallback.
Pages without metadata are eligible by default (as per Chrome Intelligence guidance)
That's good to know, thank you!
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 1 comment![]( "Open in Gerrit")
if (!IsZeroStateSuggestionsEnabled() || url != current_url_) {some of the below code should ideally run regardless of `ZeroStateSuggestionsEnabled()`. if eligibility is *good*, lines 760 to 768 (GeminiImageRemix code) of this patchset should be executed regardless of `IsZeroStateSuggestionsEnabled()`. thanks!
Related details
- Francis Beauchamp
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. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 4 comments![]( "Open in Gerrit")
if (web_state_->GetVisibleURL().GetWithoutRef() == current_url_) {when would this not be true? we set the new URL right when navigations start, so this would presumably always be true, no? I think the URL in the ZeroState suggestions struct is there because the ZS suggestions themselves are tied to a URL, and this helps them not became stale. CC: @tarekm...@google.com for input here
Yeah that's exactly it. The plan was to have the ZSSs tightly coupled with their corresponding URL, which ideally would always be the current one, but ```web_state_->GetVisibleURL().GetWithoutRef()``` will always have the current URL, so the check was added to ensure we don't push stale suggestions, even if it means we have to go with the default suggestions. In short, it's to ensure we fail elegantly if the ZSSs are out of sync for any reason.
Perhaps @ada...@google.com can confirm the behaviour we are going for; essentially the goal is to use the stored url in the tab helper (which is essentially the same as using a stored URL in the ZSS that gets updated at every `DidStartNavigation`).
Now, perhaps I am mistaken here: my understanding is that since we only store one copy of the ZSS structure, it doesn't matter that we tightly couple it to an URL or use the one in the tab helper: they both get updated at every `DidStartNavigation`.
In any case, I might require some of your inputs to better understand what we want, because this is all quite new to me and I doubt I have all the context to make an appropriate decision.
ClearZeroStateSuggestions();Francis BeauchampThis should be behind the `IsZeroStateSuggestionsEnabled` flag
Adam ArcaroSince the function already gates the entirety of its statements (leading to NO-OP), it felt extraneous. Happy to add the double check here if you think it is necessary in case the content of the function changes someday. I personally find that adding checks everywhere is rather verbose and noisy, as opposed to gating the code once within the functions. Do we have a clear preference in our codebase?
For readability, I would suggest either moving the flag check here or just duplicating it. Flag checks are temporary code so I don't feel too strongly, but it's misleading to see an unchecked `ClearZeroStateSuggestions` call on each navigation
Done
is_gemini_eligible_ =
decision == optimization_guide::OptimizationGuideDecision::kTrue;Tarek MakkoukI'm not sure this is true, but am really unsure. Look at the below comment (line #737). I think OptiGuide returns kTrue when it has something to say, and the actual result is computed below -> this confuses me too. (`zero_state_suggestions_->can_apply` is equivalent to `is_gemini_eligible_`, no?)
CC: @tarekm...@google.com who worked on this initially, and @ada...@google.com
Adam ArcaroI'm not sure if they are equivalent, so I'll defer to @ada...@google.com on that, but if it is the case, that would imply that theoretically we should never see the default suggestions (i.e., summary, FAQ, "What can Gemini do?") as any page eligible for Gemini would also be eligible for ZSSs. I'm not sure if this is the intended functionality and, from what I can recall, I think I experienced the default suggestions in the current state of the app.
Tarek MakkoukIt's correct that any page eligible for Gemini should also be eligible for ZSS.
The logic for when static chips should be shown is handled within the SDK (e.g. "Summarize" is always shown, "What can Gemini do?" is shown to new users, etc), and the "Create a FAQ" chip is just a fallback.
Pages without metadata are eligible by default (as per Chrome Intelligence guidance)
That's good to know, thank you!
I feel like this might benefit from a sync conversation when we are back in the office; there seems to be either a misunderstanding on my part or a misalignment between the requirements that we had VS the ones that have been brought to my attention.
if (!IsZeroStateSuggestionsEnabled() || url != current_url_) {some of the below code should ideally run regardless of `ZeroStateSuggestionsEnabled()`. if eligibility is *good*, lines 760 to 768 (GeminiImageRemix code) of this patchset should be executed regardless of `IsZeroStateSuggestionsEnabled()`. thanks!
My feeling, as I see this function grow (originally much smaller when I started what was supposed to be a small, trivial refactor), is that it is no longer adequately separating the different concerns of updating the page context, the zero state suggestions, and now calling conditional code based on GeminiImageRemix.
We have an increasing number of conditions and this could lead a dip in maintainability. I would suggest, although beyond the scope of the current change, that we start thinking about a better way of separating these concerns such that the side effects can be contained.
On that point, the creation of smaller functional units (i.e. functions) gating their own behaviour as opposed to having a bunch of conditions that can conflict with one another would yield cleaner, more maintainable code.
Related details
- Adam Arcaro
- Nicolas MacBeth
- Tarek Makkouk
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. |
Adam Arcaro (Gerrit)
Adam Arcaro added 5 comments![]( "Open in Gerrit")
2 minor comments and this should be ready to land!
if (web_state_->GetVisibleURL().GetWithoutRef() == current_url_) {Tarek Makkoukwhen would this not be true? we set the new URL right when navigations start, so this would presumably always be true, no? I think the URL in the ZeroState suggestions struct is there because the ZS suggestions themselves are tied to a URL, and this helps them not became stale. CC: @tarekm...@google.com for input here
Francis BeauchampYeah that's exactly it. The plan was to have the ZSSs tightly coupled with their corresponding URL, which ideally would always be the current one, but ```web_state_->GetVisibleURL().GetWithoutRef()``` will always have the current URL, so the check was added to ensure we don't push stale suggestions, even if it means we have to go with the default suggestions. In short, it's to ensure we fail elegantly if the ZSSs are out of sync for any reason.
Perhaps @ada...@google.com can confirm the behaviour we are going for; essentially the goal is to use the stored url in the tab helper (which is essentially the same as using a stored URL in the ZSS that gets updated at every `DidStartNavigation`).
Now, perhaps I am mistaken here: my understanding is that since we only store one copy of the ZSS structure, it doesn't matter that we tightly couple it to an URL or use the one in the tab helper: they both get updated at every `DidStartNavigation`.
In any case, I might require some of your inputs to better understand what we want, because this is all quite new to me and I doubt I have all the context to make an appropriate decision.
This CL just extracted the `zero_staet_suggestions_->url` field into a more generic `current_url_` field, so this change should be a no-op. As Tarek said, the condition was previously added as a safety check to make sure we're generating suggestions for the active web state, and not some other tab
page_context->set_url(current_url_.spec());I just realized that `current_url_` is being used here, which would benefit from the full URL (and not just the ref-stripped version)
Would you mind updating the CL to store the full URL in `current_url_`, but then use the ref-stripped version for comparison?
Thanks for your patience as we figure this out! There are a lot of features at play here
const GURL& new_url = navigation_context->GetUrl().GetWithoutRef();Francis BeauchampIn order to better handle single-page applications, we should remove `GetWithoutRef` for the zero-state suggestions check (even though I think this was the existing behavior)
Adam ArcaroJust to make sure we are on the same page; we are to remove the `GetWithoutRef` which will result in all of the tab helper code relying on the URL to include the `#` parts? I would assume that links within a page that simply lead to an anchor within the same page would result in the equality check to return `false`, thus reloading the ZSS and all that (which might be counter intuitive).
Francis BeauchampFor SPAs, the `#` part isn't always simply an anchor within the page. Use gmail.com as an example - the "inbox" URL is https://mail.google.com/mail/u/0/#inbox, and the "sent" URL is https://mail.google.com/mail/u/0/#sent, which have entirely different content and thus different zero-state suggestions
However, it is true that the Gemini page eligibility check should be consistent for both. We should probably store the full URL, and then add a `GetWithoutRef` check to determine whether to make another eligibility call, but ignore that for the suggestions check
Does that make sense?
Adam ArcaroMy understanding of this is that you want the rest of the code to ignore the presence of the ref (i.e. use the URL as is), but have the check here as:
```
const GURL& new_url = navigation_context->GetUrl();
if (new_url.GetWithoutRef() != current_url_.GetWithoutRef()) { ... }
```Is that what you were trying to say?
Exactly!
Obsolete, resolving
return;Let's remove this `return`, since we still want to generate ZSS if the IPH was shown
Related details
- Francis Beauchamp
- Nicolas MacBeth
- Tarek Makkouk
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. |
Adam Arcaro (Gerrit)
Adam Arcaro added 2 comments![]( "Open in Gerrit")
I was wonderring why are we opting to untie the URL from the ZSS? Couldn't we just keep it to ensure that we don't push stale ZSSs and just use the webstate to more accurately get the current URL?
The ZSS struct should always reflect the current web state, and we already track the current web state's URL in `current_url_`. The way I see it, there should not be a case where `current_url_` and `zero_state_suggestions_->url` are different (the ZSS should just be empty if they don't represent the same URL), and thus both variables feel redundant
Tarek, please let me know if you see this differently. Thanks!
is_gemini_eligible_ =
decision == optimization_guide::OptimizationGuideDecision::kTrue;Tarek MakkoukI'm not sure this is true, but am really unsure. Look at the below comment (line #737). I think OptiGuide returns kTrue when it has something to say, and the actual result is computed below -> this confuses me too. (`zero_state_suggestions_->can_apply` is equivalent to `is_gemini_eligible_`, no?)
CC: @tarekm...@google.com who worked on this initially, and @ada...@google.com
Adam ArcaroI'm not sure if they are equivalent, so I'll defer to @ada...@google.com on that, but if it is the case, that would imply that theoretically we should never see the default suggestions (i.e., summary, FAQ, "What can Gemini do?") as any page eligible for Gemini would also be eligible for ZSSs. I'm not sure if this is the intended functionality and, from what I can recall, I think I experienced the default suggestions in the current state of the app.
Tarek MakkoukIt's correct that any page eligible for Gemini should also be eligible for ZSS.
The logic for when static chips should be shown is handled within the SDK (e.g. "Summarize" is always shown, "What can Gemini do?" is shown to new users, etc), and the "Create a FAQ" chip is just a fallback.
Pages without metadata are eligible by default (as per Chrome Intelligence guidance)
Francis BeauchampThat's good to know, thank you!
I feel like this might benefit from a sync conversation when we are back in the office; there seems to be either a misunderstanding on my part or a misalignment between the requirements that we had VS the ones that have been brought to my attention.
Resolving
Francis Beauchamp (Gerrit)
Francis Beauchamp added 3 comments![]( "Open in Gerrit")
Reshuffled some code as part of the latest updates. Most importantly, the eligibility computation is extracted in a function; its return value is propagated to the `ZSS->canApply` and `is_gemini_eligible`. Additionally, the `GetWithoutRef()` is added to all the places where it was needed, while ensuring we store the full URL that gets added to the page context.
I just realized that `current_url_` is being used here, which would benefit from the full URL (and not just the ref-stripped version)
Would you mind updating the CL to store the full URL in `current_url_`, but then use the ref-stripped version for comparison?
Thanks for your patience as we figure this out! There are a lot of features at play here
Done
Let's remove this `return`, since we still want to generate ZSS if the IPH was shown
- Adam Arcaro
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. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 2 comments![]( "Open in Gerrit")
// Whether the content has been deemed eligible for gemini usage.Francis Beauchampnit: `Gemini`
Done
if (!IsZeroStateSuggestionsEnabled() || url != current_url_) {some of the below code should ideally run regardless of `ZeroStateSuggestionsEnabled()`. if eligibility is *good*, lines 760 to 768 (GeminiImageRemix code) of this patchset should be executed regardless of `IsZeroStateSuggestionsEnabled()`. thanks!
My feeling, as I see this function grow (originally much smaller when I started what was supposed to be a small, trivial refactor), is that it is no longer adequately separating the different concerns of updating the page context, the zero state suggestions, and now calling conditional code based on GeminiImageRemix.
We have an increasing number of conditions and this could lead a dip in maintainability. I would suggest, although beyond the scope of the current change, that we start thinking about a better way of separating these concerns such that the side effects can be contained.
On that point, the creation of smaller functional units (i.e. functions) gating their own behaviour as opposed to having a bunch of conditions that can conflict with one another would yield cleaner, more maintainable code.
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. |
Adam Arcaro (Gerrit)
Adam Arcaro added 4 comments![]( "Open in Gerrit")
Adam ArcaroI was wonderring why are we opting to untie the URL from the ZSS? Couldn't we just keep it to ensure that we don't push stale ZSSs and just use the webstate to more accurately get the current URL?
The ZSS struct should always reflect the current web state, and we already track the current web state's URL in `current_url_`. The way I see it, there should not be a case where `current_url_` and `zero_state_suggestions_->url` are different (the ZSS should just be empty if they don't represent the same URL), and thus both variables feel redundant
Tarek, please let me know if you see this differently. Thanks!
Acknowledged
weak_ptr_factory_.InvalidateWeakPtrs();We should reset `is_gemini_eligible_` to nullptr here since the previous eligibility decision is obsolete
bool eligible = ComputeGeminiEligibility(decision, metadata);nit: We can remove the `eligible` local var since we always assign it to `is_gemini_eligible_` immediately
Related details
- Francis Beauchamp
- Tarek Makkouk
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. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nicolas MacBeth
| Code-Review | +1 |
3 comments
lgtm % 2 nits and Adam's comments. thanks! this looks much cleaner
// Gets the state of Gemini eligibility for the tab.nit: I would add the bit about why this is optional here too, since this is what's public to the class.
const GURL& url,nit: `url_without_ref` here and in the header? just to be extra clear as to what's expected/passed
Related details
- Francis Beauchamp
- Tarek Makkouk
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. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 4 comments![]( "Open in Gerrit")
// Gets the state of Gemini eligibility for the tab.nit: I would add the bit about why this is optional here too, since this is what's public to the class.
Done
We should reset `is_gemini_eligible_` to nullptr here since the previous eligibility decision is obsolete
Done
nit: `url_without_ref` here and in the header? just to be extra clear as to what's expected/passed
Done
bool eligible = ComputeGeminiEligibility(decision, metadata);nit: We can remove the `eligible` local var since we always assign it to `is_gemini_eligible_` immediately
We use it in the `IsGeminiImageRemixToolEnabled` check and it gets assigned as a `bool` to `zero_state_suggestions_->can_apply`. I'm not convinced that it is meaningfully better to rely on an `std::optional` and be forced into unwrapping with `value_or` on these two occasions.
Related details
- Adam Arcaro
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |