Helios Best effort page context generation if page isn't loading [chromium/src : main]
Adam Arcaro (Gerrit)
Adam Arcaro voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adam Arcaro
| Code-Review | +1 |
1 comment
Related details
- Nicolas MacBeth
- Yasaman Sedaghat
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 9 comments![]( "Open in Gerrit")
hey! PTAL at a few comments, thanks!
// Whether to fetch full page context.
bool get_full_page_context_ = true;
as discussed offline, all the non-test calls to this seem to be true. Should we clean it up from the function and here instead of doing this? I feel like this could be prone to bugs if another caller modifies it in the future before the originally setup extraction. WDYT?
void ForcePageContextGeneration();the page loaded callback is reset in PageLoaded. should we document here that this will do nothing if the page has already loaded?
setShouldGetAnnotatedPageContent:get_full_page_context_];with my comment above, these would simply be passed `YES` imo
// Timer to force page context generation if it takes too long.nit here and a few of the comments in this CL: "... if page load takes too long". I think it's more clear otherwise it feels like it's about page context itself, which makes it confusing because why would we try again in that case.
void OnPageContextTimeout();this feels misleading, as if page context generation was already started. what about something closer to: `TriggerBestEffortContextGeneration` or `OnPageLoadWaitTimeout`?
} // namespacesuper nit: newline here, and page context -> page load naming?
page_context_timeout_timer_.Start(FROM_HERE, kFullPageContextTimeout, this,we should pass a weak pointer here (see: `weak_factory_.GetWeakPtr()` above to prevent a crash and UAF)
browser_->GetWebStateList()->GetActiveWebState();we've had crashes where there is no active webstate. can we check for that first? (null check)
Related details
- Yasaman Sedaghat
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 1 comment![]( "Open in Gerrit")
// Start the timeout timer to force page context generation if it takes tooshould this be in the `if(IsGeminiImmediateOverlayEnabled())`?
also the `kStartupTimeWithFREHistogram` histogram in this current state would be logged twice, no?
Greg Thompson (Gerrit)
Greg Thompson removed grt+...@chromium.org from this change![]( "Open in Gerrit")
Related details
- Yasaman Sedaghat
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. |
Yasaman Sedaghat (Gerrit)
Yasaman Sedaghat added 2 comments![]( "Open in Gerrit")
// Whether to fetch full page context.
bool get_full_page_context_ = true;
as discussed offline, all the non-test calls to this seem to be true. Should we clean it up from the function and here instead of doing this? I feel like this could be prone to bugs if another caller modifies it in the future before the originally setup extraction. WDYT?
@tarekm...@google.com, @ada...@google.com since you are more familiar with this field, do we still want to keep it around or just remove it and default to full context?
void ForcePageContextGeneration();the page loaded callback is reset in PageLoaded. should we document here that this will do nothing if the page has already loaded?
- 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. |
Yasaman Sedaghat (Gerrit)
Yasaman Sedaghat added 7 comments![]( "Open in Gerrit")
setShouldGetAnnotatedPageContent:get_full_page_context_];with my comment above, these would simply be passed `YES` imo
I'll wait for Tarek to chime in as we chatted.
// Timer to force page context generation if it takes too long.nit here and a few of the comments in this CL: "... if page load takes too long". I think it's more clear otherwise it feels like it's about page context itself, which makes it confusing because why would we try again in that case.
Done
this feels misleading, as if page context generation was already started. what about something closer to: `TriggerBestEffortContextGeneration` or `OnPageLoadWaitTimeout`?
Done
super nit: newline here, and page context -> page load naming?
I have updated all the comments to make it more clear but prefer to keep the timeout variable name as is, because ultimately it is about page context fetching and the fact that we are not going to wait for page load to do full page context load.
// Start the timeout timer to force page context generation if it takes tooshould this be in the `if(IsGeminiImmediateOverlayEnabled())`?
also the `kStartupTimeWithFREHistogram` histogram in this current state would be logged twice, no?
Yes thanks for pointing this out. This flow only works and makes sense for when immediateOverlay is enabled, moved it.
page_context_timeout_timer_.Start(FROM_HERE, kFullPageContextTimeout, this,we should pass a weak pointer here (see: `weak_factory_.GetWeakPtr()` above to prevent a crash and UAF)
Done
we've had crashes where there is no active webstate. can we check for that first? (null check)
- Adam Arcaro
- 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")
// Whether to fetch full page context.
bool get_full_page_context_ = true;
Yasaman Sedaghatas discussed offline, all the non-test calls to this seem to be true. Should we clean it up from the function and here instead of doing this? I feel like this could be prone to bugs if another caller modifies it in the future before the originally setup extraction. WDYT?
@tarekm...@google.com, @ada...@google.com since you are more familiar with this field, do we still want to keep it around or just remove it and default to full context?
If all the production calls pass to true, I agree with Nick. I think keeping it despite that adds unnecessary complexity and could open us up to bugs in the future. I also think we can always add granular control later if a concrete use case arises.
That being said, I defer to @ada...@google.com as he has more context on this specific task.
Related details
- Adam Arcaro
- Nicolas MacBeth
- Yasaman Sedaghat
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")
// Whether to fetch full page context.
bool get_full_page_context_ = true;
Yasaman Sedaghatas discussed offline, all the non-test calls to this seem to be true. Should we clean it up from the function and here instead of doing this? I feel like this could be prone to bugs if another caller modifies it in the future before the originally setup extraction. WDYT?
Tarek Makkouk@tarekm...@google.com, @ada...@google.com since you are more familiar with this field, do we still want to keep it around or just remove it and default to full context?
If all the production calls pass to true, I agree with Nick. I think keeping it despite that adds unnecessary complexity and could open us up to bugs in the future. I also think we can always add granular control later if a concrete use case arises.
That being said, I defer to @ada...@google.com as he has more context on this specific task.
I agree with removing the property for this case. `SetupPageContextGeneration` is async by design, and thus should always generate full page context. Plus, as Nic said, this property needs to be maintained and can be prone to bugs
For cases where a caller wants partial page context, they can just use `GetPartialPageContext`.
Related details
- Nicolas MacBeth
- Yasaman Sedaghat
Yasaman Sedaghat (Gerrit)
Yasaman Sedaghat added 1 comment![]( "Open in Gerrit")
// Whether to fetch full page context.
bool get_full_page_context_ = true;
Yasaman Sedaghatas discussed offline, all the non-test calls to this seem to be true. Should we clean it up from the function and here instead of doing this? I feel like this could be prone to bugs if another caller modifies it in the future before the originally setup extraction. WDYT?
Tarek Makkouk@tarekm...@google.com, @ada...@google.com since you are more familiar with this field, do we still want to keep it around or just remove it and default to full context?
Adam ArcaroIf all the production calls pass to true, I agree with Nick. I think keeping it despite that adds unnecessary complexity and could open us up to bugs in the future. I also think we can always add granular control later if a concrete use case arises.
That being said, I defer to @ada...@google.com as he has more context on this specific task.
I agree with removing the property for this case. `SetupPageContextGeneration` is async by design, and thus should always generate full page context. Plus, as Nic said, this property needs to be maintained and can be prone to bugs
For cases where a caller wants partial page context, they can just use `GetPartialPageContext`.
- Adam Arcaro
- Nicolas MacBeth
- Tarek Makkouk
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Adam Arcaro
| Code-Review | +1 |
2 comments
[page_context_wrapper_ setShouldGetSnapshot:true];nit: These takes in `BOOL`s (Obj-C), not `bool`s (C++), so we should pass `YES`
Related details
- Nicolas MacBeth
- Tarek Makkouk
- Yasaman Sedaghat
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. |
Yasaman Sedaghat (Gerrit)
Yasaman Sedaghat added 1 comment![]( "Open in Gerrit")
[page_context_wrapper_ setShouldGetSnapshot:true];nit: These takes in `BOOL`s (Obj-C), not `bool`s (C++), so we should pass `YES`
Done
Related details
- Nicolas MacBeth
- Tarek Makkouk
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 voted Code-Review+1![]( "Open in Gerrit")
- Nicolas MacBeth
- Tarek Makkouk
- Yasaman Sedaghat
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nicolas MacBeth
| Code-Review | +1 |
2 comments
GeminiBrowserAgent::~GeminiBrowserAgent() {should we destroy the timer in the destructor?
Related details
- Tarek Makkouk
- Yasaman Sedaghat
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. |
Yasaman Sedaghat (Gerrit)
Yasaman Sedaghat added 1 comment![]( "Open in Gerrit")
GeminiBrowserAgent::~GeminiBrowserAgent() {should we destroy the timer in the destructor?
Hm is that needed? once the instance is destructed, wouldn't the timer get destroyed as well?
Related details
- Nicolas MacBeth
- Tarek Makkouk
Adam Arcaro (Gerrit)
Adam Arcaro added 1 comment![]( "Open in Gerrit")
GeminiBrowserAgent::~GeminiBrowserAgent() {Yasaman Sedaghatshould we destroy the timer in the destructor?
Hm is that needed? once the instance is destructed, wouldn't the timer get destroyed as well?
I don't think it's needed, since all private variables are destroyed automatically when this class is
Related details
- Nicolas MacBeth
- Tarek Makkouk
- Yasaman Sedaghat
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yasaman Sedaghat (Gerrit)
Yasaman Sedaghat voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Nicolas MacBeth
- Tarek Makkouk
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Helios Best effort page context generation if page isn't loading
Adding a timeout to force page context generation to update floaty when
a page takes a long time to load. Without the timeout we never get the
PageLoaded callback, don't generate the full page context, and floaty
will show results given the limited page context info (title, url, etc
but not innertext). The timeout is set to 3 seconds, after which we
force generating page context with whatever page info available.
If after timeout, page completes loading, we get the full page context
and update floaty again.
- M ios/chrome/browser/intelligence/bwg/coordinator/bwg_mediator.mm
- M ios/chrome/browser/intelligence/bwg/model/bwg_tab_helper.h
- M ios/chrome/browser/intelligence/bwg/model/bwg_tab_helper.mm
- M ios/chrome/browser/intelligence/bwg/model/bwg_tab_helper_unittest.mm
- M ios/chrome/browser/intelligence/bwg/model/gemini_browser_agent.h
- M ios/chrome/browser/intelligence/bwg/model/gemini_browser_agent.mm
- M ios/chrome/browser/intelligence/bwg/model/gemini_browser_agent_unittest.mm
Code-Review: +1 by Adam Arcaro, +1 by Nicolas MacBeth
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |