[Fusebox] Consolidate cached and live version of AddTabContext in native [chromium/src : main]
Shakti Sahu (Gerrit)
Shakti Sahu added 4 comments![]( "Open in Gerrit")
return Java_ComposeboxQueryControllerBridge_shouldAttemptCacheForTab(Calder KitagawaI don't know if there's really a need to do a JNI call here, all information should be available native I thought.
Shakti Sahu+1
Moved the logic to native and removed JNI call.
lens::TabContextualizationController* controller =
tab ? lens::TabContextualizationController::From(tab) : nullptr;Hailey WangI was envisioning rather than using this we'd ask the PageContentExtractionService to fetch via the same mechanism the cache uses. That way all the codepaths are the same?
Shakti Sahu+1, I think what Calder mentioned make more sense, it would allow live extraction to also be cached.
Calder KitagawaAck. But that probably needs more work to integrate the caching / live data well in the PCES? TabContextualizationController does a few things outside of PCES. I dont knwo the implications yet. Meanwhile, I wanted to unblock fusebox / composebox refactor work for mobile. Thoughts?
Hailey WangDoing this short term is OK, but I do think it is worth filing a bug and making sure we follow-up on having a single service for extracting contexts before too much gets built upon split paths.
Shivani SharmaMaybe try reaching out to @shiva...@chromium.org for the modifications of page content extraction service see if they have extra cycles for this change...
Shakti SahuAck, can you please create an issue for the follow up and assign to @ale...@chromium.org. Thanks!
Filed b/488055569
controller->GetPageContext(Shakti SahuDoes this call save to the cache somewhere? I thought the point of unifying live fetching was so the live fetch also ended up in the cache?
I think this did not. We need follow up work to integrate this with PageContentExtractionService. Currently PCES only extracts the content and caches them when a tab goes to background. This is good, but we need to integrate any ondemand stuff to be cached as well.
// TODO(shaktisahu): Add retry logic. Look for the fie_token and retry. Need
// to understand when we retry (This is replacement for retryUpload from
// Java). Might be able to get the file info from session handle or might need
// to store some additional info in this class.Shakti SahuThis might complicate things. Need to resolve beofre I land this.
Done
Related details
- Calder Kitagawa
- Hailey Wang
- Patrick Noland
- Shivani Sharma
- Sky Malice
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. |
Hailey Wang (Gerrit)
Hailey Wang added 1 comment![]( "Open in Gerrit")
Discussed offline: I think it's still better if we can have Alex merge the live fetching logic into the page content extraction service if possible in the next week.
If this is blocking desktop tab picker work, ok to land current change as it just moves Java retry logic into native to unblock desktop using cache. Maybe double check with @pno...@chromium.org what he thinks~
Related details
- Calder Kitagawa
- Patrick Noland
- Shakti Sahu
- Shivani Sharma
- Sky Malice
Calder Kitagawa (Gerrit)
Calder Kitagawa added 2 comments![]( "Open in Gerrit")
At a high level this LGTM, but not going to CR+1 it until Sky and/or Patrick also take a look.
controller->GetPageContext(Shakti SahuDoes this call save to the cache somewhere? I thought the point of unifying live fetching was so the live fetch also ended up in the cache?
I think this did not. We need follow up work to integrate this with PageContentExtractionService. Currently PCES only extracts the content and caches them when a tab goes to background. This is good, but we need to integrate any ondemand stuff to be cached as well.
SGTM we should include this in the scope of b/488055569 (I added it).
Related details
Patrick Noland (Gerrit)
Patrick Noland added 3 comments![]( "Open in Gerrit")
void TryCacheThenLive(JNIEnv* env,```suggestion
void TryAddTabContextFromCacheThenLive
```
And please add a comment explaining the callback execution flow
tabs::TabInterface* tab = tab_handle.Get();Is this guaranteed to be non-null?
// We don't retry here, as the retry has been already handled in the native code at
// this point.I would just remove this entirely; the existence of retry logic is what justified a comment; you don't need to explain the lack of it
Related details
Shakti Sahu (Gerrit)
Shakti Sahu abandoned this change.
Shakti Sahu abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |