Omnibox document suggestions provider service (refactored) [chromium/src : master]

Travis Skare (Gerrit)

unread,

Jun 15, 2018, 8:56:29 PM6/15/18

to Justin Donnelly, asvitki...@chromium.org, jdonnel...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, noyau...@chromium.org

Travis Skare would like Justin Donnelly to review this change.

View Change

Omnibox document suggestions provider service (refactored)

Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ibe9a8fdf1ae1c9eb1ef401d3f667f261a76ef65d
---
M chrome/browser/autocomplete/document_suggestions_service_factory.cc
M components/omnibox/browser/document_provider.cc
M components/omnibox/browser/document_provider.h
M components/omnibox/browser/document_suggestions_service.cc
M components/omnibox/browser/document_suggestions_service.h
M components/omnibox/browser/omnibox_pref_names.cc
M components/omnibox/browser/omnibox_pref_names.h
M third_party/metrics_proto/README.chromium
M tools/metrics/actions/actions.xml
9 files changed, 227 insertions(+), 402 deletions(-)

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jun 15, 2018, 8:56:31 PM6/15/18

to asvitki...@chromium.org, jdonnel...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Justin Donnelly, Commit Bot, chromium...@chromium.org

note - this is displaying for me on top of the original version I had in another issue (this is refactored, significantly simpler, and uses new UIs so should be preferred).

I'll try to figure out if that's just my account (unlikely) or everyone's, and merge these two branches locally.

But uploading for trybots and FYI that % a couple TODOs things have settled. Sorry for delay, was waiting for a dependent change.

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jun 18, 2018, 3:26:09 PM6/18/18

to ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, asvitki...@chromium.org, Justin Donnelly, Commit Bot, chromium...@chromium.org

FYI Rebased onto master, in case some changes were displaying diffs from another feature branch (I'm not sure if that's person-dependent or not). No code changes since last week otherwise.

Note - I have a couple todos to address and two subproject hashes (e.g. breakpad and vcdiff) crept in; I'll remove those.

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jun 18, 2018, 8:46:20 PM6/18/18

to ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, asvitki...@chromium.org, Justin Donnelly, Commit Bot, chromium...@chromium.org

added a new file to fix an ios builder I'd missed - and removed the spurious 1-line subproject files.

Any prior and ongoing reviewing is still valid, thanks!

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Justin Donnelly (Gerrit)

unread,

Jun 19, 2018, 6:12:49 PM6/19/18

to Travis Skare, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, asvitki...@chromium.org, Mark Pearson, Justin Donnelly, Commit Bot, chromium...@chromium.org

This all looks good to me but for some nits.

mpearson, would you mind giving this a look? c/b/autocomplete for OWNERS review, obviously. But the rest could use another pair of eyes since it's a whole new provider.

View Change

10 comments:

File components/omnibox/browser/autocomplete_match_type.cc:
- Patch Set #5, Line 102: 0, // DOCUMENT_SUGGESTION
  Can you either add a TODO here to come up with an appropriate i18n message later, or just make sure it's on your list of tasks?
File components/omnibox/browser/autocomplete_provider.h:
- Patch Set #5, Line 143: TYPE_PHYSICAL_WEB = 1 << 9,
  Was this just overlooked by previous work?
File components/omnibox/browser/document_provider.h:
- Patch Set #5, Line 1: // Copyright (c) 2018 The Chromium Authors. All rights reserved.
  Per the style guide, there should be no (c) here. (Here and in the next 3 files.)
  https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#file-headers
- Patch Set #5, Line 34: extern const base::Feature kDocumentProviderFeature;
  I'd prefer to see this set up in omnibox_field_trial.h/cc instead, since that's where most or all of the other omnibox Features live.
- Patch Set #5, Line 43: static DocumentProvider* Create(AutocompleteProviderClient* client,
  Forward declare AutocompleteProviderClient.
- Patch Set #5, Line 85: ACMatches matches_;
  Is this necessary? AutocompleteProvider already defines a variable with this name and type with protected visibility..
File components/omnibox/browser/document_provider.cc:
- Patch Set #5, Line 77: bool DocumentProviderAllowed(PrefService* prefs,
  The prefs parameter is not being used.
- Patch Set #5, Line 222: bool DocumentProvider::ParseDocumentSearchResults(const base::Value& root_val,
  As a follow on (I don't think it should block landing this CL), can you add a test for this? It's a pretty nicely isolated bit of functionality that lends itself to a good set of unit tests.
File components/omnibox/browser/omnibox_metrics_provider.cc:
- Patch Set #5, Line 68: return OmniboxEventProto::Suggestion::DOCUMENT;
  Have you updated the backend proto yet? I believe that has to land first.
File components/omnibox/browser/search_suggestion_parser.h:
- Patch Set #5, Line 333: // DO_NOT_SUBMIT: same, when talking to the drive endpoint
  document endpoint

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Mark Pearson (Gerrit)

unread,

Jun 19, 2018, 6:40:19 PM6/19/18

to Travis Skare, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, asvitki...@chromium.org, Justin Donnelly, Commit Bot, chromium...@chromium.org

the four files in c/b/autocomplete/... lgtm, with the caveats below. There's your owners approval.

I will not have time to review the rest before Thursday. Use that knowledge as you will.

--mark

Patch set 5:Code-Review +1

View Change

2 comments:

File chrome/browser/autocomplete/document_suggestions_service_factory.cc:
- Patch Set #5, Line 14: // BEFORE_CHECKIN(skare): probably no longer need all of these includes.
  ^^^
- Patch Set #5, Line 46: DependsOn(IdentityManagerFactory::GetInstance());
  Should there be a depends on URL loader factory?

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jun 20, 2018, 9:29:45 PM6/20/18

to ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, asvitki...@chromium.org, Mark Pearson, Justin Donnelly, Commit Bot, chromium...@chromium.org

checkpointing with most comments; need to add in a test.

View Change

9 comments:

File chrome/browser/autocomplete/document_suggestions_service_factory.cc:

- Patch Set #5, Line 14: // BEFORE_CHECKIN(skare): probably no longer need all of these includes.
  ^^^

- finished the audit, removed this, thanks.
File components/omnibox/browser/autocomplete_match_type.cc:

- Patch Set #5, Line 102: 0, // DOCUMENT_SUGGESTION

- Can you either add a TODO here to come up with an appropriate i18n message later, or just make sure […]
  http://crbug.com/854850 (can add TODO but it's currently just a bug to not pollute)
File components/omnibox/browser/autocomplete_provider.h:

- Patch Set #5, Line 143: TYPE_PHYSICAL_WEB = 1 << 9,
  Was this just overlooked by previous work?

- possibly a bad merge I kept around for a while - was removed in https://chromium-review.googlesource.com/988954
  Made TYPE_DOCUMENT 1<<9 to reflect. thanks.
File components/omnibox/browser/document_provider.h:

- Patch Set #5, Line 1: // Copyright (c) 2018 The Chromium Authors. All rights reserved.

- Per the style guide, there should be no (c) here. (Here and in the next 3 files.) […]
  Done

- Patch Set #5, Line 34: extern const base::Feature kDocumentProviderFeature;

- I'd prefer to see this set up in omnibox_field_trial. […]
  Done

- Patch Set #5, Line 43: static DocumentProvider* Create(AutocompleteProviderClient* client,
  Forward declare AutocompleteProviderClient.

- Done

- Patch Set #5, Line 85: ACMatches matches_;

- Is this necessary? AutocompleteProvider already defines a variable with this name and type with prot […]
  Done
File components/omnibox/browser/omnibox_metrics_provider.cc:

- Patch Set #5, Line 68: return OmniboxEventProto::Suggestion::DOCUMENT;
  Have you updated the backend proto yet? I believe that has to land first.

- yep, landed it internally and then it was ported over in a periodic refresh:
  https://codesearch.chromium.org/chromium/src/third_party/metrics_proto/omnibox_event.proto?l=256&rcl=9a7e3d2ed7a9d68a1eec3118807bf5829fd41fe2
File components/omnibox/browser/search_suggestion_parser.h:

- Patch Set #5, Line 333: // DO_NOT_SUBMIT: same, when talking to the drive endpoint
  document endpoint

- apologies, this was spurious; I probably rebased wrong at some point. Removed.

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jun 21, 2018, 12:36:13 PM6/21/18

to ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, asvitki...@chromium.org, Mark Pearson, Justin Donnelly, Commit Bot, chromium...@chromium.org

(no changes with this mail) - Mark, FYI the needs-a-test bit still holds and there's also a chance I missed something in a merge given a couple comments. IIRC one of the bots also needs a change with the newer revisions.

I'll try to resolve both those points today but it's ok if you want to hold off review until the test is added. thanks!

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jun 26, 2018, 9:42:11 PM6/26/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Mark Pearson, Justin Donnelly, Commit Bot, chromium...@chromium.org

added test and metrics defs since this is settling.

@mpearson, have a question for your question.

Patch set 7:Commit-Queue +1

View Change

1 comment:

File chrome/browser/autocomplete/document_suggestions_service_factory.cc:
- Patch Set #5, Line 46:

- Should there be a depends on URL loader factory?

- I don't see anything depending on url loader factory including the contextual search service, and I *think* the url loader factory doesn't inherit from DependencyNode. However that might be wrong. wdyt?

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Justin Donnelly (Gerrit)

unread,

Jun 27, 2018, 1:18:41 PM6/27/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Justin Donnelly, Thiago Farina, Mark Pearson, Commit Bot, chromium...@chromium.org

Unfortunately, two new conflicts are inbound:

https://crrev.com/c/1095098
https://crrev.com/c/1111852

View Change

1 comment:

File components/omnibox/browser/document_provider.cc:
- Patch Set #5, Line 77: // change placement.

- The prefs parameter is not being used.

- Still looks to me like you could remove this parameter in the new version of this method (IsDocumentProviderAllowed).

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Justin Donnelly (Gerrit)

unread,

Jun 27, 2018, 5:53:13 PM6/27/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Justin Donnelly, Thiago Farina, Mark Pearson, Commit Bot, chromium...@chromium.org

Patch set 9:Code-Review +1

View Change

1 comment:

File components/omnibox/browser/document_provider.cc:

- Patch Set #5, Line 77: // change placement.

- Still looks to me like you could remove this parameter in the new version of this method (IsDocument […]
  Per our offline discussion, leaving this is fine.

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jun 29, 2018, 4:43:52 PM6/29/18

to Justin Donnelly, Mark Pearson, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, chromium...@chromium.org, Thiago Farina, Commit Bot

Travis Skare uploaded patch set #12 to this change.

View Change

Omnibox document suggestions provider service

Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ibe9a8fdf1ae1c9eb1ef401d3f667f261a76ef65d
---

M chrome/browser/BUILD.gn
M chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc
M chrome/browser/autocomplete/chrome_autocomplete_provider_client.h
A chrome/browser/autocomplete/document_suggestions_service_factory.cc
A chrome/browser/autocomplete/document_suggestions_service_factory.h
M chrome/browser/ui/app_list/search/omnibox_result.cc
M components/omnibox/browser/BUILD.gn
M components/omnibox/browser/autocomplete_classifier.cc
M components/omnibox/browser/autocomplete_controller.cc
M components/omnibox/browser/autocomplete_controller.h
M components/omnibox/browser/autocomplete_match.cc
M components/omnibox/browser/autocomplete_match_type.cc
M components/omnibox/browser/autocomplete_match_type.h
M components/omnibox/browser/autocomplete_provider.cc
M components/omnibox/browser/autocomplete_provider.h
M components/omnibox/browser/autocomplete_provider_client.h
A components/omnibox/browser/document_provider.cc
A components/omnibox/browser/document_provider.h
A components/omnibox/browser/document_provider_unittest.cc
A components/omnibox/browser/document_suggestions_service.cc
A components/omnibox/browser/document_suggestions_service.h
M components/omnibox/browser/mock_autocomplete_provider_client.cc
M components/omnibox/browser/mock_autocomplete_provider_client.h
M components/omnibox/browser/omnibox_field_trial.cc
M components/omnibox/browser/omnibox_field_trial.h
M components/omnibox/browser/omnibox_metrics_provider.cc
M ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc
M ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.h
M ios/chrome/browser/ui/omnibox/omnibox_util.cc
M tools/metrics/histograms/enums.xml
M tools/metrics/histograms/histograms.xml
M tools/traffic_annotation/summary/annotations.xml
32 files changed, 955 insertions(+), 9 deletions(-)

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jun 29, 2018, 4:48:56 PM6/29/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Justin Donnelly, Thiago Farina, Mark Pearson, Commit Bot, chromium...@chromium.org

(new patch should clear the network attribution and iOS bots, ready for comments)

@Justin, thanks for the heads-up on the blocking changes. They don't look too bad and I can send the author a diff or FYI if this ends landing first. Test added here (in an earlier snapshot)

@Mark, had one question (or alternatively I think it's resolved)

View Change

2 comments:

File components/omnibox/browser/document_provider.cc:

- Patch Set #5, Line 77: // change placement.

- Per our offline discussion, leaving this is fine.
  Ack
- Patch Set #5, Line 222: if (title.empty() || url.empty()) {
  As a follow on (I don't think it should block landing this CL), can you add a test for this? It's a […]
  included. Also had a test for the various enabledness cases stashed; included that in the snapshot too.

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Mark Pearson (Gerrit)

unread,

Jul 3, 2018, 8:00:54 PM7/3/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

I managed to review everything except the core of this changelist (document_provider and document_suggestion_server). Here are my comments.

--mark

View Change

8 comments:

File chrome/browser/autocomplete/document_suggestions_service_factory.cc:

- Patch Set #5, Line 46:

- I don't see anything depending on url loader factory including the contextual search service, and I […]
  I think you're right.
File components/omnibox/browser/autocomplete_controller.cc:
- Patch Set #12, Line 244: if (document_provider_)
  We usually don't guard these calls. Can creation fail here?
File components/omnibox/browser/omnibox_field_trial.h:
- Patch Set #12, Line 45: kDocumentProviderFeature
  nit: no other features here bother saying feature. The API is obvious: base::FeatureList::IsEnabled(). Do you need feature in the parameter passed to that call?
File ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.h:
- Patch Set #12, Line 41:
  nit: unnecessary blank line?
File ios/chrome/browser/ui/omnibox/omnibox_util.cc:
- Patch Set #12, Line 52: // Document suggeestions aren't yet supported on mobile.
  Why not?
  (And why not future-proof yourself icon-wise?)
File tools/metrics/histograms/enums.xml:
- Patch Set #12, Line 34583:
  Please also update the OmniboxProviderAndResultType and OmniboxProvider enums.
File tools/metrics/histograms/histograms.xml:
- Patch Set #12, Line 63813: about
  nit: omit "about"
- Patch Set #12, Line 63822: Number of results returned in each document suggestion reply.
  nit: *successful* reply?
  Or is this recorded on all replies, including those that failed (and hence had 0 results)?

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 12, 2018, 8:58:25 PM7/12/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

View Change

9 comments:

File chrome/browser/autocomplete/document_suggestions_service_factory.cc:
- Patch Set #5, Line 14: // static

- finished the audit, removed this, thanks.

- Done

- Patch Set #5, Line 46:

- I think you're right.
  Done
File components/omnibox/browser/autocomplete_controller.cc:
- Patch Set #12, Line 244: providers_.push_back(document_provider_);
  We usually don't guard these calls. […]
  this one ends up being a pure call to new, so no - I was following the one above for zero_suggest_provider_.
  removed the guard
File components/omnibox/browser/omnibox_field_trial.h:
- Patch Set #12, Line 45: kBreakWordsAtUnderscores
  nit: no other features here bother saying feature. […]
  changed
File ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.h:

- Patch Set #12, Line 41:

  const SearchTermsData& GetSearchTermsData() const override;

nit: unnecessary blank line?

Done

File ios/chrome/browser/ui/omnibox/omnibox_util.cc:

- Patch Set #12, Line 52: // Document suggeestions aren't yet supported on mobile.

- Why not? […]
  Product decisions, like whether this should launch a registered app if possible, or fall back to a webpage (in this case it would also introduce functionality that can't be replicated by other existing search providers).
  Also, traffic for the first-party provider.
  Overall, it will be a while before this is settled. I'm waiting on icons at the moment; would you prefer that we define them here when they arrive, or is HTTP ok given this is certain to not happen on ios/android for the next couple milestones?
File tools/metrics/histograms/enums.xml:
- Patch Set #12, Line 34583: <int value="19" label="Loading failed (retriable, net error)">

- Please also update the OmniboxProviderAndResultType and OmniboxProvider enums.

- Done, thanks for pointing those out. I also added the deprecated physical web provider to the provider enum to not leave a blank space.
File tools/metrics/histograms/histograms.xml:
- Patch Set #12, Line 63813: mich@
  nit: omit "about"
  Done
- Patch Set #12, Line 63822: </histogram>
  nit: *successful* reply? […]
  yes, clarified - successful request and reply must be in the expected format.

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 12, 2018, 8:59:07 PM7/12/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

dding OWNERs for last few files. Alphabetically:

dullweber@
tools/traffic_annotation/summary/annotations.xml
[added a traffic annotation]

eugenebut@:
ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.h
ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc
[providing a null provider - this omnibox provider feature is not in scope for ios]

jennyz@
chrome/browser/ui/app_list/search/omnibox_result.cc
[filling out an enum switch - this omnibox provider will not show results in app list]

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 12, 2018, 9:05:52 PM7/12/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Jenny Zhang, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

Adding OWNERs for last few files. Alphabetically:

dullweber@
tools/traffic_annotation/summary/annotations.xml
[added a traffic annotation]

eugenebut@:
ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.h
ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc
[providing a null provider - this omnibox provider feature is not in scope for ios]

jennyz@
chrome/browser/ui/app_list/search/omnibox_result.cc
[filling out an enum switch - this omnibox provider will not show results in app list]

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Christian Dullweber (Gerrit)

unread,

Jul 13, 2018, 3:59:42 AM7/13/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Jenny Zhang, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

View Change

2 comments:

File components/omnibox/browser/document_suggestions_service.cc:
- Patch Set #14, Line 89: "Google."
  Could you add a short explanation about what a document suggestion is? Does this query go to google drive?
- Patch Set #14, Line 90: trigger: "Signed-in user with Google DS enters text in the omnibox."
  Maybe remove "with Google DS" as it is already mentioned in the description and setting part and makes the sentence hard to read as I had to figure out what a DS is.

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Eugene But (Gerrit)

unread,

Jul 13, 2018, 11:35:56 AM7/13/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Christian Dullweber, Jenny Zhang, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

ios lgtm

Patch set 14:Code-Review +1

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 13, 2018, 7:29:00 PM7/13/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Eugene But, Christian Dullweber, Jenny Zhang, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

View Change

2 comments:

File components/omnibox/browser/document_suggestions_service.cc:

- Patch Set #14, Line 89: "Google."

- Could you add a short explanation about what a document suggestion is? Does this query go to google […]
  Done

- Patch Set #14, Line 90: trigger: "Signed-in user with Google DS enters text in the omnibox."

- Maybe remove "with Google DS" as it is already mentioned in the description and setting part and mak […]
  Done

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Christian Dullweber (Gerrit)

unread,

Jul 16, 2018, 4:15:17 AM7/16/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Eugene But, Jenny Zhang, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

annotations.xml lgtm, thanks

Patch set 15:Code-Review +1

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 16, 2018, 8:33:03 PM7/16/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Christian Dullweber, Eugene But, Jenny Zhang, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

Hi jennyz@, could you review for app_list OWNERS?

chrome/browser/ui/app_list/search/omnibox_result.cc

it's a one-liner making sure all values of an enum are handled in a switch statement to get an icon. This result type won't be fetched for that surface at runtime, however.

thanks!

[uploaded new snapshot with network annotations hash and resolving changes to PrimaryAccountAuthTokenFetcher]

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 17, 2018, 4:32:35 PM7/17/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Christian Dullweber, Eugene But, Jenny Zhang, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

+khmel for OWNERS on
chrome/browser/ui/app_list/search/omnibox_result.cc

(sorry to double up on OWNERS, have a couple diffbased changes waiting on that line)

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Jenny Zhang (Gerrit)

unread,

Jul 18, 2018, 1:10:35 PM7/18/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

Patch set 19:Code-Review +1

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 18, 2018, 1:37:16 PM7/18/18

to Eugene But, Mark Pearson, Justin Donnelly, Yury Khmel, Jenny Zhang, Christian Dullweber, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, chromium...@chromium.org, Thiago Farina, Commit Bot

Travis Skare uploaded patch set #20 to this change.

View Change

Omnibox document suggestions provider service

Bug: 864302



Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ibe9a8fdf1ae1c9eb1ef401d3f667f261a76ef65d
---

32 files changed, 961 insertions(+), 9 deletions(-)

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 18, 2018, 1:37:26 PM7/18/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Jenny Zhang, Yury Khmel, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

Patch set 20:Commit-Queue +2

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Jul 18, 2018, 1:37:37 PM7/18/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Jenny Zhang, Yury Khmel, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, chromium...@chromium.org

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/1069735/20

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1069735/20

Bot data: {"action": "start", "triggered_at": "2018-07-18T17:37:24.0Z", "cq_cfg_revision": "49aa7c3ca384645e343717176af0972552fc0356", "revision": "a4f6d1d12349b80e851639de132b837a0166c791"}

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Jul 18, 2018, 5:40:55 PM7/18/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Jenny Zhang, Yury Khmel, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, chromium...@chromium.org

Try jobs failed on following builders:
  android-kitkat-arm-rel on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-kitkat-arm-rel/38611)

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Yury Khmel (Gerrit)

unread,

Jul 18, 2018, 5:52:59 PM7/18/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Jenny Zhang, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

chrome/browser/ui/app_list/search/omnibox_result.cc lgtm

Patch set 20:Code-Review +1

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 18, 2018, 6:37:45 PM7/18/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

Patch set 21:Commit-Queue +2

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Jul 18, 2018, 6:37:49 PM7/18/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, chromium...@chromium.org

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.

"no-op; rebase to be able to upload downstream changes" https://chromium-review.googlesource.com/c/1069735/21

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1069735/21

Bot data: {"action": "start", "triggered_at": "2018-07-18T22:37:39.0Z", "cq_cfg_revision": "49aa7c3ca384645e343717176af0972552fc0356", "revision": "6e92e3c093a8be8d5cc97123cc3a1ac42203af7e"}

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Jul 18, 2018, 7:24:45 PM7/18/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, chromium...@chromium.org

Try jobs failed on following builders:

  linux-chromeos-rel on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux-chromeos-rel/50630)

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Jul 18, 2018, 7:33:32 PM7/18/18

to asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, Commit Bot, chromium...@chromium.org

Patch set 21:Commit-Queue +2

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Jul 18, 2018, 7:33:42 PM7/18/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, chromium...@chromium.org

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"no-op; rebase to be able to upload downstream changes" https://chromium-review.googlesource.com/c/1069735/21

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1069735/21

Bot data: {"action": "start", "triggered_at": "2018-07-18T23:33:30.0Z", "cq_cfg_revision": "49aa7c3ca384645e343717176af0972552fc0356", "revision": "6e92e3c093a8be8d5cc97123cc3a1ac42203af7e"}

View Change

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Jul 18, 2018, 10:48:10 PM7/18/18

to Travis Skare, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, chromium...@chromium.org

Commit Bot merged this change.

View Change

Approvals:
  Jenny Zhang: Looks good to me
  Mark Pearson: Looks good to me
  Justin Donnelly: Looks good to me
  Eugene But: Looks good to me
  Yury Khmel: Looks good to me
  Christian Dullweber: Looks good to me
  Travis Skare: Commit

Omnibox document suggestions provider service

Bug: 864302

Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ibe9a8fdf1ae1c9eb1ef401d3f667f261a76ef65d

Reviewed-on: https://chromium-review.googlesource.com/1069735
Commit-Queue: Travis Skare <sk...@chromium.org>
Reviewed-by: Yury Khmel <kh...@chromium.org>
Reviewed-by: Jenny Zhang <jen...@chromium.org>
Reviewed-by: Christian Dullweber <dull...@chromium.org>
Reviewed-by: Eugene But <euge...@chromium.org>
Reviewed-by: Justin Donnelly <jdon...@chromium.org>
Reviewed-by: Mark Pearson <mpea...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576339}

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Mark Pearson (Gerrit)

unread,

Aug 28, 2018, 6:44:47 PM8/28/18

to Travis Skare, Commit Bot, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Justin Donnelly, Thiago Farina, chromium...@chromium.org

Well, this is a bit awkward.

While cleanup up open tabs, I found I had comments on this changelist that I never published. (Some I never even saved, sitting as open boxes. That's why you'll see some dates from today in this list.) I hit save on them all and published them. Looking below, some are general cleanup (comments) and some flag things that could probably be done better. I don't think any are particularly critical (blocking). That said, it would be nice to fix some of these things. Please feel free to send me a review whenever.

cheers,
mark

View Change

13 comments:

File components/omnibox/browser/document_provider.h:
- Patch Set #18, Line 67: // Called when loading is complete.
  nit: loading what? A document page?
- Patch Set #18, Line 80: // Parses document search result JSON.
  nit: what's the return type mean?
File components/omnibox/browser/document_provider.cc:
- Patch Set #18, Line 77: // change placement.
  nit: I don't understand the second part of this sentence. (I understand the words, just not why they're here.)
- Patch Set #18, Line 104:
  You probably want a check for INVALID input type here, just to short-circuit as well. That should only happen on empty inputs.
- Patch Set #18, Line 150: // TODO(skare): Verify that we don't lose metrics based on what
  You might want to add provider info about whether the provider has completed running.
  https://cs.chromium.org/chromium/src/components/omnibox/browser/base_search_provider.cc?type=cs&q=AddProviderInfo+file:search&g=0&l=198
  That said, I don't think any of the metrics you're missing (that one, plus the others) are important.
- Patch Set #18, Line 236: ui::PAGE_TRANSITION_GENERATED
  I don't think GENERATED is appropriate here. (I'm leaning toward TYPED, as with other URL suggestions.) Please either comment why GENERATED is appropriate or replace it with something more appropriate.
File components/omnibox/browser/document_suggestions_service.cc:
- Patch Set #18, Line 34: // The format of the request is:
  ... "in JSON:"
- Patch Set #18, Line 44: root.SetKey("start", base::Value(0));
  Would be nice to comment on what start and pageSize mean.
- Patch Set #18, Line 78: Endpoint
  Please use a clearer param name ("DocumentEndpoint"?); omnibox experiments often run in experiment groups that have params from other features in the same place.
- Patch Set #18, Line 118: // TODO(skare): Include code to catch token fetch in-progress; otherwise
  TODO before submitting, or later?
- Patch Set #18, Line 123: OAuth2Scope
  ditto param name comment
- Patch Set #18, Line 160: StartDownloadAndTransferLoader(std::move(request), std::move(request_body),
  Why bother still sending the request if you don't have an oauth token?
File ios/chrome/browser/ui/omnibox/omnibox_util.cc:

- Patch Set #12, Line 52: // Document suggeestions aren't yet supported on mobile.

- Product decisions, like whether this should launch a registered app if possible, or fall back to a w […]
  Thanks for the explanation. This is fine for now.

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Travis Skare (Gerrit)

unread,

Aug 29, 2018, 5:26:42 PM8/29/18

to Commit Bot, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Mark Pearson, Justin Donnelly, Thiago Farina, chromium...@chromium.org

Hi Mark,

Sending 1195927 for cleanup.

Also resolving some of these comments, as some were addressed in follow-on CLs and cleanup.

A couple are still open; the last one in document_provider.cc (around input type GENERATED vs TYPED) has questions. I'm ok either way on that.

View Change

11 comments:

File components/omnibox/browser/document_provider.h:

- Patch Set #18, Line 67: // Called when loading is complete.
  nit: loading what? A document page?

- Done. I stole/borrowed the pattern from the refactorings to SimpleURLLoader, so will change this in ZeroSuggestProvider too.

- Patch Set #18, Line 80: // Parses document search result JSON.
  nit: what's the return type mean?

- already elaborated @HEAD.
File components/omnibox/browser/document_provider.cc:

- Patch Set #18, Line 77: // change placement.

- nit: I don't understand the second part of this sentence. […]
  yeah, this wording isn't great... took me a bit to parse it and remember.
  The intent was "These document results may look like search provider results, so it might look weird if we interleave things that look like Google search results between non-Google default search engine results". Of course that can happen between any two providers with interleaved scores.
  Removed the last bit.

- Patch Set #18, Line 104:

- You probably want a check for INVALID input type here, just to short-circuit as well. […]
  added. in practice we also short-circuit all inputs <4 characters (where N=4 is experiment param), but this prevents needing to depend on that.

- Patch Set #18, Line 150: // TODO(skare): Verify that we don't lose metrics based on what

- You might want to add provider info about whether the provider has completed running. […]
  ACK. Thanks, will see about adding and finishing this TODO. Talked with server folks today actually about metrics, analysis, things that will affect CTR vs. their other surfaces, etc.

- Patch Set #18, Line 236: ui::PAGE_TRANSITION_GENERATED

- I don't think GENERATED is appropriate here. […]
  I originally went with GENERATED since it's "selecting an entry that did not look like a URL."
  Of course it is a URL behind the scenes, and we do show the URL in the omnibox for active document selections, just that the result text itself doesn't look like a URL.
  Happy to switch to TYPED though. WDYT?
File components/omnibox/browser/document_suggestions_service.cc:

- Patch Set #18, Line 34: // The format of the request is:

- ... […]
  Done

- Patch Set #18, Line 44: root.SetKey("start", base::Value(0));
  Would be nice to comment on what start and pageSize mean.

- Done

- Patch Set #18, Line 78: Endpoint

- Please use a clearer param name ("DocumentEndpoint"?); omnibox experiments often run in experiment g […]
  already DocumentProviderEndpoint @HEAD

- Patch Set #18, Line 118: // TODO(skare): Include code to catch token fetch in-progress; otherwise
  TODO before submitting, or later?

- TODO is resolved @HEAD (a bug was fixed that obviated it).

- Patch Set #18, Line 123: OAuth2Scope
  ditto param name comment

- scope code is hardcoded @HEAD

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Mark Pearson (Gerrit)

unread,

Aug 30, 2018, 12:50:11 AM8/30/18

to Travis Skare, Commit Bot, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Justin Donnelly, Thiago Farina, chromium...@chromium.org

View Change

2 comments:

File components/omnibox/browser/document_provider.cc:
- Patch Set #18, Line 236: ui::PAGE_TRANSITION_GENERATED
  I originally went with GENERATED since it's "selecting an entry that did not look like a URL." […]
  True, that description reads in a way that makes it more like GENERATED than TYPED. That said, much of the omnibox code and other systems I've seen implicitly assume GENERATED is the same thing as search query and TYPED is a URL. We've have to do a thorough audit of at least the omnibox code. I'm think, for example, that using GENERATED will prevent these from getting a typed_count increment in the history database, which will prevent history from learning to suggest them more aggressively. It might prevent shortcuts database from learning correctly, or may learn wrong things? And there may be others.
  It's code like this for example that has me worried.
  https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_model.cc?q=file:omnibox+TYPED+-file:test+case:yes&sq=package:chromium&dr=C&l=713-720
File components/omnibox/browser/document_suggestions_service.cc:
- Patch Set #18, Line 160: StartDownloadAndTransferLoader(std::move(request), std::move(request_body),

- Why bother still sending the request if you don't have an oauth token?

- Not replied to.

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Mark Pearson (Gerrit)

unread,

Sep 11, 2018, 5:33:07 PM9/11/18

to Travis Skare, Commit Bot, asvitki...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, jdonnel...@chromium.org, marq+...@chromium.org, noyau...@chromium.org, Yury Khmel, Jenny Zhang, Christian Dullweber, Eugene But, Justin Donnelly, Thiago Farina, chromium...@chromium.org

reposting, with additional context, in case you missed the last message

View Change

1 comment:

File components/omnibox/browser/document_provider.cc:

- Patch Set #18, Line 236: ui::PAGE_TRANSITION_GENERATED

- True, that description reads in a way that makes it more like GENERATED than TYPED. […]
  This should probably be resolved before a wider launch. I'll note, for example, that TYPED URLs get scored from the history providers more aggressively. By using GENERATED, the bootstrapping to get history providers to suggest these URLs will be much less effective.

To view, visit change 1069735. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward