Drive results: Update sourceOptions->dataSourceRestrictions param from API migration |
|||||||||
Issue descriptionIn 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.
,
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
,
Aug 13
+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?
,
Aug 13
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)
,
Aug 13
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
,
Aug 13
Missed this was P3. Corpus restrict is wanted if possible.
,
Aug 13
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
,
Aug 13
How critical and safe is this merge to M69?
,
Aug 13
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.
,
Aug 13
Off by default, can be turned on only via Finch. All code in this path is behind that Finch flag.
,
Aug 13
Approving merge to M69 branch 3497 based on comment #9 and #10. Please merge. Thank you.
,
Aug 14
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
,
Aug 15
Fixed?
,
Aug 18
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by skare@chromium.org
, Aug 10