Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2913573002:
Updates language model on iOS. (Closed)
|
Created:
3 years, 6 months ago by ramyasharma Modified:
3 years, 6 months ago Reviewers:
droger, napper, pkl (ping after 24h if needed), jkrcal, michaeldo, martis, rohitrao (ping after 24h) CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), PL Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdates language model on iOS
This cl updates language model on Bling. And clears language model
history on clear browsing data.
This CL does not update language model from
ios/web_view/internal/translate/web_view_translate_client.
Mode details here: https://docs.google.com/document/d/17metf0LUD_p38DhPx7P_lyH9bVvRfuXlVvy91ESZT9M/edit
BUG=715447
Review-Url: https://codereview.chromium.org/2913573002
Cr-Commit-Position: refs/heads/master@{#477548}
Committed: https://chromium.googlesource.com/chromium/src/+/8709f78a5a70fd9db4414ca6c0fe281d668f31e5
Patch Set 1 #
Total comments: 14
Patch Set 2 #
Total comments: 3
Patch Set 3 #
Total comments: 4
Patch Set 4 #
Total comments: 4
Patch Set 5 #Messages
Total messages: 58 (42 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Updates language model on iOS. BUG=715447 ========== to ========== Updates language model on iOS This cl updates language model on Bling. And clears language model history on clear browsing data. This CL does not update language model from ios/web_view/internal/translate/web_view_translate_client. BUG=715447 ==========
Patchset #1 (id:60001) has been deleted
ramyasharma@chromium.org changed reviewers: + martis@chromium.org
Description was changed from ========== Updates language model on iOS This cl updates language model on Bling. And clears language model history on clear browsing data. This CL does not update language model from ios/web_view/internal/translate/web_view_translate_client. BUG=715447 ========== to ========== Updates language model on iOS This cl updates language model on Bling. And clears language model history on clear browsing data. This CL does not update language model from ios/web_view/internal/translate/web_view_translate_client. Mode details here: https://docs.google.com/document/d/17metf0LUD_p38DhPx7P_lyH9bVvRfuXlVvy91ESZT... BUG=715447 ==========
lgtm https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... File components/translate/ios/browser/language_detection_controller.h (right): https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... components/translate/ios/browser/language_detection_controller.h:37: struct DetectionDetails { Did you investigate reusing LanguageDetectionDetails from components/translate/core/common/language_detection_details.h? It might be too complex to unify the data types in this change, but we should keep the possibility in mind. https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... File components/translate/ios/browser/language_detection_controller.mm (right): https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... components/translate/ios/browser/language_detection_controller.mm:55: LanguageDetectionController::DetectionDetails::DetectionDetails() Do constructors/destructors operate in the same manner as C++? These three all match their implicit definitions; you could have "= default" for them all. https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... components/translate/ios/browser/language_detection_controller.mm:126: std::string cld_language; If you wanted to avoid the extra variables (and string copies), you could have: DetectionDetails details; details.content_language = ... details.html_root_language = ... details.adopted_language = translate::DeterminePageLanguage(..., &details.cld_language, &details.is_cld_reliable); https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/brow... File ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.mm (right): https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.mm:356: // Remove language model history. I believe the language model should only be cleared if the user requests a history clear (as opposed to e.g. a cookie clear); at least, this is the behavior for non-iOS Chrome. This logic should be moved to line 249. https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/pref... File ios/chrome/browser/prefs/browser_prefs.mm (right): https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/pref... ios/chrome/browser/prefs/browser_prefs.mm:116: translate::LanguageModel::RegisterProfilePrefs(registry); Nitpick: this list should be in alphabetical order (i.e. move this line up one). https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/tran... File ios/chrome/browser/translate/language_model_factory.cc (right): https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/tran... ios/chrome/browser/translate/language_model_factory.cc:22: ios::ChromeBrowserState* state) { In general, I think we should try to be as const-correct as possible. Up to you, but method parameters and instance variables in this cc file could be declared as e.g. ios::ChromeBrowserState* const state to stop you from accidentally overwriting them. Same comment for instance variables in this class' unit tests. https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/tran... File ios/chrome/browser/translate/language_model_factory.h (right): https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/tran... ios/chrome/browser/translate/language_model_factory.h:18: namespace translate { I think only components/translate/... code should be in the translate namespace. See e.g. ios/chrome/browser/translate/chrome_ios_translate_client.h.
ramyasharma@chromium.org changed reviewers: + napper@chromium.org
Patchset #2 (id:100001) has been deleted
The CQ bit was checked by ramyasharma@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-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...)
ramyasharma@chromium.org changed reviewers: + jkrcal@chromium.org
Thanks Michael. Adding Jan for review. https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... File components/translate/ios/browser/language_detection_controller.h (right): https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... components/translate/ios/browser/language_detection_controller.h:37: struct DetectionDetails { On 2017/06/01 06:49:21, martis wrote: > Did you investigate reusing LanguageDetectionDetails from > components/translate/core/common/language_detection_details.h? > > It might be too complex to unify the data types in this change, but we should > keep the possibility in mind. Yes, I did some basic investigation, and it looked too big a change for this CL. Maybe I can look at it in the next CL? Have added a TODO. https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... File components/translate/ios/browser/language_detection_controller.mm (right): https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... components/translate/ios/browser/language_detection_controller.mm:55: LanguageDetectionController::DetectionDetails::DetectionDetails() On 2017/06/01 06:49:21, martis wrote: > Do constructors/destructors operate in the same manner as C++? These three all > match their implicit definitions; you could have "= default" for them all. Yes, this is essentially C++ in objective-C++ file. I have an initialization variable (is_cld_reliable) in case of the constructor, can I still use default in that case? https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... components/translate/ios/browser/language_detection_controller.mm:126: std::string cld_language; On 2017/06/01 06:49:21, martis wrote: > If you wanted to avoid the extra variables (and string copies), you could have: > DetectionDetails details; > details.content_language = ... > details.html_root_language = ... > details.adopted_language = > translate::DeterminePageLanguage(..., &details.cld_language, > &details.is_cld_reliable); Thanks. But I think it's clearer this way. https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/brow... File ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.mm (right): https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.mm:356: // Remove language model history. On 2017/06/01 06:49:21, martis wrote: > I believe the language model should only be cleared if the user requests a > history clear (as opposed to e.g. a cookie clear); at least, this is the > behavior for non-iOS Chrome. > > This logic should be moved to line 249. Thanks, good catch. https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/pref... File ios/chrome/browser/prefs/browser_prefs.mm (right): https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/pref... ios/chrome/browser/prefs/browser_prefs.mm:116: translate::LanguageModel::RegisterProfilePrefs(registry); On 2017/06/01 06:49:21, martis wrote: > Nitpick: this list should be in alphabetical order (i.e. move this line up one). Done. https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/tran... File ios/chrome/browser/translate/language_model_factory.cc (right): https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/tran... ios/chrome/browser/translate/language_model_factory.cc:22: ios::ChromeBrowserState* state) { On 2017/06/01 06:49:21, martis wrote: > In general, I think we should try to be as const-correct as possible. > > Up to you, but method parameters and instance variables in this cc file could be > declared as e.g. > ios::ChromeBrowserState* const state > to stop you from accidentally overwriting them. > > Same comment for instance variables in this class' unit tests. Thanks Michael. I have changed it in here. I am not sure what you mean in the unit tests? https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/tran... File ios/chrome/browser/translate/language_model_factory.h (right): https://codereview.chromium.org/2913573002/diff/80001/ios/chrome/browser/tran... ios/chrome/browser/translate/language_model_factory.h:18: namespace translate { On 2017/06/01 06:49:21, martis wrote: > I think only components/translate/... code should be in the translate namespace. > See e.g. ios/chrome/browser/translate/chrome_ios_translate_client.h. Oops, yes. Thanks, fixed.
lgtm, thanks! https://codereview.chromium.org/2913573002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2913573002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:48: nil) {} nit: /*language_model=*/nil Why don't you feed the model from web_view? Is it simply consistent with other behaviour of webview and user expectations thereof? Could you also add a comment here explaining why "nil" is used?
https://codereview.chromium.org/2913573002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2913573002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:48: nil) {} On 2017/06/02 08:35:09, jkrcal wrote: > nit: /*language_model=*/nil > > Why don't you feed the model from web_view? Is it simply consistent with other > behaviour of webview and user expectations thereof? > > Could you also add a comment here explaining why "nil" is used? Actually, I know nothing about code style for .mm files. Take my nit merely as a comment that just "nil" is not very clear to me.
marq@chromium.org changed reviewers: + marq@chromium.org
+peterlaurens@ : FYI.
marq@chromium.org changed reviewers: - marq@chromium.org
ramyasharma@chromium.org changed reviewers: + droger@chromium.org, rohitrao@chromium.org
Adding owners to the review. droger@chromium.org: Please review changes in ios/ rohitrao@chromium.org: Please review changes in web_view https://codereview.chromium.org/2913573002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2913573002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:48: nil) {} On 2017/06/02 08:36:39, jkrcal wrote: > On 2017/06/02 08:35:09, jkrcal wrote: > > nit: /*language_model=*/nil > > > > Why don't you feed the model from web_view? Is it simply consistent with other > > behaviour of webview and user expectations thereof? > > > > Could you also add a comment here explaining why "nil" is used? > > Actually, I know nothing about code style for .mm files. Take my nit merely as a > comment that just "nil" is not very clear to me. Thanks Jan. The only reason I pass nil, is because 1. I don't know if this functionality needs to be part of webview 2. Even if it does, it was not straight forward, and needed some code duplication in webview (i am not sure if I depend on the factory class I introduced here). So if required I will add this in the next CL after I discuss it with the owners of webview.
The CQ bit was checked by ramyasharma@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...)
pkl@chromium.org changed reviewers: + michaeldo@chromium.org, pkl@chromium.org
+michaeldo for ios/web_view
ios/web_view changes look fine, but I'll let michaeldo sign off on them. I'm not sure what the language model is or what it's used for. michaeldo and eugenebut can help determine whether or not this is something that we should support in web_view. https://codereview.chromium.org/2913573002/diff/140001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2913573002/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:37: // TODO(ramyasharma): Investigate if language_model needs to be passed here Please file a bug for this and use TODO(crbug.com/NNNNNN). That will help make sure we don't forget to follow up.
ios/web_view lgtm with bug number added to TODO If I understand this change correctly, this change is to support histogram logging of the detected page language. ios/web_view doesn't support histogram logging yet, so I'm ok with the TODO. If possible, please explain in the TODO bug when we should support this inside ios/web_view if it is known. Or at least the repercussions of making this change. (e.g. support logging better histogram data of detected page language.) so that it can be correctly prioritized in the future. https://codereview.chromium.org/2913573002/diff/140001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2913573002/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:37: // TODO(ramyasharma): Investigate if language_model needs to be passed here On 2017/06/05 13:37:43, rohitrao (ping after 24h) wrote: > Please file a bug for this and use TODO(crbug.com/NNNNNN). That will help make > sure we don't forget to follow up. Agreed, we always use bug number references in ios/ to ensure we don't lose track of TODOs. Please add the "Mobile>iOSWebView" component to the bug you create.
On 2017/06/02 07:20:24, ramyasharma wrote: > https://codereview.chromium.org/2913573002/diff/80001/components/translate/io... > components/translate/ios/browser/language_detection_controller.mm:55: > LanguageDetectionController::DetectionDetails::DetectionDetails() > On 2017/06/01 06:49:21, martis wrote: > > Do constructors/destructors operate in the same manner as C++? These three all > > match their implicit definitions; you could have "= default" for them all. > > Yes, this is essentially C++ in objective-C++ file. > I have an initialization variable (is_cld_reliable) in case of the constructor, > can I still use default in that case? > Sorry, I missed that detail. It is correct to leave as-is.
The CQ bit was checked by ramyasharma@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 ramyasharma@chromium.org to run a CQ dry run
Patchset #4 (id:160001) 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 ramyasharma@chromium.org
Patchset #4 (id:180001) has been deleted
The CQ bit was checked by ramyasharma@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...
Thanks everyone. droger@ PTAL for owners approval in components/ and ios/ https://codereview.chromium.org/2913573002/diff/140001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2913573002/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:37: // TODO(ramyasharma): Investigate if language_model needs to be passed here On 2017/06/05 13:37:43, rohitrao (ping after 24h) wrote: > Please file a bug for this and use TODO(crbug.com/NNNNNN). That will help make > sure we don't forget to follow up. Done. https://codereview.chromium.org/2913573002/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:37: // TODO(ramyasharma): Investigate if language_model needs to be passed here On 2017/06/05 15:03:43, michaeldo wrote: > On 2017/06/05 13:37:43, rohitrao (ping after 24h) wrote: > > Please file a bug for this and use TODO(crbug.com/NNNNNN). That will help > make > > sure we don't forget to follow up. > > Agreed, we always use bug number references in ios/ to ensure we don't lose > track of TODOs. > > Please add the "Mobile>iOSWebView" component to the bug you create. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
lgtm with comments. https://codereview.chromium.org/2913573002/diff/200001/components/translate/i... File components/translate/ios/browser/BUILD.gn (right): https://codereview.chromium.org/2913573002/diff/200001/components/translate/i... components/translate/ios/browser/BUILD.gn:62: "//ios/chrome/browser/browser_state", Components cannot depend on chrome (the dependency is the other way around: chrome depends on components). It seems you don't really need this though, so just remove that diff. https://codereview.chromium.org/2913573002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2913573002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:51: nil /* language_model */) {} nullptr
The CQ bit was checked by ramyasharma@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...
Thanks David. https://codereview.chromium.org/2913573002/diff/200001/components/translate/i... File components/translate/ios/browser/BUILD.gn (right): https://codereview.chromium.org/2913573002/diff/200001/components/translate/i... components/translate/ios/browser/BUILD.gn:62: "//ios/chrome/browser/browser_state", On 2017/06/06 09:06:13, droger wrote: > Components cannot depend on chrome (the dependency is the other way around: > chrome depends on components). > > It seems you don't really need this though, so just remove that diff. Thanks for pointing this out, I had removed this, but i think i skipped it during rebase. https://codereview.chromium.org/2913573002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2913573002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:51: nil /* language_model */) {} On 2017/06/06 09:06:13, droger wrote: > nullptr Done.
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 ramyasharma@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from martis@chromium.org, jkrcal@chromium.org, michaeldo@chromium.org, droger@chromium.org Link to the patchset: https://codereview.chromium.org/2913573002/#ps220001 (title: "")
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": 220001, "attempt_start_ts": 1496809269646100,
"parent_rev": "db30f9ca562c571e7233710f0e9f69a882a7045e", "commit_rev":
"aae17f7f9d6adf84bc3a49f3a77c6954832c2c2a"}
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496809269646100,
"parent_rev": "0798d9b1c0023ab613e13091f2b7e7a10f5b780b", "commit_rev":
"8709f78a5a70fd9db4414ca6c0fe281d668f31e5"}
Message was sent while issue was closed.
Description was changed from ========== Updates language model on iOS This cl updates language model on Bling. And clears language model history on clear browsing data. This CL does not update language model from ios/web_view/internal/translate/web_view_translate_client. Mode details here: https://docs.google.com/document/d/17metf0LUD_p38DhPx7P_lyH9bVvRfuXlVvy91ESZT... BUG=715447 ========== to ========== Updates language model on iOS This cl updates language model on Bling. And clears language model history on clear browsing data. This CL does not update language model from ios/web_view/internal/translate/web_view_translate_client. Mode details here: https://docs.google.com/document/d/17metf0LUD_p38DhPx7P_lyH9bVvRfuXlVvy91ESZT... BUG=715447 Review-Url: https://codereview.chromium.org/2913573002 Cr-Commit-Position: refs/heads/master@{#477548} Committed: https://chromium.googlesource.com/chromium/src/+/8709f78a5a70fd9db4414ca6c0fe... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:220001) as https://chromium.googlesource.com/chromium/src/+/8709f78a5a70fd9db4414ca6c0fe... |
