|
|
Created:
4 years ago by Roger McFarlane (Chromium) Modified:
3 years, 9 months ago Reviewers:
asanka, rkaplow, Ryan Sleevi, groby-ooo-7-16, fdoray, Eugene But (OOO till 7-30), davidben, sdefresne, hamelphi 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 #Messages
Total messages: 179 (147 generated)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
Description was changed from ========== [translate] Add translate ranker model loader. This CL extract the functionality to downlaod a cache a model file to a generic ModelLoader and then specializes that model loader for use with the TranslateRankerModel. BUG=646711 ========== to ========== [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 ==========
rogerm@chromium.org changed reviewers: + groby@chromium.org, hamelphi@chromium.org, pasko@chromium.org, rkaplow@chromium.org
PTAL? This CL downloads and caches translate ranker models (using a generic model loader template) in the background.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks I am not an owner in the area, and to me it seems there are a few style issues, so I would prefer for owners to decide on them first top-level questions/notes: 1. do you have browsertests for the feature or any other kind of integration tests? 2. please outline the threading model for each of the new classes (which threads they live on, which threads they are being accessed from, justify the amount of locking) https://codereview.chromium.org/2565873002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros_internal.h (right): https://codereview.chromium.org/2565873002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros_internal.h:108: typename std::remove_const<decltype(sample)>::type, \ please explain why this is relevant to this change https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ranker.cc (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker.cc:48: const char kUserDataDirSwitch[] = "user-data-dir"; this looks like the wrong layer to do it. kUserDataDir is handled by chrome_switches. Various profile storages consume it in profile_impl_io_data.cc or around those lines, without so many lines of conditional includes, like the above lines 27-40. I don't see any reason of using this lower level API for this one extra file on disk. If there is a reason, please make sure to put an explanation for it in this file. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ranker.h (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker.h:29: typedef ModelLoader<TranslateRankerModelTraits> TranslateRankerModelLoader; is this repetition of the same typedef intentional? https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker.h:114: mutable base::Lock lock_; generally we discourage locks as a measure to avoid deadlocks, priority inversion, etc. Please outline the threading model of the relevant classes in the design doc. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:70: ModelLoader& set_is_compatible_func(IsValidFunc f) { why is it necessary to inject various functions like this dynamically? it certainly makes it harder to read. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:195: FROM_HERE, base::Bind(&ModelLoader<T>::LoadData, base::Unretained(this))); base::Unretained(this): what ensures that this modelloader does not get destroyed on shutdown before this task runs? https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:199: void ModelLoader<T>::NotifyOfTranslateEvent() { please consider moving the implementation to the .cc file. Binary size is a large concern on android, identical code folding is not ideal, extra safety measures would help to avoid accidentally having several versions of this code in the binary. This code is not that large, it is rather a coding style issue. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:233: base::Bind(&ModelLoader<T>::OnDownloadComplete, base::Unretained(this))); what guarantees that this callback does not get run on shutdown when the ModelLoader has gone? which taskrunner is it on? https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:352: class TranslateRankerModelTraits { Do you have plans to add more traits? If not, please remove this abstraction layer. Let's keep it as simple as possible until it absolutely has to be more complex.
fdoray@chromium.org changed reviewers: + fdoray@chromium.org
https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:147: // requirements of the URLFetcher. // TODO(fdoray): Make this a SequencedTaskRunner once supported by URLFetcher. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:314: base::Time::Now() - download_start_time_); Use TimeTicks instead of Time to compute the the amount of time taken by a task https://cs.chromium.org/chromium/src/base/time/time.h?l=42 https://docs.google.com/document/d/1ypBZPZnshJ612FWAjkkpSBQiII7vYEEDsRkDOkVjQ...
gab@chromium.org changed reviewers: + gab@chromium.org
Top-level review as requested. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ranker.h (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker.h:114: mutable base::Lock lock_; On 2016/12/19 14:26:49, pasko wrote: > generally we discourage locks as a measure to avoid deadlocks, priority > inversion, etc. Please outline the threading model of the relevant classes in > the design doc. +1, prefer non-thread-safe classes running on sequences (split logic in two or more classes if there are distinct sequences of work sharing minimal state -- which they should share by posting tasks to each other instead of using a shared copy unless there's a performance/memory cost to doing so). Locks should only be used when doing all work in parallel (haven't verified closely whether this is the case, will read about it in design doc update). https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:36: typedef typename T::ModelType ModelType; using T::ModelType; should do the trick I think. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:49: OnAvailableFunc; As of C++11, using OnAvailableCallback = base::RepeatingCallback<void(std::unique_ptr<ModelType>)>; syntax is preferred (and "Func" is typically used for raw function pointers, "Callback" is more appropriate here). (same above) https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:52: ~ModelLoader() = default; Destructors should be defined out of line for classes with non-POD members. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:129: // Task functor to read the model from cache and/or kick off a model download. s/Task functor to read .../Reads.../ ? https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:169: // Functor used to check if a model is compatible with the caller. nit: s/Functor/Callback/ (and below) https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:227: base::TimeDelta::FromMinutes(kDownloadRefractoryPeriodMin); Use TimeTicks to compute any time difference, timers, etc. (base::Time is only useful when showing time in UI). https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:291: on_model_available_func_.Run(std::move(model)); From the API it wasn't obvious to me that this callback expects to be called from the worker task (i.e. on the worker sequence instead of the sequence owning the ModelLoader). If you want the callback to run on the owning sequence, then one typical paradigm is to use PostTaskAndReplyWithResult() and have the worker method return a value processed by the callback (via unique_ptr as you did is a good way to avoid unnecessary copies). https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:303: // initial download itempt. nit: itempt?
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/19 21:00:46, gab wrote: > Top-level review as requested. > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > File components/translate/core/browser/translate_ranker.h (right): > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > components/translate/core/browser/translate_ranker.h:114: mutable base::Lock > lock_; > On 2016/12/19 14:26:49, pasko wrote: > > generally we discourage locks as a measure to avoid deadlocks, priority > > inversion, etc. Please outline the threading model of the relevant classes in > > the design doc. > > +1, prefer non-thread-safe classes running on sequences (split logic in two or > more classes if there are distinct sequences of work sharing minimal state -- > which they should share by posting tasks to each other instead of using a shared > copy unless there's a performance/memory cost to doing so). > > Locks should only be used when doing all work in parallel (haven't verified > closely whether this is the case, will read about it in design doc update). > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > File components/translate/core/browser/translate_ranker_model_loader.h (right): > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > components/translate/core/browser/translate_ranker_model_loader.h:36: typedef > typename T::ModelType ModelType; > using T::ModelType; > > should do the trick I think. > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > components/translate/core/browser/translate_ranker_model_loader.h:49: > OnAvailableFunc; > As of C++11, > > using OnAvailableCallback = > base::RepeatingCallback<void(std::unique_ptr<ModelType>)>; > > syntax is preferred > > (and "Func" is typically used for raw function pointers, "Callback" is more > appropriate here). > > (same above) > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > components/translate/core/browser/translate_ranker_model_loader.h:52: > ~ModelLoader() = default; > Destructors should be defined out of line for classes with non-POD members. > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > components/translate/core/browser/translate_ranker_model_loader.h:129: // Task > functor to read the model from cache and/or kick off a model download. > s/Task functor to read .../Reads.../ ? > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > components/translate/core/browser/translate_ranker_model_loader.h:169: // > Functor used to check if a model is compatible with the caller. > nit: s/Functor/Callback/ > > (and below) > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > components/translate/core/browser/translate_ranker_model_loader.h:227: > base::TimeDelta::FromMinutes(kDownloadRefractoryPeriodMin); > Use TimeTicks to compute any time difference, timers, etc. (base::Time is only > useful when showing time in UI). > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > components/translate/core/browser/translate_ranker_model_loader.h:291: > on_model_available_func_.Run(std::move(model)); > From the API it wasn't obvious to me that this callback expects to be called > from the worker task (i.e. on the worker sequence instead of the sequence owning > the ModelLoader). > > If you want the callback to run on the owning sequence, then one typical > paradigm is to use PostTaskAndReplyWithResult() and have the worker method > return a value processed by the callback (via unique_ptr as you did is a good > way to avoid unnecessary copies). > > https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... > components/translate/core/browser/translate_ranker_model_loader.h:303: // > initial download itempt. > nit: itempt? FWIW: I was OOO(sick) until yesterday and will be OOO(holiday) until 1/8 - if this needs urgent review, please find another reviewer. Otherwise, will look 1/9
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:260001) has been deleted
Patchset #10 (id:240001) has been deleted
Patchset #9 (id:220001) has been deleted
Patchset #8 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
Patchset #9 (id:300001) has been deleted
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:320001) has been deleted
Patchset #8 (id:340001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #3 (id:360001) has been deleted
Patchset #3 (id:380001) has been deleted
Patchset #3 (id:390001) has been deleted
Patchset #3 (id:410001) has been deleted
Patchset #3 (id:430001) has been deleted
Patchset #3 (id:440001) has been deleted
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... File components/translate/core/browser/proto/ranker_model.proto (right): https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/ranker_model.proto:25: // The timestamp at which this model was download.. downloaded, remove one '.' https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/ranker_model.proto:30: optional int64 cache_duration_sec = 3 [default = 2592000]; What does it mean for a model to be expired? Is the model cache deleted? Does that imply a new download of the model after 30 days? Does 0 mean no time limit? Do we really want the models to expire by default? Is this controlled by Finch? For instance, for stable models such as CLD, we may not know in advance what the expect time of life of the model is. And if those models are big, we may not want to retrigger a download unless needed. https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/ranker_model.proto:38: oneof model_type { TranslateRankerModel translate = 2; } This could be only 'model'. I find that model_type does not reflect the fact that this actually is the proto of the model. We could have a model_type, id, type or name in the metadata section though. It could be useful to have some kind of name or key we can refer to in finch, so we could pass a dictionary of models, and also when we want to fetch the model internally. https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... File components/translate/core/browser/proto/translate_ranker_model.proto (left): https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/translate_ranker_model.proto:14: message TranslateRankerModel { IIUC, the structure of this proto will be required to be the same for any FooRankerModel, except for the list of protos in the |model_iteration|. Am I correct? https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... File components/translate/core/browser/proto/translate_ranker_model.proto (right): https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/translate_ranker_model.proto:13: // Defines a Chrome Ranker Translate Model '.' https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/translate_ranker_model.proto:33: oneof model_iteration { LogisticRegressionModel v1 = 2; } optional: model_revision maybe? https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/translate_ranker_model.proto:33: oneof model_iteration { LogisticRegressionModel v1 = 2; } This is not backwards compatible. I'd keep the logistic_regression_model until it is rolled out.
https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:26: Any tests for the loader? https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:27: class MyScopedHistogramTimer { Why not SCOPED_UMA_HISTOGRAM_TIMER? https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:47: bool IsExpired(const chrome_intelligence::RankerModel& model) { Why is "IsExpired" not a member on the model? It pretty much exclusively depends on model data? https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:90: options.message_loop_type = base::MessageLoop::TYPE_IO; Does this need a separate thread? Can we use a WorkerPool instead? https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:112: base::HistogramBase* histogram = base::LinearHistogram::FactoryGet( Why do we need to do this manually, instead of via a macro? https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:114: static_cast<int>(RankerModelStatus::MAX), Why the cast? IIRC the standard says underlying type for enum class should be int? https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:164: std::string source = model->metadata().source(); Technically, we only need src inside the !IsExpired expression https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/browser/ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:55: // used by this observer. Wait.. isn't that called by the task_runner associated with the RankerModelLoader? https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:122: // TODO(rogerm): Use net::URLFetcher directly? There's a bunch of useful setup code in TranslateURLFetcher, amongst others the "never do cookies" part. Maybe it's worth hoisting that out into its own file? https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/translate_manager.cc:232: bool ranker_should_offer_translation = true; Shouldn't this be initialized to false? If it doesn't enter the is-enabled block, it shouldn't really offer translation? https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/browser/translate_ranker_impl.cc (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/translate_ranker_impl.cc:31: namespace translate { It's kind of hard to figure out what's new in here, and what's copied over from the old translate_ranker.cc - what should I focus on? (Also, I'd suggest breaking things like virtualizing a class into a separate CL. In the future. No point in adding extra work here) https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/common/translate_switches.cc (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/common/translate_switches.cc:25: const char kTranslateRankerModelPath[] = "translate-ranker-model-path"; Do we really need this switch? https://codereview.chromium.org/2565873002/diff/500001/ios/chrome/browser/tra... File ios/chrome/browser/translate/chrome_ios_translate_client.mm (right): https://codereview.chromium.org/2565873002/diff/500001/ios/chrome/browser/tra... ios/chrome/browser/translate/chrome_ios_translate_client.mm:42: translate::TranslateRankerFactory::GetInstance()->GetForBrowserState( Will iOS actually utilize ranker?
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:520001) has been deleted
Patchset #6 (id:540001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Patchset #6 (id:560001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:600001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Patchset #6 (id:580001) has been deleted
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #6 (id:620001) has been deleted
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Patchset #6 (id:640001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:660001) has been deleted
Patchset #6 (id:680001) has been deleted
Patchset #6 (id:700001) has been deleted
Patchset #6 (id:720001) has been deleted
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:740001) has been deleted
Sorry for the extended delay. This CL has grown quite large (almost 1900 lines). I can probably split it in half, with perhaps a coupled of additional n-liners. ranker_model_loader* = ~800 lines integration = ~1000 lines Summary: - new RankerModelLoader class - TranslateRanker moves to being a KeyedService - Platform specific factories to create the ranker keyed service - Platform specific mtrics providers - integration https://codereview.chromium.org/2565873002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros_internal.h (right): https://codereview.chromium.org/2565873002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros_internal.h:108: typename std::remove_const<decltype(sample)>::type, \ On 2016/12/19 14:26:49, pasko wrote: > please explain why this is relevant to this change This macro cannot be used to reference template params inside a template decl/def without this. If std::remove_const<decltype(Foo)>::type is a dependent name, it requires typename or it fails to compile; otherwise, it's optional. C++ standard, section 14.6: A name used in a template declaration or definition and that is dependent on a template-parameter is assumed not to name a type unless the applicable name lookup finds a type name or the name is qualified by the keyword typename. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ranker.cc (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker.cc:48: const char kUserDataDirSwitch[] = "user-data-dir"; On 2016/12/19 14:26:49, pasko wrote: > this looks like the wrong layer to do it. kUserDataDir is handled by > chrome_switches. Various profile storages consume it in profile_impl_io_data.cc > or around those lines, without so many lines of conditional includes, like the > above lines 27-40. I don't see any reason of using this lower level API for this > one extra file on disk. If there is a reason, please make sure to put an > explanation for it in this file. I've turned the ranker into a KeyedService and constructed it from a context where these values are more easily obtained. So, this code has been removed. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ranker.h (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker.h:29: typedef ModelLoader<TranslateRankerModelTraits> TranslateRankerModelLoader; On 2016/12/19 14:26:49, pasko wrote: > is this repetition of the same typedef intentional? There's no repetition here. Forward declare TranslatePrefs Forward declare TranslateRankerModelTraits; Forward declare ModelLoader (template) Forward declare TranslateRankerModelLoader (template specialization) https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker.h:114: mutable base::Lock lock_; On 2016/12/19 21:00:46, gab wrote: > On 2016/12/19 14:26:49, pasko wrote: > > generally we discourage locks as a measure to avoid deadlocks, priority > > inversion, etc. Please outline the threading model of the relevant classes in > > the design doc. > > +1, prefer non-thread-safe classes running on sequences (split logic in two or > more classes if there are distinct sequences of work sharing minimal state -- > which they should share by posting tasks to each other instead of using a shared > copy unless there's a performance/memory cost to doing so). > > Locks should only be used when doing all work in parallel (haven't verified > closely whether this is the case, will read about it in design doc update). Done. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker.h:114: mutable base::Lock lock_; On 2016/12/19 14:26:49, pasko (OOO until 2017-01-23) wrote: > generally we discourage locks as a measure to avoid deadlocks, priority > inversion, etc. Please outline the threading model of the relevant classes in > the design doc. Done. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... File components/translate/core/browser/translate_ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:36: typedef typename T::ModelType ModelType; On 2016/12/19 21:00:46, gab (slow at conference) wrote: > using T::ModelType; > > should do the trick I think. Acknowledged. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:49: OnAvailableFunc; On 2016/12/19 21:00:46, gab (slow at conference) wrote: > As of C++11, > > using OnAvailableCallback = > base::RepeatingCallback<void(std::unique_ptr<ModelType>)>; > > syntax is preferred > > (and "Func" is typically used for raw function pointers, "Callback" is more > appropriate here). > > (same above) Acknowledged. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:52: ~ModelLoader() = default; On 2016/12/19 21:00:46, gab (slow at conference) wrote: > Destructors should be defined out of line for classes with non-POD members. Done. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:70: ModelLoader& set_is_compatible_func(IsValidFunc f) { On 2016/12/19 14:26:49, pasko wrote: > why is it necessary to inject various functions like this dynamically? it > certainly makes it harder to read. Moved this functionality to a ModelObserver interface, passed in on construction. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:129: // Task functor to read the model from cache and/or kick off a model download. On 2016/12/19 21:00:46, gab (slow at conference) wrote: > s/Task functor to read .../Reads.../ ? Done. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:147: // requirements of the URLFetcher. On 2016/12/19 15:18:29, fdoray wrote: > // TODO(fdoray): Make this a SequencedTaskRunner once supported by URLFetcher. Acknowledged. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:169: // Functor used to check if a model is compatible with the caller. On 2016/12/19 21:00:46, gab (slow at conference) wrote: > nit: s/Functor/Callback/ > > (and below) Acknowledged. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:195: FROM_HERE, base::Bind(&ModelLoader<T>::LoadData, base::Unretained(this))); On 2016/12/19 14:26:49, pasko wrote: > base::Unretained(this): what ensures that this modelloader does not get > destroyed on shutdown before this task runs? The traits of the task runner are to skip running the task on shutdown. I switched to an owned thread so this potential race should go away. At some point, I'd like to switch back to a task_runner, but I need to resolve some issues with setting up the task scheduler environment for unittests. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:199: void ModelLoader<T>::NotifyOfTranslateEvent() { On 2016/12/19 14:26:49, pasko wrote: > please consider moving the implementation to the .cc file. Binary size is a > large concern on android, identical code folding is not ideal, extra safety > measures would help to avoid accidentally having several versions of this code > in the binary. > > This code is not that large, it is rather a coding style issue. Switched to a fixed ranker model proto instead of template based; so, all code has been moved to .cc files. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:227: base::TimeDelta::FromMinutes(kDownloadRefractoryPeriodMin); On 2016/12/19 21:00:46, gab (slow at conference) wrote: > Use TimeTicks to compute any time difference, timers, etc. (base::Time is only > useful when showing time in UI). Done. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:233: base::Bind(&ModelLoader<T>::OnDownloadComplete, base::Unretained(this))); On 2016/12/19 14:26:49, pasko wrote: > what guarantees that this callback does not get run on shutdown when the > ModelLoader has gone? which taskrunner is it on? It's on the current threads task runner. Which is now the thread owned by the model loader. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:291: on_model_available_func_.Run(std::move(model)); On 2016/12/19 21:00:46, gab (slow at conference) wrote: > From the API it wasn't obvious to me that this callback expects to be called > from the worker task (i.e. on the worker sequence instead of the sequence owning > the ModelLoader). > > If you want the callback to run on the owning sequence, then one typical > paradigm is to use PostTaskAndReplyWithResult() and have the worker method > return a value processed by the callback (via unique_ptr as you did is a good > way to avoid unnecessary copies). I've switched the API to an observer model, where the observer knows to post the model back into its own sequence as required. Hopefully that's clearer? https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:303: // initial download itempt. On 2016/12/19 21:00:46, gab (slow at conference) wrote: > nit: itempt? Done. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:314: base::Time::Now() - download_start_time_); On 2016/12/19 15:18:29, fdoray wrote: > Use TimeTicks instead of Time to compute the the amount of time taken by a task > https://cs.chromium.org/chromium/src/base/time/time.h?l=42 > https://docs.google.com/document/d/1ypBZPZnshJ612FWAjkkpSBQiII7vYEEDsRkDOkVjQ... Done. https://codereview.chromium.org/2565873002/diff/100001/components/translate/c... components/translate/core/browser/translate_ranker_model_loader.h:352: class TranslateRankerModelTraits { On 2016/12/19 14:26:49, pasko wrote: > Do you have plans to add more traits? If not, please remove this abstraction > layer. Let's keep it as simple as possible until it absolutely has to be more > complex. Done. https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... File components/translate/core/browser/proto/ranker_model.proto (right): https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/ranker_model.proto:25: // The timestamp at which this model was download.. On 2017/01/20 18:44:18, hamelphi wrote: > downloaded, remove one '.' Done. https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/ranker_model.proto:30: optional int64 cache_duration_sec = 3 [default = 2592000]; On 2017/01/20 18:44:18, hamelphi wrote: > What does it mean for a model to be expired? Is the model cache deleted? > Does that imply a new download of the model after 30 days? > Does 0 mean no time limit? > Do we really want the models to expire by default? > Is this controlled by Finch? > > For instance, for stable models such as CLD, we may not know in advance what the > expect time of life of the model is. And if those models are big, we may not > want to retrigger a download unless needed. Updated thee documentation and dropped the default. This is optional functionality, which for translate ranker we probably won't use, since we plan to publish immutable models which never expire and move the URL in Finch. I can pull this out with the assumption that YAGNI and if we need it eventually, it's easy to put back in. https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/ranker_model.proto:38: oneof model_type { TranslateRankerModel translate = 2; } On 2017/01/20 18:44:18, hamelphi wrote: > This could be only 'model'. I find that model_type does not reflect the fact > that this actually is the proto of the model. That's not how the proto API works. You don't access the members through the oneof name, you access them directly and switch on the oneof name. I.e., it's the prefix of the switch case value you use to check which of the member fields should be valid; it becomes an enum. > We could have a model_type, id, type or name in the metadata section though. It > could be useful to have some kind of name or key we can refer to in finch, so we > could pass a dictionary of models, and also when we want to fetch the model > internally. Added a name and label. https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... File components/translate/core/browser/proto/translate_ranker_model.proto (left): https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/translate_ranker_model.proto:14: message TranslateRankerModel { On 2017/01/20 18:44:18, hamelphi wrote: > IIUC, the structure of this proto will be required to be the same for any > FooRankerModel, except for the list of protos in the |model_iteration|. Am I > correct? There no technical requirement that other FooRankerModel's follow this pattern. The expected convention would be that they would either have a oneof list of contained model implementations or a set of optional model implementation and some way to discriminate between them. I would ultimately expect these to all look something like: message FooRankerModel { oneof { // Initial release of the FooRankerModel. Supports // features X and Y. Deprecated as of DD/MM/YYYY. GenericNeuralNetModelProto v1 = 1; // Second release of the FooRankerModel. Adds features // A and B and renames feature Y to Z. GenericNeuralNetModelProto v2 = 1; } } Or something similar. https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... File components/translate/core/browser/proto/translate_ranker_model.proto (right): https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/translate_ranker_model.proto:13: // Defines a Chrome Ranker Translate Model On 2017/01/20 18:44:18, hamelphi wrote: > '.' Done. https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/translate_ranker_model.proto:33: oneof model_iteration { LogisticRegressionModel v1 = 2; } On 2017/01/20 18:44:18, hamelphi wrote: > optional: model_revision maybe? Done. https://codereview.chromium.org/2565873002/diff/480001/components/translate/c... components/translate/core/browser/proto/translate_ranker_model.proto:33: oneof model_iteration { LogisticRegressionModel v1 = 2; } On 2017/01/20 18:44:18, hamelphi wrote: > This is not backwards compatible. I'd keep the logistic_regression_model until > it is rolled out. This whole this isn't backwards compatible now that there's an envelope around the model. The current model has to stay where it currently lives and the new model can be brought up going forward. https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:26: On 2017/01/23 21:41:56, groby wrote: > Any tests for the loader? Coming. https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:27: class MyScopedHistogramTimer { On 2017/01/23 21:41:56, groby wrote: > Why not SCOPED_UMA_HISTOGRAM_TIMER? The UMA macros require static const data for the metric names. https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:47: bool IsExpired(const chrome_intelligence::RankerModel& model) { On 2017/01/23 21:41:56, groby wrote: > Why is "IsExpired" not a member on the model? It pretty much exclusively depends > on model data? RankerModel is a proto. I could rename the proto to RankerModelProto and add a RankerModel wrapper class, to which I could attach helper methods. https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:90: options.message_loop_type = base::MessageLoop::TYPE_IO; On 2017/01/23 21:41:56, groby wrote: > Does this need a separate thread? Can we use a WorkerPool instead? Ideally, I'd use the task scheduler api and just get a sequenced runner. I'm working with gab@ and fdoray@ to sort out how to set up the proper unittest scaffolding for this. I.e., the only reason it's a thread is because we're trying to figure out how to set up the unittests for the task scheduler properly (otherwise crashes ensue) https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:112: base::HistogramBase* histogram = base::LinearHistogram::FactoryGet( On 2017/01/23 21:41:56, groby wrote: > Why do we need to do this manually, instead of via a macro? The macros require static const histogram names. https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:114: static_cast<int>(RankerModelStatus::MAX), On 2017/01/23 21:41:56, groby wrote: > Why the cast? IIRC the standard says underlying type for enum class should be > int? clang++ fails to resolve the conversion from the enum to Sample (aka int). This happens even with enum class RankerModelStatus : int { ... }; :( https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:164: std::string source = model->metadata().source(); On 2017/01/23 21:41:56, groby wrote: > Technically, we only need src inside the !IsExpired expression Oops! the DVLOG line above had the wrong source. Good catch! https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/browser/ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:55: // used by this observer. On 2017/01/23 21:41:57, groby wrote: > Wait.. isn't that called by the task_runner associated with the > RankerModelLoader? Sorry, a previous iteration had a task_runner() getter and the loader would PostTask to the task_runner to run OnModelAvailable. I neglected to update the comment when I hid the task_runner behind the interface. Done. https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:122: // TODO(rogerm): Use net::URLFetcher directly? On 2017/01/23 21:41:56, groby wrote: > There's a bunch of useful setup code in TranslateURLFetcher, amongst others the > "never do cookies" part. Maybe it's worth hoisting that out into its own file? I can look at creating a CookieLessURLFetcher or something, that might be reusable. The main reason I reused the TranslateURLFetcher is that it also defers the retry handling to the caller, which I used to implement the retry if and when the browser navigates, because that may mean it has network connectivity. https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/translate_manager.cc:232: bool ranker_should_offer_translation = true; On 2017/01/23 21:41:57, groby wrote: > Shouldn't this be initialized to false? If it doesn't enter the is-enabled > block, it shouldn't really offer translation? In it's current incarnation, the ranker is as gate, not a trigger. I the ranker is not enabled and/or it's enforcement is not enabled, the default/existing behavior is to show the prompt, subject to the various other limiting heuristics below. To make it clearer, I've gotten rid of the generic IsEnabled() and retained just the specific IsQueryEnabled(), IsEnforcementEnabled(), IsLoggingEnabled() methods.. https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/browser/translate_ranker_impl.cc (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/browser/translate_ranker_impl.cc:31: namespace translate { On 2017/01/23 21:41:57, groby wrote: > It's kind of hard to figure out what's new in here, and what's copied over from > the old translate_ranker.cc - what should I focus on? > > (Also, I'd suggest breaking things like virtualizing a class into a separate CL. > In the future. No point in adding extra work here) Almost nothing changed from the old translate_ranker.cc other than the class name. Some of the changes requested by other reviewers meant a fairly large refactor from my original CL proposal in order to avoid some logic duplication and/or avoid abusing the abstraction layering. Hence the virtualization of the TranslateRanker to an interface. It also made the testing a bit simpler. https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... File components/translate/core/common/translate_switches.cc (right): https://codereview.chromium.org/2565873002/diff/500001/components/translate/c... components/translate/core/common/translate_switches.cc:25: const char kTranslateRankerModelPath[] = "translate-ranker-model-path"; On 2017/01/23 21:41:57, groby wrote: > Do we really need this switch? It was intended to allow for testing of a particular model file (by precluding the download step). But, that would be the equivalent of specifying an empty/invalid URL... Removed. https://codereview.chromium.org/2565873002/diff/500001/ios/chrome/browser/tra... File ios/chrome/browser/translate/chrome_ios_translate_client.mm (right): https://codereview.chromium.org/2565873002/diff/500001/ios/chrome/browser/tra... ios/chrome/browser/translate/chrome_ios_translate_client.mm:42: translate::TranslateRankerFactory::GetInstance()->GetForBrowserState( On 2017/01/23 21:41:57, groby wrote: > Will iOS actually utilize ranker? That's the plan. Should it not?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
First quick review. Protos look good. I did not have time to review the rest of the CL, I'll come back to it soon.
Reviewed ranker_model_loader part. LG, only a few nits and comments. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:117: // Transfer ownership of |model| to the |observer_|. |observer_| is not defined in this class. Do you mean |on_model_available_callback_|, or the model loader client? It would be good to describe what is meant by "observer" here or in the .h file (I assume the model loader client?). "Observer" is used in a few places throughout the file and we are left to infer what it exactly means. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:269: if (url_spec == model_url_.spec() && !is_expired) Maybe comment on what it means if either condition is false. IIUC, we have a valid but expired, or deprecated model. We pass it to the "observer" so that it can use the current model as a fallback, but will schedule the download of a fresh model and update the observer once it is available. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:299: DVLOG(2) << "ModelLoadser: Last download attempt was too recent."; s\ModelLoadser\ModelLoader https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:47: using ValidateCallback = base::Callback<RankerModelStatus( s\ValidateCallback\ValidateModelCallback ? I think it would make it more obvious what the purpose of this callback is. I don't insist. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:64: // |uma_prefix| will be used to start all UMA metrics genereted by this generated https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:64: // |uma_prefix| will be used to start all UMA metrics genereted by this Nit. I was confused a bit by "used to start..." Maybe "will be used as a prefix for..."
Overall, this lg to me, so I've got mostly nits/minor API design things, sorry :( Reviewed mock_translate_ranker* and ranker_model_loader* in isolation so far. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/mock_translate_ranker.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/mock_translate_ranker.cc:21: return is_query_enabled_ || is_logging_enabled_ || is_enforcement_enabled_; I'm not sure we should encode this behavior in the mock. I'd rather have CHECK(!is_logging_enabled || is_query_enabled); CHECK(!is_enforcement_enabled || is_query_enabled) to force tests to fail with a badly initialized object. (It's still vulnerable to behavior changes in the original object, but presumably new tests would flush that out when new behavior is added - IsQueryEnabled as is would just silently have the old behavior) https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:30: const int kUrlFetcherId = 2; Why do we hardcode a fetcher ID? I've only seen this used for tests, in which case we should probably expose this to share with the test. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:37: const int kDownloadRefractoryPeriodMin = 3; 1) since this is a duration, I'd love for this to be a TimeDelta 2) That's a complicated name. (I had no idea what a refractory period was). Would you mind using a slightly simpler name? Maybe kDownloadMinimumWaitTime? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:50: MyScopedHistogramTimer(const base::StringPiece& name) FWIW: I take it the UMA peeps are OK with dynamically named histograms? (Did I ask this before? It's something that does make me nervous) https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:70: bool IsExpired(const chrome_intelligence::RankerModel& model) { Should this be a member on the model? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:110: // Called to construct a model from the given |data|. nit: Please don't use passive voice: "Constructs a model from the given |data|". (Here and elsewhere) https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:114: // Called the download initiated by LoadFromURL() completes. Would you mind documenting |id|? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:125: // The TaskRunner on which this was constructed. nit: "On which |this| was constructed" https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:153: base::TimeTicks next_earliest_download_time_; Why not just use a timer to schedule the next attempt? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:192: Maybe at least a double line separator to make it easier to find the end of RankerModelLoader/Beginning of Backend? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:208: RankerModelLoader::Backend::~Backend() = default; curious - why RankerModelLoader::Backend::~Backend() = default; instead of RankerModelLoader::Backend::~Backend() {}; https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:233: RankerModelStatus model_status = validate_callback_.Run(*model); Thought: If the callback took a ptr, and you had a factory for RankerModel, this flow is much more straightforward: auto model = chrome_intelligence::RankerModel::CreateFromString(data); ReportModelStatus(validate_callback(model.get())); return model; Which means you could inline it below :) https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:283: LoadFromURL(); FWIW, LoadFromURL and LoadFromFile have slightly asymmetric behavior in that one checks its parameters for validity, and the other doesn't. I wonder if the above code should live in LoadFromURL, and current LoadFromURL should be LoadFromURLFetcher https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:292: // If a request is already in flight, do not issue a new one. IIUC, we only need this because we're currently using essentially a polling approach for Network availability. One more point of a transition-based approach? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:341: // translation opportunity. The TranslateURLFetcher enforces a limit for Should we instead retry once network is available? I.e. an opportunistic fetch? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:26: // TODO(rogerm): rename the enum in histograms.xml to be more generic Can you do that as part of this CL? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:51: // client. This will be called on the sequence on which the model loader was Since we're having a set of callbacks here, does it make sense to use a Delegate instead of several callbacks? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:56: // |valildate_callback| may be called on any sequence; it must, therefore, s/valildate/validate https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:62: // |model_path| denotes the file path nit: end with period. More importantly: This probably deserves an explanation why there is both a URL and a path. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:75: // previously configured. Why not pass path and URL here, then? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:79: // be available. If a model download is pending, this will trigger (subject to That's probably not the right name if we call this periodically, then. I'd expect this to only be called on network signal edges as is. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:86: class Backend; Why do we have a separate backend? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader_unittest.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:44: class RankerModelLoaderTest : public ::testing::Test { This is a fairly huge fixture. I like those better with the code being outside the declaration for readability, but I'm not insisting. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:53: TranslateDownloadManager::GetInstance()->set_application_locale("en-US"); Would you mind not using en-US? I have the sneaking suspicion that half of the tests running in en-US only work because that's the default for devs :) https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:58: const auto& temp_dir = scoped_temp_dir_.GetPath(); temp_dir_path? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:175: available_models_.push_back(*model); Wait - isn't that model deleted when you leave OnModelAvailable, since it's a unique_ptr, and you don't Pass/move to available_models? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:227: ASSERT_EQ(0U, validated_models_.size()); Nit: The last two should probably be EXPECT_EQ, not ASSERT_EQ At least that's my understanding: ASSERT_* is a precondition for the test to even be able to run. EXPECT_* is the expected outcome of a test. I.e. ASSERT a vector is not empty, so you can EXPECT on the first value in it.
https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:30: const int kUrlFetcherId = 2; constexpr here and below https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:169: base::TaskTraits().MayBlock())), Backend doesn't access memory that could be freed during shutdown? Add .WithShutdownBehavior(base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) to allow other components to shutdown and the process to be terminated even if Backend is still running. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:46: // may be called on any sequence and must, therefore, be thread-safe. + // This callback may be invoked after RankerModelLoader has been destroyed and therefore shouldn't access memory that it doesn't own. (Since the callback outlives RankerModelLoader, there is no way to know when it is safe to release memory that is accessed but not owned by the callback.) https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:51: // client. This will be called on the sequence on which the model loader was On 2017/02/14 20:56:48, groby wrote: > Since we're having a set of callbacks here, does it make sense to use a Delegate > instead of several callbacks? Since ValidateCallback can be invoked after the RankerModelLoader has been destroyed, the Delegate would have to be owned by the RankerModelLoader. This is undesirable. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:86: class Backend; On 2017/02/14 20:56:48, groby wrote: > Why do we have a separate backend? The Backend performs asynchronous operations and can outlive the RankerModelLoader. We don't want to block in ~RankerModelLoader if a Backend operation is in progress. Maybe: // The backend that handles asynchronous loads. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:92: // delegated. + // When the RankerModelLoader is destroyed, ownership of |backend_| is transferred to a delete task posted to |backend_task_runner_|. That means that Backend can outlive RankerModelLoader.
Patchset #8 (id:800001) has been deleted
Thanks! Another look? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/mock_translate_ranker.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/mock_translate_ranker.cc:21: return is_query_enabled_ || is_logging_enabled_ || is_enforcement_enabled_; On 2017/02/14 20:56:47, groby wrote: > I'm not sure we should encode this behavior in the mock. I'd rather have > CHECK(!is_logging_enabled || is_query_enabled); > CHECK(!is_enforcement_enabled || is_query_enabled) > > to force tests to fail with a badly initialized object. (It's still vulnerable > to behavior changes in the original object, but presumably new tests would flush > that out when new behavior is added - IsQueryEnabled as is would just silently > have the old behavior) Simplified this to do no implicit enabling. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:30: const int kUrlFetcherId = 2; On 2017/02/14 20:56:47, groby wrote: > Why do we hardcode a fetcher ID? I've only seen this used for tests, in which > case we should probably expose this to share with the test. I followed the other examples of the TranslateURLFetcher instantiation(s). Which specify a hard-coded ID. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:30: const int kUrlFetcherId = 2; On 2017/02/15 20:07:40, fdoray wrote: > constexpr here and below Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:37: const int kDownloadRefractoryPeriodMin = 3; On 2017/02/14 20:56:47, groby wrote: > 1) since this is a duration, I'd love for this to be a TimeDelta I'll make this an input param (via an options class) to the loader. But I'll defer that to another CL. > 2) That's a complicated name. (I had no idea what a refractory period was). > Would you mind using a slightly simpler name? Maybe kDownloadMinimumWaitTime? Renamed to kMinRetryDelayMins for now. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:50: MyScopedHistogramTimer(const base::StringPiece& name) On 2017/02/14 20:56:48, groby wrote: > FWIW: I take it the UMA peeps are OK with dynamically named histograms? (Did I > ask this before? It's something that does make me nervous) I got this recipe from the UMA folks, yes. I'll add them to the CL to give an official yay or nay. These metrics should only be constructed a few times over the life of the process, so its a similar amount of work to what static allocations would be. Static allocations use the same recipe, but cache the result to a function local variable. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:70: bool IsExpired(const chrome_intelligence::RankerModel& model) { On 2017/02/14 20:56:47, groby wrote: > Should this be a member on the model? Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:110: // Called to construct a model from the given |data|. On 2017/02/14 20:56:48, groby wrote: > nit: Please don't use passive voice: "Constructs a model from the given |data|". > (Here and elsewhere) Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:114: // Called the download initiated by LoadFromURL() completes. On 2017/02/14 20:56:47, groby wrote: > Would you mind documenting |id|? Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:117: // Transfer ownership of |model| to the |observer_|. On 2017/02/13 19:18:58, hamelphi wrote: > |observer_| is not defined in this class. Do you mean > |on_model_available_callback_|, or the model loader client? It would be good to > describe what is meant by "observer" here or in the .h file (I assume the model > loader client?). "Observer" is used in a few places throughout the file and we > are left to infer what it exactly means. I updated the comment and method names to refer to the client. thanks! https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:125: // The TaskRunner on which this was constructed. On 2017/02/14 20:56:48, groby wrote: > nit: "On which |this| was constructed" Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:153: base::TimeTicks next_earliest_download_time_; On 2017/02/14 20:56:47, groby wrote: > Why not just use a timer to schedule the next attempt? I'm actually using this a throttle, not a deadline/schedule. I'm using translate initiation as a proxy for "chrome has network connectivity". I only want to attempt a download if I have reason to believe that the device has network connectivity. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:169: base::TaskTraits().MayBlock())), On 2017/02/15 20:07:40, fdoray wrote: > Backend doesn't access memory that could be freed during shutdown? Add > .WithShutdownBehavior(base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) to allow > other components to shutdown and the process to be terminated even if Backend is > still running. Done. The backend forwards downloaded models to the recipient via the on_model_available_callback_, which is bound to a weakptr. So, this should be all good. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:192: On 2017/02/14 20:56:48, groby wrote: > Maybe at least a double line separator to make it easier to find the end of > RankerModelLoader/Beginning of Backend? Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:208: RankerModelLoader::Backend::~Backend() = default; On 2017/02/14 20:56:47, groby wrote: > curious - why > > RankerModelLoader::Backend::~Backend() = default; > > instead of > > RankerModelLoader::Backend::~Backend() {}; they're equivalent, so no particular reason. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:233: RankerModelStatus model_status = validate_callback_.Run(*model); On 2017/02/14 20:56:48, groby wrote: > Thought: If the callback took a ptr, and you had a factory for RankerModel, this > flow is much more straightforward: > > auto model = chrome_intelligence::RankerModel::CreateFromString(data); > ReportModelStatus(validate_callback(model.get())); > return model; > > Which means you could inline it below :) Not quite. I might have a non-null, but invalid model that I don't want to return. I'd also lose my timing info. I've condensed it a bit and added the parse failure message to the callback (taking the model as a pointer). https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:269: if (url_spec == model_url_.spec() && !is_expired) On 2017/02/13 19:18:58, hamelphi wrote: > Maybe comment on what it means if either condition is false. IIUC, we have a > valid but expired, or deprecated model. We pass it to the "observer" so that it > can use the current model as a fallback, but will schedule the download of a > fresh model and update the observer once it is available. Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:283: LoadFromURL(); On 2017/02/14 20:56:48, groby wrote: > FWIW, LoadFromURL and LoadFromFile have slightly asymmetric behavior in that one > checks its parameters for validity, and the other doesn't. > > I wonder if the above code should live in LoadFromURL, and current LoadFromURL > should be LoadFromURLFetcher I made them a bit more symmetric. url_fetcher_ == nullptr is no longer used as a enabled/disabled flag, the loader now tracks the completion state of the backend. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:292: // If a request is already in flight, do not issue a new one. On 2017/02/14 20:56:47, groby wrote: > IIUC, we only need this because we're currently using essentially a polling > approach for Network availability. One more point of a transition-based > approach? By transition-based approach, you mean watching the network state? My thinking in doing it this way is that I want to trigger a download retry when the user is doing something that would require the model. I don't want to download the model on a metered network, for example, if the user wasn't already downloading something on that network. I also don't want Chrome waking up at arbitrary times just to download the model in the background. i.e., I see network availability as a necessary but insufficient signal of a good time to download the model. Your thoughts? https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:299: DVLOG(2) << "ModelLoadser: Last download attempt was too recent."; On 2017/02/13 19:18:58, hamelphi wrote: > s\ModelLoadser\ModelLoader Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:341: // translation opportunity. The TranslateURLFetcher enforces a limit for On 2017/02/14 20:56:48, groby wrote: > Should we instead retry once network is available? I.e. an opportunistic fetch? See my previous comments/thoughts/questions about this re: line 292 https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:26: // TODO(rogerm): rename the enum in histograms.xml to be more generic On 2017/02/14 20:56:48, groby wrote: > Can you do that as part of this CL? Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:46: // may be called on any sequence and must, therefore, be thread-safe. On 2017/02/15 20:07:40, fdoray wrote: > + // This callback may be invoked after RankerModelLoader has been destroyed and > therefore shouldn't access memory that it doesn't own. > > (Since the callback outlives RankerModelLoader, there is no way to know when it > is safe to release memory that is accessed but not owned by the callback.) Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:47: using ValidateCallback = base::Callback<RankerModelStatus( On 2017/02/13 19:18:58, hamelphi wrote: > s\ValidateCallback\ValidateModelCallback ? > > I think it would make it more obvious what the purpose of this callback is. > I don't insist. Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:51: // client. This will be called on the sequence on which the model loader was On 2017/02/14 20:56:48, groby wrote: > Since we're having a set of callbacks here, does it make sense to use a Delegate > instead of several callbacks? fdoray@ suggested to revert to callbacks (instead of an observer/delegate) to simplify lifetime and dependency management. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:51: // client. This will be called on the sequence on which the model loader was On 2017/02/15 20:07:40, fdoray wrote: > On 2017/02/14 20:56:48, groby wrote: > > Since we're having a set of callbacks here, does it make sense to use a > Delegate > > instead of several callbacks? > > Since ValidateCallback can be invoked after the RankerModelLoader has been > destroyed, the Delegate would have to be owned by the RankerModelLoader. This is > undesirable. Acknowledged. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:56: // |valildate_callback| may be called on any sequence; it must, therefore, On 2017/02/14 20:56:48, groby wrote: > s/valildate/validate Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:62: // |model_path| denotes the file path On 2017/02/14 20:56:48, groby wrote: > nit: end with period. > > More importantly: This probably deserves an explanation why there is both a URL > and a path. Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:64: // |uma_prefix| will be used to start all UMA metrics genereted by this On 2017/02/13 19:18:58, hamelphi wrote: > Nit. > I was confused a bit by "used to start..." > Maybe "will be used as a prefix for..." Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:64: // |uma_prefix| will be used to start all UMA metrics genereted by this On 2017/02/13 19:18:58, hamelphi wrote: > generated Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:75: // previously configured. On 2017/02/14 20:56:48, groby wrote: > Why not pass path and URL here, then? The loader is stateful. Passing the path and url here would imply (to me at least) that the caller should expect to be able to Start() multiple path/url loads, which isn't supported in this iteration. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:79: // be available. If a model download is pending, this will trigger (subject to On 2017/02/14 20:56:48, groby wrote: > That's probably not the right name if we call this periodically, then. I'd > expect this to only be called on network signal edges as is. Renamed to NotifyOfRankerActivity https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:86: class Backend; On 2017/02/14 20:56:48, groby wrote: > Why do we have a separate backend? The threading and task-scheduling folks recommended that I break up the classes by the sequences on which they run... to not have multiple threads/sequences running through the same class instance. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:86: class Backend; On 2017/02/15 20:07:40, fdoray wrote: > On 2017/02/14 20:56:48, groby wrote: > > Why do we have a separate backend? > > The Backend performs asynchronous operations and can outlive the > RankerModelLoader. We don't want to block in ~RankerModelLoader if a Backend > operation is in progress. > Maybe: > > // The backend that handles asynchronous loads. Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:92: // delegated. On 2017/02/15 20:07:40, fdoray wrote: > + // When the RankerModelLoader is destroyed, ownership of |backend_| is > transferred to a delete task posted to |backend_task_runner_|. That means that > Backend can outlive RankerModelLoader. Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... File components/translate/core/browser/ranker_model_loader_unittest.cc (right): https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:44: class RankerModelLoaderTest : public ::testing::Test { On 2017/02/14 20:56:48, groby wrote: > This is a fairly huge fixture. I like those better with the code being outside > the declaration for readability, but I'm not insisting. Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:53: TranslateDownloadManager::GetInstance()->set_application_locale("en-US"); On 2017/02/14 20:56:48, groby wrote: > Would you mind not using en-US? I have the sneaking suspicion that half of the > tests running in en-US only work because that's the default for devs :) Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:58: const auto& temp_dir = scoped_temp_dir_.GetPath(); On 2017/02/14 20:56:48, groby wrote: > temp_dir_path? Done. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:175: available_models_.push_back(*model); On 2017/02/14 20:56:48, groby wrote: > Wait - isn't that model deleted when you leave OnModelAvailable, since it's a > unique_ptr, and you don't Pass/move to available_models? This was storing a copy of the model. I've added a RankerModel wrapper class between the raw proto and the ranker, which isn't directly copy-able, so I've switched this to holding unique_ptr to RankerModel. https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:227: ASSERT_EQ(0U, validated_models_.size()); On 2017/02/14 20:56:48, groby wrote: > Nit: The last two should probably be EXPECT_EQ, not ASSERT_EQ > > At least that's my understanding: > ASSERT_* is a precondition for the test to even be able to run. EXPECT_* is > the expected outcome of a test. I.e. ASSERT a vector is not empty, so you can > EXPECT on the first value in it. Done.
Patchset #8 (id:820001) has been deleted
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
translate/* LG, except for translate_ranker_impl.* which I haven't reviewed yet (I'm hoping to get a diff :) https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... components/translate/core/browser/translate_manager.cc:237: translate_ranker_->IsLoggingEnabled() || Should IsLoggingEnabled check QueryEnabled/EnforcementEnabled instead? To keep the policy decisions within ranker? https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... File components/translate/core/browser/translate_ranker_impl.cc (right): https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... components/translate/core/browser/translate_ranker_impl.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Can you convince codereview this is mostly a copy of translate_ranker.cc? (Same for the other ranker files). https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:102: std::unique_ptr<MockTranslateRanker> ranker_; Might want to add a comment to please not reorder, due to shutdown dependencies :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Task posting-related code lgtm. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:10: #include "base/files/file_path.h" Already included in the .h file. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:15: #include "base/memory/ref_counted.h" Already included in the .h file. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:116: // |internaL_on_model_available_cb_|. |is_finished| denotes whether the L -> l https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:123: // correct sequence. *same* sequence? https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... File components/translate/core/browser/ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:15: #include "base/threading/thread.h" Not needed https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:45: // may be called on any sequence and must, therefore, be thread-safe. ping https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... // This callback may be invoked after the RankerModelLoader has been destroyed and must, therefore, be careful with what memory it accesses. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:53: std::unique_ptr<chrome_intelligence::RankerModel> model)>; // This callback may be invoked after the RankerModelLoader has been destroyed and must, therefore, be careful with what memory it accesses. TranslateRankerImpl uses a weakptr to be "careful" about memory usage (i.e. to make sure that the callback does not do anything after it has been destroyed). https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:106: // Validates that ranker model loader tasks are all performed on the correct on the *same* sequence? https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... File components/translate/core/browser/ranker_model_loader_unittest.cc (right): https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:62: static bool IsEquivalent(const RankerModel& m1, const RankerModel& m2); REturns -> Returns
On 2017/02/23 16:41:26, fdoray wrote: > Task posting-related code lgtm. > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > File components/translate/core/browser/ranker_model_loader.cc (right): > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > components/translate/core/browser/ranker_model_loader.cc:10: #include > "base/files/file_path.h" > Already included in the .h file. > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > components/translate/core/browser/ranker_model_loader.cc:15: #include > "base/memory/ref_counted.h" > Already included in the .h file. > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > components/translate/core/browser/ranker_model_loader.cc:116: // > |internaL_on_model_available_cb_|. |is_finished| denotes whether the > L -> l > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > components/translate/core/browser/ranker_model_loader.cc:123: // correct > sequence. > *same* sequence? > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > File components/translate/core/browser/ranker_model_loader.h (right): > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > components/translate/core/browser/ranker_model_loader.h:15: #include > "base/threading/thread.h" > Not needed > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > components/translate/core/browser/ranker_model_loader.h:45: // may be called on > any sequence and must, therefore, be thread-safe. > ping > https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... > > // This callback may be invoked after the RankerModelLoader has been destroyed > and must, therefore, be careful with what memory it accesses. > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > components/translate/core/browser/ranker_model_loader.h:53: > std::unique_ptr<chrome_intelligence::RankerModel> model)>; > // This callback may be invoked after the RankerModelLoader has been destroyed > and must, therefore, be careful with what memory it accesses. > > TranslateRankerImpl uses a weakptr to be "careful" about memory usage (i.e. to > make sure that the callback does not do anything after it has been destroyed). > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > components/translate/core/browser/ranker_model_loader.h:106: // Validates that > ranker model loader tasks are all performed on the correct > on the *same* sequence? > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > File components/translate/core/browser/ranker_model_loader_unittest.cc (right): > > https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... > components/translate/core/browser/ranker_model_loader_unittest.cc:62: static > bool IsEquivalent(const RankerModel& m1, const RankerModel& m2); > REturns -> Returns translate/* LGTM
Patchset #10 (id:880001) has been deleted
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rogerm@chromium.org changed reviewers: + asanka@chromium.org, lpromero@chromium.org, michaeldo@chromium.org - gab@chromium.org
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 https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... components/translate/core/browser/translate_manager.cc:237: translate_ranker_->IsLoggingEnabled() || On 2017/02/23 00:01:35, groby wrote: > Should IsLoggingEnabled check QueryEnabled/EnforcementEnabled instead? To keep > the policy decisions within ranker? Logging has actually been enabled by default. http://crbug/668549 Cleanup of this feature flag has been waiting on this. I've got the CL ready to go once this lands. https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... File components/translate/core/browser/translate_ranker_impl.cc (right): https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... components/translate/core/browser/translate_ranker_impl.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/23 00:01:35, groby wrote: > Can you convince codereview this is mostly a copy of translate_ranker.cc? (Same > for the other ranker files). sent file specific diff out of band. https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/2565873002/diff/840001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:102: std::unique_ptr<MockTranslateRanker> ranker_; On 2017/02/23 00:01:35, groby wrote: > Might want to add a comment to please not reorder, due to shutdown dependencies > :) Indeed. Actually, I think they were in the wrong order (client_ depends on pref_service_). Fixed. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... File components/translate/core/browser/ranker_model_loader.cc (right): https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:10: #include "base/files/file_path.h" On 2017/02/23 16:41:25, fdoray wrote: > Already included in the .h file. Done. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:15: #include "base/memory/ref_counted.h" On 2017/02/23 16:41:25, fdoray wrote: > Already included in the .h file. Done. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:116: // |internaL_on_model_available_cb_|. |is_finished| denotes whether the On 2017/02/23 16:41:25, fdoray wrote: > L -> l Done. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.cc:123: // correct sequence. On 2017/02/23 16:41:25, fdoray wrote: > *same* sequence? Done. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... File components/translate/core/browser/ranker_model_loader.h (right): https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:15: #include "base/threading/thread.h" On 2017/02/23 16:41:25, fdoray wrote: > Not needed Done. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:45: // may be called on any sequence and must, therefore, be thread-safe. On 2017/02/23 16:41:25, fdoray wrote: > ping > https://codereview.chromium.org/2565873002/diff/780001/components/translate/c... > > // This callback may be invoked after the RankerModelLoader has been destroyed > and must, therefore, be careful with what memory it accesses. Done. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:53: std::unique_ptr<chrome_intelligence::RankerModel> model)>; On 2017/02/23 16:41:25, fdoray wrote: > // This callback may be invoked after the RankerModelLoader has been destroyed > and must, therefore, be careful with what memory it accesses. > > TranslateRankerImpl uses a weakptr to be "careful" about memory usage (i.e. to > make sure that the callback does not do anything after it has been destroyed). Done. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader.h:106: // Validates that ranker model loader tasks are all performed on the correct On 2017/02/23 16:41:25, fdoray wrote: > on the *same* sequence? Done. https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... File components/translate/core/browser/ranker_model_loader_unittest.cc (right): https://codereview.chromium.org/2565873002/diff/860001/components/translate/c... components/translate/core/browser/ranker_model_loader_unittest.cc:62: static bool IsEquivalent(const RankerModel& m1, const RankerModel& m2); On 2017/02/23 16:41:25, fdoray wrote: > REturns -> Returns Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2565873002/diff/900001/net/url_request/url_re... File net/url_request/url_request_context_getter.cc (right): https://codereview.chromium.org/2565873002/diff/900001/net/url_request/url_re... net/url_request/url_request_context_getter.cc:8: #include "base/debug/stack_trace.h" Unused
lgtm histogram lg
lgtm https://codereview.chromium.org/2565873002/diff/900001/components/translate/c... File components/translate/core/browser/translate_ranker.h (right): https://codereview.chromium.org/2565873002/diff/900001/components/translate/c... components/translate/core/browser/translate_ranker.h:36: // enforcement or logging are enabled, but can also be enabled independently. Logging is independent of querying (if not queries, we just log NOT_QUERIED as ranker_response
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Thanks https://codereview.chromium.org/2565873002/diff/900001/components/translate/c... File components/translate/core/browser/translate_ranker.h (right): https://codereview.chromium.org/2565873002/diff/900001/components/translate/c... components/translate/core/browser/translate_ranker.h:36: // enforcement or logging are enabled, but can also be enabled independently. On 2017/02/23 23:05:03, hamelphi wrote: > Logging is independent of querying (if not queries, we just log NOT_QUERIED as > ranker_response Done. https://codereview.chromium.org/2565873002/diff/900001/net/url_request/url_re... File net/url_request/url_request_context_getter.cc (right): https://codereview.chromium.org/2565873002/diff/900001/net/url_request/url_re... net/url_request/url_request_context_getter.cc:8: #include "base/debug/stack_trace.h" On 2017/02/23 22:29:07, asanka wrote: > Unused oops. remnants of debugging. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rogerm@chromium.org changed reviewers: - pasko@chromium.org
rogerm@chromium.org changed reviewers: + davidben@chromium.org
+davidben for net (asanka is OOO)
rogerm@chromium.org changed reviewers: + eugenebut@chromium.org - asanka@chromium.org, lpromero@chromium.org, michaeldo@chromium.org
+eugenebut for ios/*
eugenebut@chromium.org changed reviewers: + sdefresne@chromium.org
sdefresne@ would be a better reviewer for translate code
+sdefresne for ios/*
On 2017/02/24 23:00:40, Roger McFarlane (Chromium) wrote: > +sdefresne for ios/* The prefixes in ios/web_view have been updated, please update the name of CRIWVTranslateRankerFactory to WebViewTranslateRankerFactory. (I'll let sdefresne do the ios/ review) as I'm not familiar with translate.
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rogerm@chromium.org changed reviewers: + asanka@chromium.org, rsleevi@chromium.org
Ping looking for reviewers for ios/chrome/browser/metrics/* ios/web_view/internal/translate/* net/url_request/test_url_fetcher_factory.cc
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Patchset #12 (id:940001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... File ios/chrome/browser/translate/chrome_ios_translate_client.mm (right): https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... ios/chrome/browser/translate/chrome_ios_translate_client.mm:40: translate_manager_(new translate::TranslateManager( nit: please use base::MakeUnique<> here if possible https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... File ios/chrome/browser/translate/translate_ranker_factory.h (right): https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... ios/chrome/browser/translate/translate_ranker_factory.h:11: #include "base/memory/singleton.h" You can forward-declare the base::DefaultSingletonTraits template and move the #include to the implementation file: namespace base { template < typename T > struct DefaultSingletonTraits; } https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... File ios/chrome/browser/translate/translate_ranker_metrics_provider.cc (right): https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... ios/chrome/browser/translate/translate_ranker_metrics_provider.cc:26: TranslateRankerFactory::GetInstance()->GetForBrowserState(state); TranslateRankerFactory::GetForBrowserState() cannot return null (as the class does not override ServiceIsNULLWhileTesting, and uses the non-incognito BrowserState if the current BrowserState is incognito). So either remove the null pointer check or add, implement and call GetForBrowserStateIfExists().
Thanks! https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... File ios/chrome/browser/translate/chrome_ios_translate_client.mm (right): https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... ios/chrome/browser/translate/chrome_ios_translate_client.mm:40: translate_manager_(new translate::TranslateManager( On 2017/02/27 21:27:18, sdefresne wrote: > nit: please use base::MakeUnique<> here if possible Done. https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... File ios/chrome/browser/translate/translate_ranker_factory.h (right): https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... ios/chrome/browser/translate/translate_ranker_factory.h:11: #include "base/memory/singleton.h" On 2017/02/27 21:27:18, sdefresne wrote: > You can forward-declare the base::DefaultSingletonTraits template and move the > #include to the implementation file: > > namespace base { > template < typename T > > struct DefaultSingletonTraits; > } Done. https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... File ios/chrome/browser/translate/translate_ranker_metrics_provider.cc (right): https://codereview.chromium.org/2565873002/diff/920001/ios/chrome/browser/tra... ios/chrome/browser/translate/translate_ranker_metrics_provider.cc:26: TranslateRankerFactory::GetInstance()->GetForBrowserState(state); On 2017/02/27 21:27:18, sdefresne wrote: > TranslateRankerFactory::GetForBrowserState() cannot return null (as the class > does not override ServiceIsNULLWhileTesting, and uses the non-incognito > BrowserState if the current BrowserState is incognito). So either remove the > null pointer check or add, implement and call GetForBrowserStateIfExists(). Done (switched the check to a DCHECK).
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
/net/ lgtm
The CQ bit was unchecked by rogerm@chromium.org
The CQ bit was checked by rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org, fdoray@chromium.org, rkaplow@chromium.org, hamelphi@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2565873002/#ps980001 (title: "comments from sdefresne")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 980001, "attempt_start_ts": 1488233452770650, "parent_rev": "e853fed56dd2eee26d9ead959380c6e4d89a4c99", "commit_rev": "2233a4a7cb12ac4acdebd33491b528f8fb670f02"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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/+/2233a4a7cb12ac4acdebd33491b5... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:980001) as https://chromium.googlesource.com/chromium/src/+/2233a4a7cb12ac4acdebd33491b5... |