Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738583002:
Add a way to cache and show zero suggest results before native (Closed)
DescriptionAdd a way to cache and show zero suggest results before native
* Adds utilities to OmniboxSuggestion for caching and retrieving a list of
suggestion with enough info to be visually and functionally the same.
* Uses the utilities to add a new mode in AutocompleteController where
it caches each zero suggest update and sends them to its listener when
set.
* Uses this new mode in LocationBarLayout to show zero suggest results
before native initialization. If the user selects these results without
native, the selection action is delayed until the suggestion list is
updated. This gives the native AutocompleteController to create the
necessary components that will handle the logs correctly. Also adds logc
to check whether the suggestion to be logged matched with the suggestion
selected.
Review-Url: https://codereview.chromium.org/2738583002
Cr-Commit-Position: refs/heads/master@{#456096}
Committed: https://chromium.googlesource.com/chromium/src/+/75f655ce3baead660fc6fc9ef5b2f05f16458ea6
Patch Set 1 #
Total comments: 10
Patch Set 2 : comments #Patch Set 3 : reset logic change #
Total comments: 2
Patch Set 4 : reset on start call as well #
Messages
Total messages: 26 (17 generated)
yusufo@chromium.org changed reviewers: + mariakhomenko@chromium.org
I am still trying to grasp if the timing we care about is staying correct and I think it is. Here is what the rest of it looks like while I try to confirm that for sure.
https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:82: public void setUseCachedZeroSuggestResults() { nit: I don't know why but I have a lot of trouble parsing the name of this function -- something about the combination of words makes it hard to grasp what it's doing. How about startCachedZeroSuggest? That goes together with the rest of method naming. https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:86: if (suggestions != null) mListener.onSuggestionsReceived(suggestions, ""); I wonder if we can hit some subtle assumptions here by being synchronous whereas we are usually async. But if it works, I guess then it's fine. https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:229: if (mWaitingForSuggestionsToCache) { This is a little fragile because it relies on zero suggest returning everything in the first suggestions response (in general, this method can get called many times with updated suggestions as they come in over the wire). I think today we get zero suggest all at once, but with some new providers (that perhaps return their suggestions async later) in this area you might end up with an incomplete list here. https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:331: if (mNativeAutocompleteControllerAndroid == 0) return null; I thought this couldn't get called until native is loaded?
https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:82: public void setUseCachedZeroSuggestResults() { On 2017/03/08 06:48:08, Maria wrote: > nit: I don't know why but I have a lot of trouble parsing the name of this > function -- something about the combination of words makes it hard to grasp what > it's doing. > > How about startCachedZeroSuggest? That goes together with the rest of method > naming. Done. https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:86: if (suggestions != null) mListener.onSuggestionsReceived(suggestions, ""); On 2017/03/08 06:48:08, Maria wrote: > I wonder if we can hit some subtle assumptions here by being synchronous whereas > we are usually async. But if it works, I guess then it's fine. Acknowledged. https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:229: if (mWaitingForSuggestionsToCache) { On 2017/03/08 06:48:08, Maria wrote: > This is a little fragile because it relies on zero suggest returning everything > in the first suggestions response (in general, this method can get called many > times with updated suggestions as they come in over the wire). I think today we > get zero suggest all at once, but with some new providers (that perhaps return > their suggestions async later) in this area you might end up with an incomplete > list here. On the omnibox side, we never combine these calls though, right? So should I maybe have a lower limit for # of suggestions? > 1, > 2? https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:331: if (mNativeAutocompleteControllerAndroid == 0) return null; On 2017/03/08 06:48:08, Maria wrote: > I thought this couldn't get called until native is loaded? Done. Yeah this seems to be a leftover from initial code.
https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:229: if (mWaitingForSuggestionsToCache) { On 2017/03/08 20:10:46, Yusuf wrote: > On 2017/03/08 06:48:08, Maria wrote: > > This is a little fragile because it relies on zero suggest returning > everything > > in the first suggestions response (in general, this method can get called many > > times with updated suggestions as they come in over the wire). I think today > we > > get zero suggest all at once, but with some new providers (that perhaps return > > their suggestions async later) in this area you might end up with an > incomplete > > list here. > > On the omnibox side, we never combine these calls though, right? So should I > maybe have a lower limit for # of suggestions? > 1, > 2? Correct. Each onSuggestionReceived set is newly ranked set including top suggestions we have up to date. To give an example, let's say we have zero suggest providers A, B, and C. A returns suggestions A1 with score 10, A2 with score 9, and A3 with score 2. B returns suggestions B1 with score 5 and B2 with score 4 C returns suggestions C1 with score 11 and C2 with score 8 Now B and C are async providers that will message their suggestions as they get them and C is slower than B. We may see something like the following: onSuggestionsReceived call 1: A1 A2 A3 onSuggestionReceived call 2: A1 A2 B1 B2 A3 onSuggestionReceived call 3: C1 A1 A2 C2 B1 Ideally, we would show the user the results of call 3 immediately. So I think it may make sense to reset mWaitingForSuggestionsToCache in stop() call instead of after caching the first set of suggestions and allow cacheOmniboxSuggestionListForZeroSuggest to keep getting updated.
And with regards to the timing, looks like the number we care about in the log is elapsedTimeSinceModified which does get resetted on the LocationBarLayout side and is essentially relying on java only stuff. So we should be good to go as long as we don't have any other concerns about the CL. https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2738583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:229: if (mWaitingForSuggestionsToCache) { On 2017/03/09 17:55:09, Maria wrote: > On 2017/03/08 20:10:46, Yusuf wrote: > > On 2017/03/08 06:48:08, Maria wrote: > > > This is a little fragile because it relies on zero suggest returning > > everything > > > in the first suggestions response (in general, this method can get called > many > > > times with updated suggestions as they come in over the wire). I think today > > we > > > get zero suggest all at once, but with some new providers (that perhaps > return > > > their suggestions async later) in this area you might end up with an > > incomplete > > > list here. > > > > On the omnibox side, we never combine these calls though, right? So should I > > maybe have a lower limit for # of suggestions? > 1, > 2? > > Correct. Each onSuggestionReceived set is newly ranked set including top > suggestions we have up to date. > > To give an example, let's say we have zero suggest providers A, B, and C. > > A returns suggestions A1 with score 10, A2 with score 9, and A3 with score 2. > B returns suggestions B1 with score 5 and B2 with score 4 > C returns suggestions C1 with score 11 and C2 with score 8 > > Now B and C are async providers that will message their suggestions as they get > them and C is slower than B. We may see something like the following: > > onSuggestionsReceived call 1: > A1 > A2 > A3 > > onSuggestionReceived call 2: > A1 > A2 > B1 > B2 > A3 > > onSuggestionReceived call 3: > C1 > A1 > A2 > C2 > B1 > > > Ideally, we would show the user the results of call 3 immediately. So I think it > may make sense to reset mWaitingForSuggestionsToCache in stop() call instead of > after caching the first set of suggestions and allow > cacheOmniboxSuggestionListForZeroSuggest to keep getting updated. Done.
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2738583002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2738583002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:177: mWaitingForSuggestionsToCache = false; let's also reset these in start() call to ensure we don't cache wrong results. I think we usually call stop() first, but not sure that's guaranteed.
https://codereview.chromium.org/2738583002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2738583002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:177: mWaitingForSuggestionsToCache = false; On 2017/03/09 22:05:22, Maria wrote: > let's also reset these in start() call to ensure we don't cache wrong results. I > think we usually call stop() first, but not sure that's guaranteed. Done.
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yusufo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2738583002/#ps60001 (title: "reset on start call as well")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489167674535630,
"parent_rev": "6ce2feb35c970fe7d9557c851c016c6d6c8b1d39", "commit_rev":
"75f655ce3baead660fc6fc9ef5b2f05f16458ea6"}
Message was sent while issue was closed.
Description was changed from ========== Add a way to cache and show zero suggest results before native * Adds utilities to OmniboxSuggestion for caching and retrieving a list of suggestion with enough info to be visually and functionally the same. * Uses the utilities to add a new mode in AutocompleteController where it caches each zero suggest update and sends them to its listener when set. * Uses this new mode in LocationBarLayout to show zero suggest results before native initialization. If the user selects these results without native, the selection action is delayed until the suggestion list is updated. This gives the native AutocompleteController to create the necessary components that will handle the logs correctly. Also adds logc to check whether the suggestion to be logged matched with the suggestion selected. ========== to ========== Add a way to cache and show zero suggest results before native * Adds utilities to OmniboxSuggestion for caching and retrieving a list of suggestion with enough info to be visually and functionally the same. * Uses the utilities to add a new mode in AutocompleteController where it caches each zero suggest update and sends them to its listener when set. * Uses this new mode in LocationBarLayout to show zero suggest results before native initialization. If the user selects these results without native, the selection action is delayed until the suggestion list is updated. This gives the native AutocompleteController to create the necessary components that will handle the logs correctly. Also adds logc to check whether the suggestion to be logged matched with the suggestion selected. Review-Url: https://codereview.chromium.org/2738583002 Cr-Commit-Position: refs/heads/master@{#456096} Committed: https://chromium.googlesource.com/chromium/src/+/75f655ce3baead660fc6fc9ef5b2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/75f655ce3baead660fc6fc9ef5b2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
