Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(242)

Issue 2565873002: [translate] Add translate ranker model loader. (Closed)

Created:
4 years ago by Roger McFarlane (Chromium)
Modified:
3 years, 9 months ago
CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org, pasko
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[translate] Add translate ranker model loader. This CL extract the functionality to downlaod and cache a model file to a generic ModelLoader and then specializes that model loader for use with the TranslateRankerModel. BUG=646711 Review-Url: https://codereview.chromium.org/2565873002 Cr-Commit-Position: refs/heads/master@{#453378} Committed: https://chromium.googlesource.com/chromium/src/+/2233a4a7cb12ac4acdebd33491b528f8fb670f02

Patch Set 1 : Initial CL #

Total comments: 40

Patch Set 2 : large refactor (still need to fix iOS) #

Patch Set 3 : fix the builders? #

Patch Set 4 : fix builders? #

Total comments: 14

Patch Set 5 : comments from hamelphi #

Total comments: 26

Patch Set 6 : add missing unittest fix ios web view #

Patch Set 7 : for asan testing only - DO NOT COMMIT #

Total comments: 78

Patch Set 8 : Address comments from groby, fdoray, hamelphi #

Total comments: 6

Patch Set 9 : rebase to pick up https://codereview.chromium.org/2713513003/ #

Total comments: 18

Patch Set 10 : comments from groby and fdoray #

Total comments: 4

Patch Set 11 : comments from asanka and hamelphi #

Total comments: 6

Patch Set 12 : rebase and fix web_view prefix #

Patch Set 13 : comments from sdefresne #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2422 lines, -762 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/metrics/chrome_metrics_service_client.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +1 line, -1 line 0 comments Download
M chrome/browser/translate/chrome_translate_client.cc View 1 2 2 chunks +8 lines, -5 lines 0 comments Download
A chrome/browser/translate/translate_ranker_factory.h View 1 2 3 4 1 chunk +38 lines, -0 lines 0 comments Download
A chrome/browser/translate/translate_ranker_factory.cc View 1 2 3 4 1 chunk +48 lines, -0 lines 0 comments Download
A + chrome/browser/translate/translate_ranker_metrics_provider.h View 1 2 3 chunks +5 lines, -5 lines 0 comments Download
A chrome/browser/translate/translate_ranker_metrics_provider.cc View 1 2 3 4 5 1 chunk +32 lines, -0 lines 0 comments Download
M components/translate/core/browser/BUILD.gn View 1 2 3 4 5 6 7 3 chunks +10 lines, -4 lines 0 comments Download
A components/translate/core/browser/mock_translate_ranker.h View 1 2 3 4 5 1 chunk +65 lines, -0 lines 0 comments Download
A components/translate/core/browser/mock_translate_ranker.cc View 1 2 3 4 5 6 7 1 chunk +51 lines, -0 lines 0 comments Download
M components/translate/core/browser/proto/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
A components/translate/core/browser/proto/ranker_model.proto View 1 2 3 4 5 6 7 1 chunk +49 lines, -0 lines 0 comments Download
M components/translate/core/browser/proto/translate_ranker_model.proto View 1 2 3 4 5 6 7 2 chunks +5 lines, -2 lines 0 comments Download
A components/translate/core/browser/ranker_model.h View 1 2 3 4 5 6 7 1 chunk +46 lines, -0 lines 0 comments Download
A components/translate/core/browser/ranker_model.cc View 1 2 3 4 5 6 7 1 chunk +54 lines, -0 lines 0 comments Download
A components/translate/core/browser/ranker_model_loader.h View 1 2 3 4 5 6 7 8 9 1 chunk +139 lines, -0 lines 0 comments Download
A components/translate/core/browser/ranker_model_loader.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +420 lines, -0 lines 0 comments Download
A components/translate/core/browser/ranker_model_loader_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +374 lines, -0 lines 0 comments Download
M components/translate/core/browser/translate_manager.h View 1 3 chunks +3 lines, -0 lines 0 comments Download
M components/translate/core/browser/translate_manager.cc View 1 2 3 4 5 6 7 4 chunks +26 lines, -21 lines 0 comments Download
M components/translate/core/browser/translate_manager_unittest.cc View 1 2 3 4 5 6 chunks +6 lines, -4 lines 0 comments Download
M components/translate/core/browser/translate_ranker.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +19 lines, -83 lines 0 comments Download
D components/translate/core/browser/translate_ranker.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -311 lines 0 comments Download
A components/translate/core/browser/translate_ranker_impl.h View 1 2 3 4 5 6 7 8 9 1 chunk +132 lines, -0 lines 0 comments Download
A components/translate/core/browser/translate_ranker_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +282 lines, -0 lines 0 comments Download
A components/translate/core/browser/translate_ranker_impl_unittest.cc View 1 2 3 4 5 6 7 9 10 11 1 chunk +324 lines, -0 lines 0 comments Download
D components/translate/core/browser/translate_ranker_metrics_provider.h View 1 1 chunk +0 lines, -28 lines 0 comments Download
D components/translate/core/browser/translate_ranker_metrics_provider.cc View 1 1 chunk +0 lines, -37 lines 0 comments Download
D components/translate/core/browser/translate_ranker_unittest.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -236 lines 0 comments Download
M components/translate/core/browser/translate_ui_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 4 chunks +7 lines, -3 lines 0 comments Download
M components/translate/core/browser/translate_url_fetcher.cc View 1 chunk +1 line, -1 line 0 comments Download
M ios/chrome/browser/metrics/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +1 line, -1 line 0 comments Download
M ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +1 line, -1 line 0 comments Download
M ios/chrome/browser/translate/BUILD.gn View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
M ios/chrome/browser/translate/chrome_ios_translate_client.mm View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +8 lines, -2 lines 0 comments Download
A ios/chrome/browser/translate/translate_ranker_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +51 lines, -0 lines 0 comments Download
A ios/chrome/browser/translate/translate_ranker_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +50 lines, -0 lines 0 comments Download
A + ios/chrome/browser/translate/translate_ranker_metrics_provider.h View 1 2 3 chunks +5 lines, -5 lines 0 comments Download
A ios/chrome/browser/translate/translate_ranker_metrics_provider.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +37 lines, -0 lines 0 comments Download
M ios/web_view/internal/translate/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M ios/web_view/internal/translate/web_view_translate_client.mm View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -0 lines 0 comments Download
A ios/web_view/internal/translate/web_view_translate_ranker_factory.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +48 lines, -0 lines 0 comments Download
A ios/web_view/internal/translate/web_view_translate_ranker_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +47 lines, -0 lines 0 comments Download
M net/url_request/test_url_fetcher_factory.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +10 lines, -10 lines 0 comments Download

Messages

Total messages: 179 (147 generated)
Roger McFarlane (Chromium)
PTAL? This CL downloads and caches translate ranker models (using a generic model loader template) ...
4 years ago (2016-12-16 21:27:13 UTC) #15
pasko
thanks I am not an owner in the area, and to me it seems there ...
4 years ago (2016-12-19 14:26:49 UTC) #18
fdoray
https://codereview.chromium.org/2565873002/diff/100001/components/translate/core/browser/translate_ranker_model_loader.h File components/translate/core/browser/translate_ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/core/browser/translate_ranker_model_loader.h#newcode147 components/translate/core/browser/translate_ranker_model_loader.h:147: // requirements of the URLFetcher. // TODO(fdoray): Make this ...
4 years ago (2016-12-19 15:18:29 UTC) #20
gab
Top-level review as requested. https://codereview.chromium.org/2565873002/diff/100001/components/translate/core/browser/translate_ranker.h File components/translate/core/browser/translate_ranker.h (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/core/browser/translate_ranker.h#newcode114 components/translate/core/browser/translate_ranker.h:114: mutable base::Lock lock_; On 2016/12/19 ...
4 years ago (2016-12-19 21:00:46 UTC) #22
groby-ooo-7-16
On 2016/12/19 21:00:46, gab wrote: > Top-level review as requested. > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/core/browser/translate_ranker.h > File ...
3 years, 12 months ago (2016-12-22 22:24:52 UTC) #27
hamelphi
https://codereview.chromium.org/2565873002/diff/480001/components/translate/core/browser/proto/ranker_model.proto File components/translate/core/browser/proto/ranker_model.proto (right): https://codereview.chromium.org/2565873002/diff/480001/components/translate/core/browser/proto/ranker_model.proto#newcode25 components/translate/core/browser/proto/ranker_model.proto:25: // The timestamp at which this model was download.. ...
3 years, 11 months ago (2017-01-20 18:44:18 UTC) #80
groby-ooo-7-16
https://codereview.chromium.org/2565873002/diff/500001/components/translate/core/browser/ranker_model_loader.cc File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/core/browser/ranker_model_loader.cc#newcode26 components/translate/core/browser/ranker_model_loader.cc:26: Any tests for the loader? https://codereview.chromium.org/2565873002/diff/500001/components/translate/core/browser/ranker_model_loader.cc#newcode27 components/translate/core/browser/ranker_model_loader.cc:27: class MyScopedHistogramTimer ...
3 years, 11 months ago (2017-01-23 21:41:57 UTC) #81
Roger McFarlane (Chromium)
Sorry for the extended delay. This CL has grown quite large (almost 1900 lines). I ...
3 years, 10 months ago (2017-02-08 23:08:16 UTC) #118
hamelphi
First quick review. Protos look good. I did not have time to review the rest ...
3 years, 10 months ago (2017-02-09 22:26:20 UTC) #121
hamelphi
Reviewed ranker_model_loader part. LG, only a few nits and comments. https://codereview.chromium.org/2565873002/diff/780001/components/translate/core/browser/ranker_model_loader.cc File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/core/browser/ranker_model_loader.cc#newcode117 ...
3 years, 10 months ago (2017-02-13 19:18:58 UTC) #122
groby-ooo-7-16
Overall, this lg to me, so I've got mostly nits/minor API design things, sorry :( ...
3 years, 10 months ago (2017-02-14 20:56:48 UTC) #123
fdoray
https://codereview.chromium.org/2565873002/diff/780001/components/translate/core/browser/ranker_model_loader.cc File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/core/browser/ranker_model_loader.cc#newcode30 components/translate/core/browser/ranker_model_loader.cc:30: const int kUrlFetcherId = 2; constexpr here and below ...
3 years, 10 months ago (2017-02-15 20:07:40 UTC) #124
Roger McFarlane (Chromium)
Thanks! Another look? https://codereview.chromium.org/2565873002/diff/780001/components/translate/core/browser/mock_translate_ranker.cc File components/translate/core/browser/mock_translate_ranker.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/core/browser/mock_translate_ranker.cc#newcode21 components/translate/core/browser/mock_translate_ranker.cc:21: return is_query_enabled_ || is_logging_enabled_ || is_enforcement_enabled_; ...
3 years, 10 months ago (2017-02-21 22:16:51 UTC) #126
groby-ooo-7-16
translate/* LG, except for translate_ranker_impl.* which I haven't reviewed yet (I'm hoping to get a ...
3 years, 10 months ago (2017-02-23 00:01:35 UTC) #130
fdoray
Task posting-related code lgtm. https://codereview.chromium.org/2565873002/diff/860001/components/translate/core/browser/ranker_model_loader.cc File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/860001/components/translate/core/browser/ranker_model_loader.cc#newcode10 components/translate/core/browser/ranker_model_loader.cc:10: #include "base/files/file_path.h" Already included in ...
3 years, 10 months ago (2017-02-23 16:41:26 UTC) #133
groby-ooo-7-16
On 2017/02/23 16:41:26, fdoray wrote: > Task posting-related code lgtm. > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/core/browser/ranker_model_loader.cc > File ...
3 years, 10 months ago (2017-02-23 19:03:53 UTC) #134
Roger McFarlane (Chromium)
Thanks. Adding additional OWNERS asanka: net/url_request/test_url_fetcher_factory.cc net/url_request/url_request_context_getter.cc lpromero: ios/chrome/browser/metrics/BUILD.gn ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm michaeldo: ios/web_view/internal/translate/BUILD.gn ios/web_view/internal/translate/criwv_translate_client.mm ios/web_view/internal/translate/criwv_translate_ranker_factory.cc ios/web_view/internal/translate/criwv_translate_ranker_factory.h ...
3 years, 10 months ago (2017-02-23 21:17:57 UTC) #139
asanka
https://codereview.chromium.org/2565873002/diff/900001/net/url_request/url_request_context_getter.cc File net/url_request/url_request_context_getter.cc (right): https://codereview.chromium.org/2565873002/diff/900001/net/url_request/url_request_context_getter.cc#newcode8 net/url_request/url_request_context_getter.cc:8: #include "base/debug/stack_trace.h" Unused
3 years, 10 months ago (2017-02-23 22:29:08 UTC) #142
rkaplow
lgtm histogram lg
3 years, 10 months ago (2017-02-23 22:57:21 UTC) #143
hamelphi
lgtm https://codereview.chromium.org/2565873002/diff/900001/components/translate/core/browser/translate_ranker.h File components/translate/core/browser/translate_ranker.h (right): https://codereview.chromium.org/2565873002/diff/900001/components/translate/core/browser/translate_ranker.h#newcode36 components/translate/core/browser/translate_ranker.h:36: // enforcement or logging are enabled, but can ...
3 years, 10 months ago (2017-02-23 23:05:03 UTC) #144
Roger McFarlane (Chromium)
Thanks https://codereview.chromium.org/2565873002/diff/900001/components/translate/core/browser/translate_ranker.h File components/translate/core/browser/translate_ranker.h (right): https://codereview.chromium.org/2565873002/diff/900001/components/translate/core/browser/translate_ranker.h#newcode36 components/translate/core/browser/translate_ranker.h:36: // enforcement or logging are enabled, but can ...
3 years, 10 months ago (2017-02-24 02:49:53 UTC) #146
Roger McFarlane (Chromium)
+davidben for net (asanka is OOO)
3 years, 10 months ago (2017-02-24 20:05:39 UTC) #152
Roger McFarlane (Chromium)
+eugenebut for ios/*
3 years, 10 months ago (2017-02-24 20:32:50 UTC) #154
Eugene But (OOO till 7-30)
sdefresne@ would be a better reviewer for translate code
3 years, 10 months ago (2017-02-24 21:19:11 UTC) #156
Roger McFarlane (Chromium)
+sdefresne for ios/*
3 years, 10 months ago (2017-02-24 23:00:40 UTC) #157
michaeldo
On 2017/02/24 23:00:40, Roger McFarlane (Chromium) wrote: > +sdefresne for ios/* The prefixes in ios/web_view ...
3 years, 9 months ago (2017-02-26 22:49:44 UTC) #158
Roger McFarlane (Chromium)
Ping looking for reviewers for ios/chrome/browser/metrics/* ios/web_view/internal/translate/* net/url_request/test_url_fetcher_factory.cc
3 years, 9 months ago (2017-02-27 17:57:09 UTC) #162
sdefresne
lgtm https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/translate/chrome_ios_translate_client.mm File ios/chrome/browser/translate/chrome_ios_translate_client.mm (right): https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/translate/chrome_ios_translate_client.mm#newcode40 ios/chrome/browser/translate/chrome_ios_translate_client.mm:40: translate_manager_(new translate::TranslateManager( nit: please use base::MakeUnique<> here if ...
3 years, 9 months ago (2017-02-27 21:27:18 UTC) #168
Roger McFarlane (Chromium)
Thanks! https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/translate/chrome_ios_translate_client.mm File ios/chrome/browser/translate/chrome_ios_translate_client.mm (right): https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/translate/chrome_ios_translate_client.mm#newcode40 ios/chrome/browser/translate/chrome_ios_translate_client.mm:40: translate_manager_(new translate::TranslateManager( On 2017/02/27 21:27:18, sdefresne wrote: > ...
3 years, 9 months ago (2017-02-27 21:58:34 UTC) #169
asanka
/net/ lgtm
3 years, 9 months ago (2017-02-27 22:07:21 UTC) #172
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/2565873002/980001
3 years, 9 months ago (2017-02-27 22:11:53 UTC) #176
commit-bot: I haz the power
3 years, 9 months ago (2017-02-27 23:12:05 UTC) #179
Message was sent while issue was closed.
Committed patchset #13 (id:980001) as
https://chromium.googlesource.com/chromium/src/+/2233a4a7cb12ac4acdebd33491b5...

Powered by Google App Engine
This is Rietveld 408576698