New issue
Advanced search Search tips

Issue 873290 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 18
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Drive results: Update sourceOptions->dataSourceRestrictions param from API migration

Project Member Reported by skare@chromium.org, Aug 10

Issue description

In the move from v1beta -> v1 API, the request param "sourceOptions" became "dataSourceRestrictions". Need to update this token so other corpuses don't get added in the future.

Should also sanitize the title per and provide default match data per comments in the ACMatch header.
 
Components: UI>Browser>Omnibox
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4cf7e0b53900f489bcfb7bd7db7b356cfb31d2e5

commit 4cf7e0b53900f489bcfb7bd7db7b356cfb31d2e5
Author: Travis Skare <skare@chromium.org>
Date: Fri Aug 10 22:48:29 2018

[omnibox/drive] Updates for /v1beta -> /v1 api migration.

-Use originalURL if provided to dedupe against history.
-Fix "typo" for corpus restrict request param (I missed it was renamed).
-Sanitize title, provide default match data.

Bug:  873290 
Change-Id: Idc2d0dd92a6afedc7086c283fe85bb4257a800d3
Reviewed-on: https://chromium-review.googlesource.com/1171550
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Travis Skare <skare@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582372}
[modify] https://crrev.com/4cf7e0b53900f489bcfb7bd7db7b356cfb31d2e5/components/omnibox/browser/document_provider.cc
[modify] https://crrev.com/4cf7e0b53900f489bcfb7bd7db7b356cfb31d2e5/components/omnibox/browser/document_suggestions_service.cc

Cc: jdonnelly@chromium.org
+cc jdonnelly@ - I'm requesting a merge for the one line of parameter renaming and canonical URL support which is now sent to Chrome by the server. It's (+8/-3) change in total but I recognize it's third beta. wdyt?
Labels: Merge-Request-69 OS-Chrome OS-Linux OS-Mac OS-Windows
merge rationale: API parameter renamed on the server. 

This also allows deduping URLs against history URLs which is a good improvement in UX if it's not too late to merge that piece as well (total 8 lines, reading one new param)
Project Member

Comment 5 by sheriffbot@chromium.org, Aug 13

Labels: -Merge-Request-69 Hotlist-Merge-Reject Merge-Reject-69
The bug is marked as P3 or Feature. It should not be merged as M69 is in beta. 
Please contact the approriate milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Pri-3 -Hotlist-Merge-Reject -Merge-Reject-69 Merge-Request-69 Pri-1
Missed this was P3. Corpus restrict is wanted if possible.
Project Member

Comment 7 by sheriffbot@chromium.org, Aug 13

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
How critical and safe is this merge to M69? 
Safety? - whole feature is behind a finch flag, off by default. Kept delta small (8 lines), has been tested with and without the optional param. Has been tested against live API with the renamed parameter.

Feature will be experimental in this milestone. Stable is mostly for traffic and usage analysis. Finch study will be off by default.

Needed? - Primary function of the change is a parameter rename in an API endpoint that just moved from beta to release; this is needed to restrict to just the subset of docs we want. 


Off by default, can be turned on only via Finch.
All code in this path is behind that Finch flag.
Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #9 and #10. Please merge. Thank you.
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 14

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f43215338b6ee8192a8481e5fe6a5cbf2c01504

commit 9f43215338b6ee8192a8481e5fe6a5cbf2c01504
Author: Travis Skare <skare@chromium.org>
Date: Tue Aug 14 20:43:54 2018

[omnibox/drive] Updates for /v1beta -> /v1 api migration.

-Use originalURL if provided to dedupe against history.
-Fix "typo" for corpus restrict request param (I missed it was renamed).
-Sanitize title, provide default match data.

Bug:  873290 
Change-Id: Idc2d0dd92a6afedc7086c283fe85bb4257a800d3
Reviewed-on: https://chromium-review.googlesource.com/1171550
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Travis Skare <skare@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#582372}(cherry picked from commit 4cf7e0b53900f489bcfb7bd7db7b356cfb31d2e5)
Reviewed-on: https://chromium-review.googlesource.com/1173081
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#631}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/9f43215338b6ee8192a8481e5fe6a5cbf2c01504/components/omnibox/browser/document_provider.cc
[modify] https://crrev.com/9f43215338b6ee8192a8481e5fe6a5cbf2c01504/components/omnibox/browser/document_suggestions_service.cc

Fixed?
Status: Fixed (was: Started)

Sign in to add a comment