Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(20)

Issue 2857333002: [TTS] Write initial Tap-features to Ranker. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 3 weeks ago by Donn Denman
Modified:
1 month, 1 week ago
CC:
chromium-reviews, twellington+watch_chromium.org, Roger McFarlane (Chromium), mdjones+watch_chromium.org, donnd+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, hamelphi, msramek (slow)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[TTS] Write initial Tap-features to Ranker. This is an initial write-to-Ranker change that just writes features that we already have on-hand to validate the feature-write mechanism. This introduces a new native class that does the actual feature writing to Ranker. This class is connected to an existing Java feature collection stub (implemented a few months ago). All new code is behind a new flag, so nothing should change by default. Still TODO: Tests. BUG=676108 Review-Url: https://codereview.chromium.org/2857333002 Cr-Commit-Position: refs/heads/master@{#473234} Committed: https://chromium.googlesource.com/chromium/src/+/1c0f254c93df392634578b2520fad79b6cf1003f

Patch Set 1 #

Patch Set 2 : Updated code that writes to UKM to include a URL and reset the builder between records. #

Patch Set 3 : Reverted changes to translate_ranker_impl.cc and changed DVLOG(0) to DVLOG(3). #

Total comments: 12

Patch Set 4 : Removed the Flush() call, and some other nits from Robert's review. Rebased, which restricts what … #

Patch Set 5 : Added logging of the CS-model URL and updated console-logging output. #

Total comments: 15

Patch Set 6 : Updated based on reviews by Roger and Theresa. Some API and doc changes. Handle null base page UR… #

Total comments: 1

Patch Set 7 : Fix a FindBugs issue in ContextualSearchRankerLoggerImpl.java and rebase. #

Patch Set 8 : Remove the metric that specifies what model to use -- we'll use Chrome version to determine that. #

Patch Set 9 : Reworked ContextualSearchRankerLoggerImpl.java somewhat: assert that calling sequence is valid even… #

Total comments: 2

Patch Set 10 : Removed DVLOGs and rebased. #

Total comments: 2

Patch Set 11 : Added an entry for CS and metrics written to ukm.xml. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+388 lines, -14 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java View 1 2 3 4 5 6 7 8 9 4 chunks +14 lines, -5 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFieldTrial.java View 3 chunks +15 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLogger.java View 1 2 3 4 5 2 chunks +15 lines, -3 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java View 1 2 3 4 5 6 7 8 1 chunk +115 lines, -5 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/android/chrome_jni_registrar.cc View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
A chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.h View 1 2 3 4 5 6 7 1 chunk +68 lines, -0 lines 0 comments Download
A chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +77 lines, -0 lines 0 comments Download
M components/ukm/ukm_service.h View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M tools/metrics/ukm/ukm.xml View 1 2 3 4 5 6 7 8 9 10 1 chunk +75 lines, -0 lines 1 comment Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 69 (42 generated)
Donn Denman
Theresa, Here's a first cut at a first implementation of writing code to Ranker via ...
1 month, 3 weeks ago (2017-05-03 23:31:04 UTC) #4
Donn Denman
Robert, PTAL at contextual_search_ranker_logger_impl to see if the way we're logging to UKM seems reasonable. ...
1 month, 2 weeks ago (2017-05-10 02:58:42 UTC) #12
Donn Denman
On 2017/05/10 02:58:42, Donn Denman wrote: > Robert, PTAL at contextual_search_ranker_logger_impl to see if the ...
1 month, 2 weeks ago (2017-05-11 15:20:32 UTC) #17
rkaplow
https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc#newcode16 chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:16: ContextualSearchRankerLoggerImpl::ContextualSearchRankerLoggerImpl( to verify - when void destroy() is called ...
1 month, 2 weeks ago (2017-05-11 19:46:49 UTC) #18
Donn Denman
Robert, thanks for the review!!!! https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc#newcode16 chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:16: ContextualSearchRankerLoggerImpl::ContextualSearchRankerLoggerImpl( On 2017/05/11 19:46:49, ...
1 month, 2 weeks ago (2017-05-11 23:34:40 UTC) #20
Donn Denman
Theresa, PTAL -- this should be ready for a full review now.
1 month, 2 weeks ago (2017-05-11 23:36:07 UTC) #22
Theresa
https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java#newcode147 chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:147: mTapSuppressionRankerLogger.writeLogAndReset(); Why was just this line pulled out of ...
1 month, 2 weeks ago (2017-05-12 01:02:25 UTC) #23
Roger McFarlane (Chromium)
https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java#newcode68 chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java:68: nativeLogString(mNativePointer, feature.toString(), value.toString()); Do we have go ahead to ...
1 month, 2 weeks ago (2017-05-12 15:43:27 UTC) #25
Donn Denman
Roger and Theresa, PTAL, and THANKS for the reviews! https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java#newcode147 chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:147: ...
1 month, 2 weeks ago (2017-05-12 23:08:55 UTC) #26
Donn Denman
cc: Martin Šrámek for Chrome Privacy.
1 month, 2 weeks ago (2017-05-15 18:17:35 UTC) #34
Donn Denman
Reviewers, I plan to make some minor changes to this CL so don't bother reviewing ...
1 month, 2 weeks ago (2017-05-15 20:41:27 UTC) #37
Donn Denman
Martin, Please review for Chrome Privacy. Everything that is logged is listed in ContextualSearchRankerLogger.java and ...
1 month, 2 weeks ago (2017-05-15 23:05:26 UTC) #38
msramek (slow)
Thanks. I'll leave the detailed review to the UKM owners. From privacy side, I looked ...
1 month, 1 week ago (2017-05-16 14:59:34 UTC) #39
Donn Denman
All, I think this is ready for a last round of reviews. PTAL in the ...
1 month, 1 week ago (2017-05-16 20:53:14 UTC) #46
Donn Denman
Ted, PTAL at chrome_jni_registrar.cc. All Reviewers, I plan to try to land this tomorrow morning ...
1 month, 1 week ago (2017-05-17 22:24:26 UTC) #50
Ted C
On 2017/05/17 22:24:26, Donn Denman wrote: > Ted, PTAL at chrome_jni_registrar.cc. > > All Reviewers, ...
1 month, 1 week ago (2017-05-17 23:31:45 UTC) #51
Theresa
lgtm https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java#newcode147 chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:147: mTapSuppressionRankerLogger.writeLogAndReset(); On 2017/05/12 23:08:55, Donn Denman wrote: > ...
1 month, 1 week ago (2017-05-18 15:35:11 UTC) #52
Donn Denman
Thanks Theresa! https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java#newcode147 chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:147: mTapSuppressionRankerLogger.writeLogAndReset(); On 2017/05/18 15:35:11, Theresa wrote: > ...
1 month, 1 week ago (2017-05-18 18:06:07 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857333002/180001
1 month, 1 week ago (2017-05-18 18:07:14 UTC) #56
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/441382)
1 month, 1 week ago (2017-05-18 18:19:30 UTC) #58
Donn Denman
Robert, PTAL. Approval needed for ukm_service.h. Thanks!
1 month, 1 week ago (2017-05-18 18:23:46 UTC) #59
rkaplow
https://codereview.chromium.org/2857333002/diff/180001/chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/180001/chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc#newcode43 chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:43: builder_ = ukm_service_->GetEntryBuilder(source_id_, "ContextualSearch"); can you register the UKM ...
1 month, 1 week ago (2017-05-18 18:33:13 UTC) #60
Donn Denman
Robert, PTAL. https://codereview.chromium.org/2857333002/diff/180001/chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/180001/chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc#newcode43 chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:43: builder_ = ukm_service_->GetEntryBuilder(source_id_, "ContextualSearch"); On 2017/05/18 18:33:13, ...
1 month, 1 week ago (2017-05-18 21:46:36 UTC) #61
Donn Denman
rkaplow@, Hopefully you're OK with landing this first cut and doing some iteration. If you'd ...
1 month, 1 week ago (2017-05-19 15:06:01 UTC) #62
rkaplow
lgtm
1 month, 1 week ago (2017-05-19 15:23:47 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857333002/200001
1 month, 1 week ago (2017-05-19 15:45:09 UTC) #66
commit-bot: I haz the power
1 month, 1 week ago (2017-05-19 17:38:15 UTC) #69
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/1c0f254c93df392634578b2520fa...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 23e94e589