Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2872083003:
Added translation policy API. (Closed)
|
Created:
3 years, 7 months ago by jzw1 Modified:
3 years, 6 months ago Reviewers:
napper, *groby-ooo-7-16, Eugene But (OOO till 7-30), sdefresne, michaeldo, Hiroshi Ichikawa CC:
chromium-reviews, ios-reviews+web_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded translation policy API. Translation policies are configured by
the user to decide how to translate certain languages.
This API is written to be purely Objective-C to hide C++ details from
the client app.
BUG=706289
Review-Url: https://codereview.chromium.org/2872083003
Cr-Commit-Position: refs/heads/master@{#475276}
Committed: https://chromium.googlesource.com/chromium/src/+/c21bd08f739ae50633429f5a15ac7f62ebcdea8e
Patch Set 1 #Patch Set 2 : refactored #Patch Set 3 : remove extra import #Patch Set 4 : added page host translation policies #
Total comments: 23
Patch Set 5 : Addressed ichikawa comments #
Total comments: 4
Patch Set 6 : addressed ichikawa comments part 2 #
Total comments: 2
Patch Set 7 : updated comments #
Total comments: 51
Patch Set 8 : addressed michael and eugene comments #
Total comments: 28
Patch Set 9 : addressed some more remaining comments #Patch Set 10 : don't expose index unnecessairly in translate_ui_delegate #
Total comments: 9
Patch Set 11 : addressed some final comments #
Total comments: 5
Patch Set 12 : Addressed sdefresne comments #Patch Set 13 : use prefs directly #
Total comments: 12
Patch Set 14 : refactored a bit #Patch Set 15 : update for loop to use reference #
Total comments: 2
Patch Set 16 : addressed eugene's comments #
Total comments: 8
Patch Set 17 : refactored supported languages to use a NSDictionary #Patch Set 18 : sync #
Messages
Total messages: 79 (18 generated)
jzw@chromium.org changed reviewers: + ichikawa@chromium.org
PTAL. First draft-ish.
ichikawa@chromium.org changed reviewers: + michaeldo@chromium.org
https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:194: if (languages_[i].first.compare(language_code) == 0) { How about simplifying this to languages_[i] == language_code? https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:114: void SetLanguageBlocked(bool value, const std::string& language_code); Optional: I slightly prefer the opposite order of parameters: language_code, value. I believe setter for key-value pairs are usually in (key, value) order. https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:134: bool ShouldAlwaysTranslate(const std::string& from_language, Is this method used? Looks sufficient with only GetAlwaysTranslateToLanguage(). https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:143: void SetAlwaysTranslate(bool value, This API looks a bit weird. This API seems to indicate that you can set a boolean value for each (from_language, to_language) pair. But it doesn't make sense to e.g., SetAlwaysTranslate(true, "en", "ja"); SetAlwaysTranslate(true, "en", "ko"); Instead, how about defining: void EnableToAlwaysTranslate(const std::string& from_language, const std::string& to_language); void DisableToAlwaysTranslate(const std::string& from_language); Or maybe only the former method, and let it disable always-translate when to_language is empty. https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:147: // Returns the language to always translate |from_language| to. Can you document the return value in case |from_language| is not set as a language to always translate? https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:174: false, languageCode, s/false/true/ ? https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:219: // TODO(706289): Support auto translation policies for websites. should be TODO(crbug.com/706289): ... Call NOTREACHED(); here so that the app crashes when this method is called with CWVTranslationPolicyAuto. https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:69: - (void)saveTranslationPolicy:(CWVTranslationPolicy*)policy 1. Maybe "set" not "save" to be consistent with standard naming convention of setters in Obj-C? 2. "language" is unclear, so how about "pageLanguage" instead? 3. Maybe add "for" to the selector to be consistent with -translationPolicyForLanguage? Putting all together, my suggestion is to change the selector to: -setTranslationPolicy:forPageLanguage: https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:75: - (void)saveTranslationPolicy:(CWVTranslationPolicy*)policy 1. Maybe "set" not "save" to be consistent with standard naming convention of setters in Obj-C? 2. Maybe add "for" to the selector to be consistent with -translationPolicyForLanguage? Putting all together, my suggestion is to change the selector to: -setTranslationPolicy:forPageHost: https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_policy.h:35: // Nil unless |type| is CWVTranslationPolicyAuto. Can you add a comment that this is the language it auto-translates to? Optional: Also, it may be clearer if the property name is targetLanguage instead?
https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:194: if (languages_[i].first.compare(language_code) == 0) { On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > How about simplifying this to languages_[i] == language_code? Wouldn't I need to do langauges_[i].first == language_code? updated the code https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:114: void SetLanguageBlocked(bool value, const std::string& language_code); On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > Optional: I slightly prefer the opposite order of parameters: language_code, > value. I believe setter for key-value pairs are usually in (key, value) order. I kind of like this order since I'm overloading the method above it. If someone else feels strongly about this I'll change it. https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:134: bool ShouldAlwaysTranslate(const std::string& from_language, On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > Is this method used? Looks sufficient with only GetAlwaysTranslateToLanguage(). Done. https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:143: void SetAlwaysTranslate(bool value, On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > This API looks a bit weird. This API seems to indicate that you can set a > boolean value for each (from_language, to_language) pair. But it doesn't make > sense to e.g., > > SetAlwaysTranslate(true, "en", "ja"); > SetAlwaysTranslate(true, "en", "ko"); > > Instead, how about defining: > > void EnableToAlwaysTranslate(const std::string& from_language, const > std::string& to_language); > void DisableToAlwaysTranslate(const std::string& from_language); > > Or maybe only the former method, and let it disable always-translate when > to_language is empty. Wouldn't you still be able to do something like: EnableAlwaysTranslate("en", "ja"); EnableAlwaysTranslate("en", "ko"); How about I add a comment that the last pair wins? https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:147: // Returns the language to always translate |from_language| to. On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > Can you document the return value in case |from_language| is not set as a > language to always translate? Done. https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:174: false, languageCode, On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > s/false/true/ ? Done. https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:219: // TODO(706289): Support auto translation policies for websites. On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > should be TODO(crbug.com/706289): ... > > Call NOTREACHED(); here so that the app crashes when this method is called with > CWVTranslationPolicyAuto. Done. https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:69: - (void)saveTranslationPolicy:(CWVTranslationPolicy*)policy On 2017/05/10 08:36:56, Hiroshi Ichikawa wrote: > 1. Maybe "set" not "save" to be consistent with standard naming convention of > setters in Obj-C? > 2. "language" is unclear, so how about "pageLanguage" instead? > 3. Maybe add "for" to the selector to be consistent with > -translationPolicyForLanguage? > > Putting all together, my suggestion is to change the selector to: > -setTranslationPolicy:forPageLanguage: Done. https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:75: - (void)saveTranslationPolicy:(CWVTranslationPolicy*)policy On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > 1. Maybe "set" not "save" to be consistent with standard naming convention of > setters in Obj-C? > 2. Maybe add "for" to the selector to be consistent with > -translationPolicyForLanguage? > > Putting all together, my suggestion is to change the selector to: > -setTranslationPolicy:forPageHost: Done. https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_policy.h:35: // Nil unless |type| is CWVTranslationPolicyAuto. On 2017/05/10 08:36:56, Hiroshi Ichikawa wrote: > Can you add a comment that this is the language it auto-translates to? > > Optional: Also, it may be clearer if the property name is targetLanguage > instead? I'll just add some cmoments.
https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:194: if (languages_[i].first.compare(language_code) == 0) { On 2017/05/11 03:03:17, jzw1 wrote: > On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > > How about simplifying this to languages_[i] == language_code? > > Wouldn't I need to do langauges_[i].first == language_code? updated the code Oops sorry, that's what I tried to mean. https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:143: void SetAlwaysTranslate(bool value, On 2017/05/11 03:03:17, jzw1 wrote: > On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > > This API looks a bit weird. This API seems to indicate that you can set a > > boolean value for each (from_language, to_language) pair. But it doesn't make > > sense to e.g., > > > > SetAlwaysTranslate(true, "en", "ja"); > > SetAlwaysTranslate(true, "en", "ko"); > > > > Instead, how about defining: > > > > void EnableToAlwaysTranslate(const std::string& from_language, const > > std::string& to_language); > > void DisableToAlwaysTranslate(const std::string& from_language); > > > > Or maybe only the former method, and let it disable always-translate when > > to_language is empty. > > Wouldn't you still be able to do something like: > EnableAlwaysTranslate("en", "ja"); > EnableAlwaysTranslate("en", "ko"); > > How about I add a comment that the last pair wins? My point is rather that having a boolean parameter for this method is wierd. If |value| is false, |to_language| is ignored, right? That's why I feel this API is more natural: void EnableToAlwaysTranslate(const std::string& from_language, const std::string& to_language); void DisableToAlwaysTranslate(const std::string& from_language); In this API, it's obvious that |to_language| is not necessary when you disable it. But if you document the behavior in detail, the current API would be acceptable too. https://codereview.chromium.org/2872083003/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:144: // string if none is found. s/if none is found/if it doesn't always translate |from_language|/ would be clearer. https://codereview.chromium.org/2872083003/diff/80001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/80001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:220: // TODO(crbug.com/706289): Support auto translation policies for websites. Nit: It may be better to put this comment before NOTREACHED().
https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:143: void SetAlwaysTranslate(bool value, On 2017/05/11 04:14:15, Hiroshi Ichikawa wrote: > On 2017/05/11 03:03:17, jzw1 wrote: > > On 2017/05/10 08:36:55, Hiroshi Ichikawa wrote: > > > This API looks a bit weird. This API seems to indicate that you can set a > > > boolean value for each (from_language, to_language) pair. But it doesn't > make > > > sense to e.g., > > > > > > SetAlwaysTranslate(true, "en", "ja"); > > > SetAlwaysTranslate(true, "en", "ko"); > > > > > > Instead, how about defining: > > > > > > void EnableToAlwaysTranslate(const std::string& from_language, const > > > std::string& to_language); > > > void DisableToAlwaysTranslate(const std::string& from_language); > > > > > > Or maybe only the former method, and let it disable always-translate when > > > to_language is empty. > > > > Wouldn't you still be able to do something like: > > EnableAlwaysTranslate("en", "ja"); > > EnableAlwaysTranslate("en", "ko"); > > > > How about I add a comment that the last pair wins? > > My point is rather that having a boolean parameter for this method is wierd. If > |value| is false, |to_language| is ignored, right? That's why I feel this API is > more natural: > > void EnableToAlwaysTranslate(const std::string& from_language, const > std::string& to_language); > void DisableToAlwaysTranslate(const std::string& from_language); > > In this API, it's obvious that |to_language| is not necessary when you disable > it. > > But if you document the behavior in detail, the current API would be acceptable > too. I'll just update the comment. https://codereview.chromium.org/2872083003/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:144: // string if none is found. On 2017/05/11 04:14:15, Hiroshi Ichikawa wrote: > s/if none is found/if it doesn't always translate |from_language|/ would be > clearer. I'll update the comment differently. https://codereview.chromium.org/2872083003/diff/80001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/80001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:220: // TODO(crbug.com/706289): Support auto translation policies for websites. On 2017/05/11 04:14:15, Hiroshi Ichikawa wrote: > Nit: It may be better to put this comment before NOTREACHED(). Done.
lgtm https://codereview.chromium.org/2872083003/diff/100001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:136: // Sets the value if the webpage in the |from_language| should be Optional: Thanks, it's clearer now. But I feel the comment is still a bit hard to understand. How about something like this, if you prefer: If |value| is true, update the preference to always translate |from_languge| to |to_language| without prompting. If it is called multiple times for the same |from_language|, the last specified |to_language| is used. If |value| is false, update the preference not to always translate |from_language|. |target_language| is ignored.
https://codereview.chromium.org/2872083003/diff/100001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:136: // Sets the value if the webpage in the |from_language| should be On 2017/05/11 04:59:44, Hiroshi Ichikawa wrote: > Optional: Thanks, it's clearer now. But I feel the comment is still a bit hard > to understand. How about something like this, if you prefer: > > If |value| is true, update the preference to always translate |from_languge| to > |to_language| without prompting. If it is called multiple times for the same > |from_language|, the last specified |to_language| is used. > > If |value| is false, update the preference not to always translate > |from_language|. |target_language| is ignored. Done.
jzw@chromium.org changed reviewers: + eugenebut@chromium.org
Generally LG. Have a few minor comments https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:193: for (size_t i = 0; i < languages_.size(); ++i) { Do you want to use std::find instead? https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:320: if (page_host.empty()) Should this be DCHECK instead? Is it reasonable for clients to pass an empty string? https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:108: bool IsLanguageBlocked(const std::string& language_code); Please document if strings can be empty and what happens if they are. https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:113: // Sets the value if the |language_code| is blocked. This method unconditionally sets the value, not only "if the |language_code| is blocked.". Right? How about "Blocks/Unblocks the translation for the language with |language_code| code" ? https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:125: // Sets the value if the |page_host| is blacklisted. ditto https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:136: // If |value| is true, update the preference to always translate s/update/updates https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:184: std::string autoTargetLanguageCode = Do we need this variable? This will not be used if |isLanguageBlocked| is false. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:231: } else { From Chromium Style Guide: "Don't use else after return." https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_policy.mm (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_policy.mm:20: policy->_type = CWVTranslationPolicyAsk; alloc/init can return null and this will crash. Do you want to add a private initWithPolicyType:language: init? https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:68: // Save or retrieve translation policies associated with a specified language. Please explain pageLanguage in the comments. Same for |pageHost|. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:68: // Save or retrieve translation policies associated with a specified language. /Save or retrieve/Sets or retrieves Per Style Guide: "Use descriptive form ("Opens the file") rather than imperative form ("Open the file") for method and function comments." Same for the other comment. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:16: CWVTranslationPolicyAsk, // Prompt user on whether or not to translate. nit: It's a good practice to initialize first enum item (in this case with 1, because 0 is default in Objective-C). https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:26: // Convenience constructors. Technically Objective-C does not have constructors. In the comments, please explain each convenience method (which type and language the resulting policy will have). https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:34: - (instancetype)init NS_UNAVAILABLE; https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:36: // Nil unless |type| is CWVTranslationPolicyAuto. nit: s/Nil/nil Uppercase Nil is a real thing which is used for classes.
Overall looks ok to me too. I think we should loop in droger@ (who owns //src/components/translate) sooner rather than later since there are quite a few changes to translate_ui_delegate. https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:81: size_t GetLanguageIndex(const std::string& language_code); Should this be avoided per comment above about crbug.com/555124? https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:186: bool isLanguageBlocked = use BOOL for obj-C https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:189: return [CWVTranslationPolicy translationPolicyNever]; I think the if/else can be cleaned up by early returning in the simple conditions for |isLanguageBlocked| and |autoTargetLanguageCode.empty()|. Then the remainder of the code can assume a non empty autoTargetLanguageCode. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:194: _translateUIDelegate->GetLanguageNameAt(autoTargetLanguageIndex); Can the language code be passed instead of the index? It seems like index based logic should be avoided per comment in translate_ui_delegate.h about crbug.com/555124 https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:227: bool IsSiteBlackListed = _translateUIDelegate->IsSiteBlacklisted( s/bool IsSiteBlackListed/BOOL isSiteBlackListed https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_policy.mm (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_policy.mm:26: policy->_type = CWVTranslationPolicyNever; Can you use synthesized setters instead? I don't know if this style is common. |setType:| will work if you re-defined property as readwrite in this file.
On 2017/05/11 23:51:34, michaeldo wrote: > Overall looks ok to me too. I think we should loop in droger@ (who owns > //src/components/translate) sooner rather than later since there are quite a few > changes to translate_ui_delegate. > > https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... > File components/translate/core/browser/translate_ui_delegate.h (right): > > https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... > components/translate/core/browser/translate_ui_delegate.h:81: size_t > GetLanguageIndex(const std::string& language_code); > Should this be avoided per comment above about crbug.com/555124? > > https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... > File ios/web_view/internal/translate/cwv_translation_controller.mm (right): > > https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller.mm:186: bool > isLanguageBlocked = > use BOOL for obj-C > > https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller.mm:189: return > [CWVTranslationPolicy translationPolicyNever]; > I think the if/else can be cleaned up by early returning in the simple > conditions for |isLanguageBlocked| and |autoTargetLanguageCode.empty()|. Then > the remainder of the code can assume a non empty autoTargetLanguageCode. > > https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller.mm:194: > _translateUIDelegate->GetLanguageNameAt(autoTargetLanguageIndex); > Can the language code be passed instead of the index? It seems like index based > logic should be avoided per comment in translate_ui_delegate.h about > crbug.com/555124 > > https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller.mm:227: bool > IsSiteBlackListed = _translateUIDelegate->IsSiteBlacklisted( > s/bool IsSiteBlackListed/BOOL isSiteBlackListed > > https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... > File ios/web_view/internal/translate/cwv_translation_policy.mm (right): > > https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_policy.mm:26: policy->_type = > CWVTranslationPolicyNever; > Can you use synthesized setters instead? I don't know if this style is common. > |setType:| will work if you re-defined property as readwrite in this file. Or maybe s/droger/sdefresne
https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:186: bool isLanguageBlocked = On 2017/05/11 23:51:33, michaeldo wrote: > use BOOL for obj-C Why? IsLanguageBlocked return bool, not BOOL https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:227: bool IsSiteBlackListed = _translateUIDelegate->IsSiteBlacklisted( On 2017/05/11 23:51:33, michaeldo wrote: > s/bool IsSiteBlackListed/BOOL isSiteBlackListed ditto
jzw@chromium.org changed reviewers: + droger@chromium.org
droger, could you PTAL? I made changes to translate_ui_delegate and I just want to make sure everything's OK. THanks. https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:193: for (size_t i = 0; i < languages_.size(); ++i) { On 2017/05/11 15:48:58, Eugene But wrote: > Do you want to use std::find instead? Can you check this for me? I'm not sure if it's 100% right. https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:320: if (page_host.empty()) On 2017/05/11 15:48:59, Eugene But wrote: > Should this be DCHECK instead? Is it reasonable for clients to pass an empty > string? I just adapted the original implementation, so I'm not sure. https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:81: size_t GetLanguageIndex(const std::string& language_code); On 2017/05/11 23:51:33, michaeldo wrote: > Should this be avoided per comment above about crbug.com/555124? I think it's mainly concerned with updating languages based on index, where as here I'm just reading the index. Should be safe! https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:108: bool IsLanguageBlocked(const std::string& language_code); On 2017/05/11 15:48:59, Eugene But wrote: > Please document if strings can be empty and what happens if they are. Do you mean string.empty() or null? I updated the comments. PTAL. https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:113: // Sets the value if the |language_code| is blocked. On 2017/05/11 15:48:59, Eugene But wrote: > This method unconditionally sets the value, not only "if the |language_code| is > blocked.". Right? > > How about "Blocks/Unblocks the translation for the language with |language_code| > code" ? Updated comments to be more clear. https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:125: // Sets the value if the |page_host| is blacklisted. On 2017/05/11 15:48:59, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:136: // If |value| is true, update the preference to always translate On 2017/05/11 15:48:59, Eugene But wrote: > s/update/updates Done and did same for below. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:184: std::string autoTargetLanguageCode = On 2017/05/11 15:48:59, Eugene But wrote: > Do we need this variable? This will not be used if |isLanguageBlocked| is false. Done. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:186: bool isLanguageBlocked = On 2017/05/12 00:31:40, Eugene But wrote: > On 2017/05/11 23:51:33, michaeldo wrote: > > use BOOL for obj-C > Why? IsLanguageBlocked return bool, not BOOL yeah I think bool is ok here. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:189: return [CWVTranslationPolicy translationPolicyNever]; On 2017/05/11 23:51:33, michaeldo wrote: > I think the if/else can be cleaned up by early returning in the simple > conditions for |isLanguageBlocked| and |autoTargetLanguageCode.empty()|. Then > the remainder of the code can assume a non empty autoTargetLanguageCode. Done. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:194: _translateUIDelegate->GetLanguageNameAt(autoTargetLanguageIndex); On 2017/05/11 23:51:33, michaeldo wrote: > Can the language code be passed instead of the index? It seems like index based > logic should be avoided per comment in translate_ui_delegate.h about > crbug.com/555124 I believe it just doesn't want you to update things via index. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:227: bool IsSiteBlackListed = _translateUIDelegate->IsSiteBlacklisted( On 2017/05/12 00:31:39, Eugene But wrote: > On 2017/05/11 23:51:33, michaeldo wrote: > > s/bool IsSiteBlackListed/BOOL isSiteBlackListed > > ditto Done. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:231: } else { On 2017/05/11 15:48:59, Eugene But wrote: > From Chromium Style Guide: "Don't use else after return." > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_policy.mm (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_policy.mm:20: policy->_type = CWVTranslationPolicyAsk; On 2017/05/11 15:48:59, Eugene But wrote: > alloc/init can return null and this will crash. Do you want to add a private > initWithPolicyType:language: init? Done. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_policy.mm:26: policy->_type = CWVTranslationPolicyNever; On 2017/05/11 23:51:33, michaeldo wrote: > Can you use synthesized setters instead? I don't know if this style is common. > |setType:| will work if you re-defined property as readwrite in this file. Done. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:68: // Save or retrieve translation policies associated with a specified language. On 2017/05/11 15:48:59, Eugene But wrote: > /Save or retrieve/Sets or retrieves > > Per Style Guide: "Use descriptive form ("Opens the file") rather than imperative > form ("Open the file") for method and function comments." > Same for the other comment. Done. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:16: CWVTranslationPolicyAsk, // Prompt user on whether or not to translate. On 2017/05/11 15:48:59, Eugene But wrote: > nit: It's a good practice to initialize first enum item (in this case with 1, > because 0 is default in Objective-C). Done. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:26: // Convenience constructors. On 2017/05/11 15:48:59, Eugene But wrote: > Technically Objective-C does not have constructors. In the comments, please > explain each convenience method (which type and language the resulting policy > will have). Done. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:34: On 2017/05/11 15:48:59, Eugene But wrote: > - (instancetype)init NS_UNAVAILABLE; Is there a way to have this and still allow the implementation file to call init? https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:36: // Nil unless |type| is CWVTranslationPolicyAuto. On 2017/05/11 15:48:59, Eugene But wrote: > nit: s/Nil/nil > > Uppercase Nil is a real thing which is used for classes. Done.
https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:108: bool IsLanguageBlocked(const std::string& language_code); On 2017/05/12 03:46:45, jzw1 wrote: > On 2017/05/11 15:48:59, Eugene But wrote: > > Please document if strings can be empty and what happens if they are. > > Do you mean string.empty() or null? I updated the comments. PTAL. I believe he means string.empty(). It will be a compiler error if you pass nullptr to const std::string&.
lgtm https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/120001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:320: if (page_host.empty()) On 2017/05/12 03:46:45, jzw1 wrote: > On 2017/05/11 15:48:59, Eugene But wrote: > > Should this be DCHECK instead? Is it reasonable for clients to pass an empty > > string? > > I just adapted the original implementation, so I'm not sure. What would be the value to accept empty strings? The downside is potentially hiding bugs. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:36: // Nil unless |type| is CWVTranslationPolicyAuto. On 2017/05/12 03:46:47, jzw1 wrote: > On 2017/05/11 15:48:59, Eugene But wrote: > > nit: s/Nil/nil > > > > Uppercase Nil is a real thing which is used for classes. > > Done. FYI: There is still uppercase Nil https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:193: auto result = std::find_if(languages_.begin(), languages_.end(), auto it = languages_.find(language_code); return (it != languages_.end()) ? result - languages_.begin() : kNoIndex; Per Chromium Style Guide, no else after return https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:323: if (page_host.empty()) Comments says that empty host is not allowed, but this code accepts and ignores it. https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:353: } else { Per Style Guide: no else after return please https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:69: // |pageLanguage| should be the language code of the language. Do you want to use nullable annotations? Please document the behavior for empty host and null page language (if allowed). https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:26: // Creates a policy with CWVTranslationPolicyAsk. "and null language"? https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:28: // Creates a policy with CWVTranslationPolicyNever. ditto https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:30: // Creates a policy with CWVTranslationPolicyAuto with language. s/with/and the given ?
https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:194: _translateUIDelegate->GetLanguageNameAt(autoTargetLanguageIndex); On 2017/05/12 03:46:46, jzw1 wrote: > On 2017/05/11 23:51:33, michaeldo wrote: > > Can the language code be passed instead of the index? It seems like index > based > > logic should be avoided per comment in translate_ui_delegate.h about > > crbug.com/555124 > > I believe it just doesn't want you to update things via index. I think you're right, but it still would keep the API cleaner and prevent bad behavior by others in the future to use a method like |GetLanguageNameForLanguageCode|. I don't think the index should be exposed unless completely necessary. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:34: On 2017/05/12 03:46:47, jzw1 wrote: > On 2017/05/11 15:48:59, Eugene But wrote: > > - (instancetype)init NS_UNAVAILABLE; > > Is there a way to have this and still allow the implementation file to call > init? It should be defined here as unavailable. Usually we have an initializer that takes a param defined elsewhere in an _internal header (if other classes need to internally allocate it) or in the implementation file if no classes need it. You can define |initWithType| inside |cwv_translation_policy.mm| https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:323: if (page_host.empty()) On 2017/05/12 14:39:19, Eugene But wrote: > Comments says that empty host is not allowed, but this code accepts and ignores > it. To retain original behavior, you can move this early return to |SetSiteBlacklist(bool value)| method. Then DCHECK in your |SetSiteBlacklist(bool value, const std::string& page_host)| method. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:192: if (!autoTargetLanguageCode.empty()) { I also recommend removing the ! from this if and return [CWVTranslationPolicy translationPolicyAsk] early. Then you don't need to nest the remainder of the method. if (autoTargetLanguageCode.empty()) { return [CWVTranslationPolicy translationPolicyAsk]; } size_t autoTargetLanguageIndex = ... return [CWVTranslationPolicy translationPolicyAutoTranslateToLanguage:targetLanguage]; https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:229: bool IsSiteBlackListed = _translateUIDelegate->IsSiteBlacklisted( sorry about that, bool does seem fine here, but variable name should still start with lowercase letter to be consistent. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_policy.mm (right): https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_policy.mm:16: @property(nonatomic, strong) CWVTranslationLanguage* language; please explicitly specify "readwrite" here https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:38: // Nil unless |type| is CWVTranslationPolicyAuto. s/Nil/nil
https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:34: On 2017/05/12 15:29:08, michaeldo wrote: > On 2017/05/12 03:46:47, jzw1 wrote: > > On 2017/05/11 15:48:59, Eugene But wrote: > > > - (instancetype)init NS_UNAVAILABLE; > > > > Is there a way to have this and still allow the implementation file to call > > init? > > It should be defined here as unavailable. Usually we have an initializer that > takes a param defined elsewhere in an _internal header (if other classes need to > internally allocate it) or in the implementation file if no classes need it. You > can define |initWithType| inside |cwv_translation_policy.mm| Sounds good. Thanks. https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:36: // Nil unless |type| is CWVTranslationPolicyAuto. On 2017/05/12 14:39:19, Eugene But wrote: > On 2017/05/12 03:46:47, jzw1 wrote: > > On 2017/05/11 15:48:59, Eugene But wrote: > > > nit: s/Nil/nil > > > > > > Uppercase Nil is a real thing which is used for classes. > > > > Done. > > FYI: There is still uppercase Nil Done. https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:193: auto result = std::find_if(languages_.begin(), languages_.end(), On 2017/05/12 14:39:19, Eugene But wrote: > auto it = languages_.find(language_code); > return (it != languages_.end()) ? result - languages_.begin() : kNoIndex; > > Per Chromium Style Guide, no else after return > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Will .find be able to do it? Think I need to keep my find_if to check against the contents of the language pair right? https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:323: if (page_host.empty()) On 2017/05/12 15:29:09, michaeldo wrote: > On 2017/05/12 14:39:19, Eugene But wrote: > > Comments says that empty host is not allowed, but this code accepts and > ignores > > it. > > To retain original behavior, you can move this early return to > |SetSiteBlacklist(bool value)| method. Then DCHECK in your > |SetSiteBlacklist(bool value, const std::string& page_host)| method. Should I just DCHECK(!page_host.empty())? https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:353: } else { On 2017/05/12 14:39:19, Eugene But wrote: > Per Style Guide: no else after return please Done. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:192: if (!autoTargetLanguageCode.empty()) { On 2017/05/12 15:29:09, michaeldo wrote: > I also recommend removing the ! from this if and return [CWVTranslationPolicy > translationPolicyAsk] early. Then you don't need to nest the remainder of the > method. > > if (autoTargetLanguageCode.empty()) { > return [CWVTranslationPolicy translationPolicyAsk]; > } > > size_t autoTargetLanguageIndex = > ... > return [CWVTranslationPolicy > translationPolicyAutoTranslateToLanguage:targetLanguage]; Done. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:229: bool IsSiteBlackListed = _translateUIDelegate->IsSiteBlacklisted( On 2017/05/12 15:29:09, michaeldo wrote: > sorry about that, bool does seem fine here, but variable name should still start > with lowercase letter to be consistent. Ah nice catch. Thakns. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_policy.mm (right): https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_policy.mm:16: @property(nonatomic, strong) CWVTranslationLanguage* language; On 2017/05/12 15:29:09, michaeldo wrote: > please explicitly specify "readwrite" here Done. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:69: // |pageLanguage| should be the language code of the language. On 2017/05/12 14:39:20, Eugene But wrote: > Do you want to use nullable annotations? Please document the behavior for empty > host and null page language (if allowed). The entire thing is wrapped in assume nonnull, which works for us. I added that pageHost below cannot be empty, also added a DCHECK for it. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:26: // Creates a policy with CWVTranslationPolicyAsk. On 2017/05/12 14:39:20, Eugene But wrote: > "and null language"? Done. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:28: // Creates a policy with CWVTranslationPolicyNever. On 2017/05/12 14:39:20, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:30: // Creates a policy with CWVTranslationPolicyAuto with language. On 2017/05/12 14:39:20, Eugene But wrote: > s/with/and the given ? Done. https://codereview.chromium.org/2872083003/diff/140001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:38: // Nil unless |type| is CWVTranslationPolicyAuto. On 2017/05/12 15:29:09, michaeldo wrote: > s/Nil/nil Done.
https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:193: auto result = std::find_if(languages_.begin(), languages_.end(), On 2017/05/15 02:43:28, jzw1 wrote: > On 2017/05/12 14:39:19, Eugene But wrote: > > auto it = languages_.find(language_code); > > return (it != languages_.end()) ? result - languages_.begin() : kNoIndex; > > > > Per Chromium Style Guide, no else after return > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Will .find be able to do it? Think I need to keep my find_if to check against > the contents of the language pair right? +1 to John. Note that it's a comparison against x.first, not x.
https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:194: _translateUIDelegate->GetLanguageNameAt(autoTargetLanguageIndex); On 2017/05/12 15:29:08, michaeldo wrote: > On 2017/05/12 03:46:46, jzw1 wrote: > > On 2017/05/11 23:51:33, michaeldo wrote: > > > Can the language code be passed instead of the index? It seems like index > > based > > > logic should be avoided per comment in translate_ui_delegate.h about > > > crbug.com/555124 > > > > I believe it just doesn't want you to update things via index. > > I think you're right, but it still would keep the API cleaner and prevent bad > behavior by others in the future to use a method like > |GetLanguageNameForLanguageCode|. I don't think the index should be exposed > unless completely necessary. that's fair. I'll just make it GetLanguageNameForLanguageCode.
https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:193: auto result = std::find_if(languages_.begin(), languages_.end(), On 2017/05/15 03:25:34, Hiroshi Ichikawa wrote: > On 2017/05/15 02:43:28, jzw1 wrote: > > On 2017/05/12 14:39:19, Eugene But wrote: > > > auto it = languages_.find(language_code); > > > return (it != languages_.end()) ? result - languages_.begin() : kNoIndex; > > > > > > Per Chromium Style Guide, no else after return > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > Will .find be able to do it? Think I need to keep my find_if to check against > > the contents of the language pair right? > > +1 to John. Note that it's a comparison against x.first, not x. map::iterator::fist is a key in std::map. map::find searches for a key and returns map::iterator: http://www.cplusplus.com/reference/map/map/find/ What is the advantage of std::find_if over std::map::find when you searching by key, not by value?
lgtm! Thank you! Please ensure the original early return is replaced inside |TranslateUIDelegate::SetSiteBlacklist(bool value)| https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:323: if (page_host.empty()) On 2017/05/15 02:43:28, jzw1 wrote: > On 2017/05/12 15:29:09, michaeldo wrote: > > On 2017/05/12 14:39:19, Eugene But wrote: > > > Comments says that empty host is not allowed, but this code accepts and > > ignores > > > it. > > > > To retain original behavior, you can move this early return to > > |SetSiteBlacklist(bool value)| method. Then DCHECK in your > > |SetSiteBlacklist(bool value, const std::string& page_host)| method. > > Should I just DCHECK(!page_host.empty())? yes, that is fine here. https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:316: SetSiteBlacklist(value, GetPageHost()); Since this is an existing method, you should leave the early return inside |SetSetBlacklist|. std::string host = GetPageHost(); if (host.empty()) return; https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:28: // Creates a policy with CWVTranslationPolicyAsk and nil language. Please use "null" in comments. https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:30: // Creates a policy with CWVTranslationPolicyNever nil language. missing "and"? Please also use "null" here. https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:32: // Creates a policy with CWVTranslationPolicyAuto with the given language. s/with/and ?
https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:316: SetSiteBlacklist(value, GetPageHost()); On 2017/05/15 17:09:30, michaeldo wrote: > Since this is an existing method, you should leave the early return inside > |SetSetBlacklist|. > > std::string host = GetPageHost(); > if (host.empty()) > return; Early return would be against Chromium Style Guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... It should be either DCHECK or early return and DCHECK seem to follow API contract.
On 2017/05/15 17:22:41, Eugene But wrote: > https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... > File components/translate/core/browser/translate_ui_delegate.cc (right): > > https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... > components/translate/core/browser/translate_ui_delegate.cc:316: > SetSiteBlacklist(value, GetPageHost()); > On 2017/05/15 17:09:30, michaeldo wrote: > > Since this is an existing method, you should leave the early return inside > > |SetSetBlacklist|. > > > > std::string host = GetPageHost(); > > if (host.empty()) > > return; > Early return would be against Chromium Style Guide: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > It should be either DCHECK or early return and DCHECK seem to follow API > contract. I don't believe that is true. If I understand correctly, the early return should exist inside |void SetSiteBlacklist(bool value);| because it pulls the host string from GetPageHost() which is documented as returning "an empty string if no URL is associated with the current page." If we DCHECK, we are changing current behavior and that should at least happen in a separate CL so this doesn't get reverted in case it causes issues. The documentation on SetSiteBlacklist(bool value) would also need to state that the page host must be set which it does not currently specify.
On 2017/05/15 20:12:58, michaeldo wrote: > On 2017/05/15 17:22:41, Eugene But wrote: > > > https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... > > File components/translate/core/browser/translate_ui_delegate.cc (right): > > > > > https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... > > components/translate/core/browser/translate_ui_delegate.cc:316: > > SetSiteBlacklist(value, GetPageHost()); > > On 2017/05/15 17:09:30, michaeldo wrote: > > > Since this is an existing method, you should leave the early return inside > > > |SetSetBlacklist|. > > > > > > std::string host = GetPageHost(); > > > if (host.empty()) > > > return; > > Early return would be against Chromium Style Guide: > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > It should be either DCHECK or early return and DCHECK seem to follow API > > contract. > > I don't believe that is true. If I understand correctly, the early return should > exist inside |void SetSiteBlacklist(bool value);| because it pulls the host > string from GetPageHost() which is documented as returning "an empty string if > no URL is associated with the current page." If we DCHECK, we are changing > current behavior and that should at least happen in a separate CL so this > doesn't get reverted in case it causes issues. The documentation on > SetSiteBlacklist(bool value) would also need to state that the page host must be > set which it does not currently specify. Makes sense. Change behavior in a separate CL sounds reasonable. Thanks!
https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:316: SetSiteBlacklist(value, GetPageHost()); On 2017/05/15 17:22:41, Eugene But wrote: > On 2017/05/15 17:09:30, michaeldo wrote: > > Since this is an existing method, you should leave the early return inside > > |SetSetBlacklist|. > > > > std::string host = GetPageHost(); > > if (host.empty()) > > return; > Early return would be against Chromium Style Guide: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > It should be either DCHECK or early return and DCHECK seem to follow API > contract. I'm going to just keep the early returns to keep the behavior unchanged. https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:28: // Creates a policy with CWVTranslationPolicyAsk and nil language. On 2017/05/15 17:09:30, michaeldo wrote: > Please use "null" in comments. Done. https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:30: // Creates a policy with CWVTranslationPolicyNever nil language. On 2017/05/15 17:09:30, michaeldo wrote: > missing "and"? Please also use "null" here. Done. https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:32: // Creates a policy with CWVTranslationPolicyAuto with the given language. On 2017/05/15 17:09:30, michaeldo wrote: > s/with/and ? Done.
On 2017/05/16 03:42:13, jzw1 wrote: > https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... > File components/translate/core/browser/translate_ui_delegate.cc (right): > > https://codereview.chromium.org/2872083003/diff/180001/components/translate/c... > components/translate/core/browser/translate_ui_delegate.cc:316: > SetSiteBlacklist(value, GetPageHost()); > On 2017/05/15 17:22:41, Eugene But wrote: > > On 2017/05/15 17:09:30, michaeldo wrote: > > > Since this is an existing method, you should leave the early return inside > > > |SetSetBlacklist|. > > > > > > std::string host = GetPageHost(); > > > if (host.empty()) > > > return; > > Early return would be against Chromium Style Guide: > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > It should be either DCHECK or early return and DCHECK seem to follow API > > contract. > > I'm going to just keep the early returns to keep the behavior unchanged. > > https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... > File ios/web_view/public/cwv_translation_policy.h (right): > > https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... > ios/web_view/public/cwv_translation_policy.h:28: // Creates a policy with > CWVTranslationPolicyAsk and nil language. > On 2017/05/15 17:09:30, michaeldo wrote: > > Please use "null" in comments. > > Done. > > https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... > ios/web_view/public/cwv_translation_policy.h:30: // Creates a policy with > CWVTranslationPolicyNever nil language. > On 2017/05/15 17:09:30, michaeldo wrote: > > missing "and"? Please also use "null" here. > > Done. > > https://codereview.chromium.org/2872083003/diff/180001/ios/web_view/public/cw... > ios/web_view/public/cwv_translation_policy.h:32: // Creates a policy with > CWVTranslationPolicyAuto with the given language. > On 2017/05/15 17:09:30, michaeldo wrote: > > s/with/and ? > > Done. Hey droger@, would you have the time to do a pass on this CL?
https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:193: auto result = std::find_if(languages_.begin(), languages_.end(), On 2017/05/15 14:36:14, Eugene But wrote: > On 2017/05/15 03:25:34, Hiroshi Ichikawa wrote: > > On 2017/05/15 02:43:28, jzw1 wrote: > > > On 2017/05/12 14:39:19, Eugene But wrote: > > > > auto it = languages_.find(language_code); > > > > return (it != languages_.end()) ? result - languages_.begin() : kNoIndex; > > > > > > > > Per Chromium Style Guide, no else after return > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > > > Will .find be able to do it? Think I need to keep my find_if to check > against > > > the contents of the language pair right? > > > > +1 to John. Note that it's a comparison against x.first, not x. > map::iterator::fist is a key in std::map. map::find searches for a key and > returns map::iterator: http://www.cplusplus.com/reference/map/map/find/ > > What is the advantage of std::find_if over std::map::find when you searching by > key, not by value? > > |languages_| is a vector, not a map. So you cannot use map::find. I believe it's a vector because it must be also accessed by an index?
https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:193: auto result = std::find_if(languages_.begin(), languages_.end(), On 2017/05/16 04:30:55, Hiroshi Ichikawa wrote: > On 2017/05/15 14:36:14, Eugene But wrote: > > On 2017/05/15 03:25:34, Hiroshi Ichikawa wrote: > > > On 2017/05/15 02:43:28, jzw1 wrote: > > > > On 2017/05/12 14:39:19, Eugene But wrote: > > > > > auto it = languages_.find(language_code); > > > > > return (it != languages_.end()) ? result - languages_.begin() : > kNoIndex; > > > > > > > > > > Per Chromium Style Guide, no else after return > > > > > > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > > > > > Will .find be able to do it? Think I need to keep my find_if to check > > against > > > > the contents of the language pair right? > > > > > > +1 to John. Note that it's a comparison against x.first, not x. > > map::iterator::fist is a key in std::map. map::find searches for a key and > > returns map::iterator: http://www.cplusplus.com/reference/map/map/find/ > > > > What is the advantage of std::find_if over std::map::find when you searching > by > > key, not by value? > > > > > > |languages_| is a vector, not a map. So you cannot use map::find. I believe it's > a vector because it must be also accessed by an index? Sorry, if it's a vector, then |std::find(languages_.begin(), languages_.end(), language_code);| should work.
On 2017/05/16 15:28:38, Eugene But wrote: > https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... > File components/translate/core/browser/translate_ui_delegate.cc (right): > > https://codereview.chromium.org/2872083003/diff/140001/components/translate/c... > components/translate/core/browser/translate_ui_delegate.cc:193: auto result = > std::find_if(languages_.begin(), languages_.end(), > On 2017/05/16 04:30:55, Hiroshi Ichikawa wrote: > > On 2017/05/15 14:36:14, Eugene But wrote: > > > On 2017/05/15 03:25:34, Hiroshi Ichikawa wrote: > > > > On 2017/05/15 02:43:28, jzw1 wrote: > > > > > On 2017/05/12 14:39:19, Eugene But wrote: > > > > > > auto it = languages_.find(language_code); > > > > > > return (it != languages_.end()) ? result - languages_.begin() : > > kNoIndex; > > > > > > > > > > > > Per Chromium Style Guide, no else after return > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > > > > > > > Will .find be able to do it? Think I need to keep my find_if to check > > > against > > > > > the contents of the language pair right? > > > > > > > > +1 to John. Note that it's a comparison against x.first, not x. > > > map::iterator::fist is a key in std::map. map::find searches for a key and > > > returns map::iterator: http://www.cplusplus.com/reference/map/map/find/ > > > > > > What is the advantage of std::find_if over std::map::find when you searching > > by > > > key, not by value? > > > > > > > > > > |languages_| is a vector, not a map. So you cannot use map::find. I believe > it's > > a vector because it must be also accessed by an index? > Sorry, if it's a vector, then |std::find(languages_.begin(), languages_.end(), > language_code);| should work. but each element inside that vector is a std::pair, which cannot be compared directly to a std::string right?
jzw@chromium.org changed reviewers: + sdefresne@chromium.org
sdefresne@, PTAL whenever you have time. n particular, I want to make sure to get approvals for changes in the translate component (translate_ui_delegate.h/cc).
Description was changed from ========== Added translation policy API. BUG=706289 ========== to ========== Added translation policy API. BUG=706289 ==========
sdefresne@chromium.org changed reviewers: + groby@chromium.org - droger@chromium.org
sdefresne@chromium.org changed required reviewers: + groby@chromium.org
-droger (OOO till end of May)/+groby: could you review components/translate as OWNERS lgtm with some small comments https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:164: _translateUIDelegate->SetAlwaysTranslate(false, languageCode, The documentation says that target_language is ignored if |value| is false [1], so there is no need to query and pass autoTargetLanguageCode here, so you can just do the following: _translateUIDelegate->SetLanguageBlocked(false, languageCode); _translateUIDelegate->SetAlwaysTranslate(false, languageCode, std::string()); Or if the target_language is not ignored, then the documentation must be updated. [1]: // If |value| is false, updates the preference not to always translate // |from_language|. |target_language| is ignored. https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| is CWVTranslationPolicyAuto. null -> Nil (since it is a Objective-C type). If you want to avoid capitalizing the N, then you can reformulate this to // Indicates the target language to automatically translate to. Only defined // if |type| is CWVTranslationPolicyAuto.
I forgot to mention, but in Chrome, policy usually design things controlled by domain administrator for managed installation (enterprises, schools, ...). Maybe you want to change your CL description to avoid the term or expand the description to explain those are not policy controlled by an administrator but settings configured by the local user.
> but each element inside that vector is a std::pair, which cannot be compared > directly to a std::string right? I'm really sorry. Apparently I'm blind or something...
https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| is CWVTranslationPolicyAuto. On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > null -> Nil (since it is a Objective-C type). > > If you want to avoid capitalizing the N, then you can reformulate this to > > // Indicates the target language to automatically translate to. Only defined > // if |type| is CWVTranslationPolicyAuto. eugenebut@ said he perfers nil to Nil because Nil means a dfferent thing in Objective-C. I personally agree with him to prefer Objective-C correctness to English correctness here, but if you do care, your proposed refinement sounds good. Or maybe just add "It is": // It is nil unless |type| is CWVTranslationPolicyAuto.
On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > File ios/web_view/public/cwv_translation_policy.h (right): > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| is > CWVTranslationPolicyAuto. > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > null -> Nil (since it is a Objective-C type). > > > > If you want to avoid capitalizing the N, then you can reformulate this to > > > > // Indicates the target language to automatically translate to. Only defined > > // if |type| is CWVTranslationPolicyAuto. > > eugenebut@ said he perfers nil to Nil because Nil means a dfferent thing in > Objective-C. > I personally agree with him to prefer Objective-C correctness to English > correctness here, but if you do care, your proposed refinement sounds good. Or > maybe just add "It is": > > // It is nil unless |type| is CWVTranslationPolicyAuto. I'm currently not clear why we need a policy API - the bug doesn't seem to say - or why the delegate functions need to be parametrized. Could I ask you to explain what motivates the changes?
On 2017/05/18 05:20:09, groby wrote: > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| is > > CWVTranslationPolicyAuto. > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > null -> Nil (since it is a Objective-C type). > > > > > > If you want to avoid capitalizing the N, then you can reformulate this to > > > > > > // Indicates the target language to automatically translate to. Only defined > > > // if |type| is CWVTranslationPolicyAuto. > > > > eugenebut@ said he perfers nil to Nil because Nil means a dfferent thing in > > Objective-C. > > I personally agree with him to prefer Objective-C correctness to English > > correctness here, but if you do care, your proposed refinement sounds good. Or > > maybe just add "It is": > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > I'm currently not clear why we need a policy API - the bug doesn't seem to say - > or why the delegate functions need to be parametrized. Could I ask you to > explain what motivates the changes? I'll update the description
Description was changed from ========== Added translation policy API. BUG=706289 ========== to ========== Added translation policy API. Translation policies are configured by the user to decide how to translate certain languages. This API is written to be purely Objective-C to hide C++ details from the client app. BUG=706289 ==========
https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:164: _translateUIDelegate->SetAlwaysTranslate(false, languageCode, On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > The documentation says that target_language is ignored if |value| is false [1], > so there is no need to query and pass autoTargetLanguageCode here, so you can > just do the following: > > _translateUIDelegate->SetLanguageBlocked(false, languageCode); > _translateUIDelegate->SetAlwaysTranslate(false, languageCode, std::string()); > > Or if the target_language is not ignored, then the documentation must be > updated. > > [1]: // If |value| is false, updates the preference not to always translate > // |from_language|. |target_language| is ignored. Done. https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| is CWVTranslationPolicyAuto. On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > null -> Nil (since it is a Objective-C type). > > > > If you want to avoid capitalizing the N, then you can reformulate this to > > > > // Indicates the target language to automatically translate to. Only defined > > // if |type| is CWVTranslationPolicyAuto. > > eugenebut@ said he perfers nil to Nil because Nil means a dfferent thing in > Objective-C. > I personally agree with him to prefer Objective-C correctness to English > correctness here, but if you do care, your proposed refinement sounds good. Or > maybe just add "It is": > > // It is nil unless |type| is CWVTranslationPolicyAuto. I'll do "It is nil"
On 2017/05/18 05:20:09, groby wrote: > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| is > > CWVTranslationPolicyAuto. > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > null -> Nil (since it is a Objective-C type). > > > > > > If you want to avoid capitalizing the N, then you can reformulate this to > > > > > > // Indicates the target language to automatically translate to. Only defined > > > // if |type| is CWVTranslationPolicyAuto. > > > > eugenebut@ said he perfers nil to Nil because Nil means a dfferent thing in > > Objective-C. > > I personally agree with him to prefer Objective-C correctness to English > > correctness here, but if you do care, your proposed refinement sounds good. Or > > maybe just add "It is": > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > I'm currently not clear why we need a policy API - the bug doesn't seem to say - > or why the delegate functions need to be parametrized. Could I ask you to > explain what motivates the changes? Hey groby, I updated a few things, let me know if it makes things more clear :)
On 2017/05/22 03:56:26, jzw1 wrote: > On 2017/05/18 05:20:09, groby wrote: > > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| is > > > CWVTranslationPolicyAuto. > > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > > null -> Nil (since it is a Objective-C type). > > > > > > > > If you want to avoid capitalizing the N, then you can reformulate this to > > > > > > > > // Indicates the target language to automatically translate to. Only > defined > > > > // if |type| is CWVTranslationPolicyAuto. > > > > > > eugenebut@ said he perfers nil to Nil because Nil means a dfferent thing in > > > Objective-C. > > > I personally agree with him to prefer Objective-C correctness to English > > > correctness here, but if you do care, your proposed refinement sounds good. > Or > > > maybe just add "It is": > > > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > > > I'm currently not clear why we need a policy API - the bug doesn't seem to say > - > > or why the delegate functions need to be parametrized. Could I ask you to > > explain what motivates the changes? > > Hey groby, I updated a few things, let me know if it makes things more clear :) A little, but I'm still unclear why we need a policy :( I'm _guessing_ you want different behaviors than other translate clients, but I'm not entirely sure. Is there any doc outlining what this is for, to help me understand? Also, adding napper@ - he's the TL of the translate team and should probably know about this as well :)
groby@chromium.org changed reviewers: + napper@chromium.org
napper@: FYI, seems related to different policies on iOS.
On 2017/05/22 14:44:42, groby wrote: > On 2017/05/22 03:56:26, jzw1 wrote: > > On 2017/05/18 05:20:09, groby wrote: > > > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| is > > > > CWVTranslationPolicyAuto. > > > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > > > null -> Nil (since it is a Objective-C type). > > > > > > > > > > If you want to avoid capitalizing the N, then you can reformulate this > to > > > > > > > > > > // Indicates the target language to automatically translate to. Only > > defined > > > > > // if |type| is CWVTranslationPolicyAuto. > > > > > > > > eugenebut@ said he perfers nil to Nil because Nil means a dfferent thing > in > > > > Objective-C. > > > > I personally agree with him to prefer Objective-C correctness to English > > > > correctness here, but if you do care, your proposed refinement sounds > good. > > Or > > > > maybe just add "It is": > > > > > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > > > > > I'm currently not clear why we need a policy API - the bug doesn't seem to > say > > - > > > or why the delegate functions need to be parametrized. Could I ask you to > > > explain what motivates the changes? > > > > Hey groby, I updated a few things, let me know if it makes things more clear > :) > > A little, but I'm still unclear why we need a policy :( > > I'm _guessing_ you want different behaviors than other translate clients, but > I'm not entirely sure. Is there any doc outlining what this is for, to help me > understand? > > Also, adding napper@ - he's the TL of the translate team and should probably > know about this as well :) We don't want different behaviors, this is just to provide a pure obj-C wrapper.
On 2017/05/22 16:13:24, jzw1 wrote: > On 2017/05/22 14:44:42, groby wrote: > > On 2017/05/22 03:56:26, jzw1 wrote: > > > On 2017/05/18 05:20:09, groby wrote: > > > > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| > is > > > > > CWVTranslationPolicyAuto. > > > > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > > > > null -> Nil (since it is a Objective-C type). > > > > > > > > > > > > If you want to avoid capitalizing the N, then you can reformulate this > > to > > > > > > > > > > > > // Indicates the target language to automatically translate to. Only > > > defined > > > > > > // if |type| is CWVTranslationPolicyAuto. > > > > > > > > > > eugenebut@ said he perfers nil to Nil because Nil means a dfferent thing > > in > > > > > Objective-C. > > > > > I personally agree with him to prefer Objective-C correctness to English > > > > > correctness here, but if you do care, your proposed refinement sounds > > good. > > > Or > > > > > maybe just add "It is": > > > > > > > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > > > > > > > I'm currently not clear why we need a policy API - the bug doesn't seem to > > say > > > - > > > > or why the delegate functions need to be parametrized. Could I ask you to > > > > explain what motivates the changes? > > > > > > Hey groby, I updated a few things, let me know if it makes things more clear > > :) > > > > A little, but I'm still unclear why we need a policy :( > > > > I'm _guessing_ you want different behaviors than other translate clients, but > > I'm not entirely sure. Is there any doc outlining what this is for, to help me > > understand? > > > > Also, adding napper@ - he's the TL of the translate team and should probably > > know about this as well :) > > We don't want different behaviors, this is just to provide a pure obj-C wrapper. Not sure whether this has been mentioned earlier, but this is to allow other apps than Chrome on iOS to re-use the translation.
On 2017/05/22 16:57:45, sdefresne wrote: > On 2017/05/22 16:13:24, jzw1 wrote: > > On 2017/05/22 14:44:42, groby wrote: > > > On 2017/05/22 03:56:26, jzw1 wrote: > > > > On 2017/05/18 05:20:09, groby wrote: > > > > > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > ios/web_view/public/cwv_translation_policy.h:40: // null unless |type| > > is > > > > > > CWVTranslationPolicyAuto. > > > > > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > > > > > null -> Nil (since it is a Objective-C type). > > > > > > > > > > > > > > If you want to avoid capitalizing the N, then you can reformulate > this > > > to > > > > > > > > > > > > > > // Indicates the target language to automatically translate to. Only > > > > defined > > > > > > > // if |type| is CWVTranslationPolicyAuto. > > > > > > > > > > > > eugenebut@ said he perfers nil to Nil because Nil means a dfferent > thing > > > in > > > > > > Objective-C. > > > > > > I personally agree with him to prefer Objective-C correctness to > English > > > > > > correctness here, but if you do care, your proposed refinement sounds > > > good. > > > > Or > > > > > > maybe just add "It is": > > > > > > > > > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > > > > > > > > > I'm currently not clear why we need a policy API - the bug doesn't seem > to > > > say > > > > - > > > > > or why the delegate functions need to be parametrized. Could I ask you > to > > > > > explain what motivates the changes? > > > > > > > > Hey groby, I updated a few things, let me know if it makes things more > clear > > > :) > > > > > > A little, but I'm still unclear why we need a policy :( > > > > > > I'm _guessing_ you want different behaviors than other translate clients, > but > > > I'm not entirely sure. Is there any doc outlining what this is for, to help > me > > > understand? > > > > > > Also, adding napper@ - he's the TL of the translate team and should probably > > > know about this as well :) > > > > We don't want different behaviors, this is just to provide a pure obj-C > wrapper. > > Not sure whether this has been mentioned earlier, but this is to allow other > apps than Chrome on iOS to re-use the translation. Several points: * This is not just a wrapper for ObjC - it extends the API surface. It's not clear to me why we need to do that. * Assuming we need to extend the API - if we do want a generic ability to set policy, it belongs on the TranslatePrefs object, not the UI delegate. The UI delegate is *only* responsible for handling interactions with the translate infobar/bubble. * We don't usually make changes to Chrome just to accommodate "other apps". If this is necessary, let's have a quick chat around that involving the Chrome Translate Team, because they're likely on the hook for long-term maintenance.
On 2017/05/22 17:19:41, groby wrote: > On 2017/05/22 16:57:45, sdefresne wrote: > > On 2017/05/22 16:13:24, jzw1 wrote: > > > On 2017/05/22 14:44:42, groby wrote: > > > > On 2017/05/22 03:56:26, jzw1 wrote: > > > > > On 2017/05/18 05:20:09, groby wrote: > > > > > > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > > ios/web_view/public/cwv_translation_policy.h:40: // null unless > |type| > > > is > > > > > > > CWVTranslationPolicyAuto. > > > > > > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > > > > > > null -> Nil (since it is a Objective-C type). > > > > > > > > > > > > > > > > If you want to avoid capitalizing the N, then you can reformulate > > this > > > > to > > > > > > > > > > > > > > > > // Indicates the target language to automatically translate to. > Only > > > > > defined > > > > > > > > // if |type| is CWVTranslationPolicyAuto. > > > > > > > > > > > > > > eugenebut@ said he perfers nil to Nil because Nil means a dfferent > > thing > > > > in > > > > > > > Objective-C. > > > > > > > I personally agree with him to prefer Objective-C correctness to > > English > > > > > > > correctness here, but if you do care, your proposed refinement > sounds > > > > good. > > > > > Or > > > > > > > maybe just add "It is": > > > > > > > > > > > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > > > > > > > > > > > I'm currently not clear why we need a policy API - the bug doesn't > seem > > to > > > > say > > > > > - > > > > > > or why the delegate functions need to be parametrized. Could I ask you > > to > > > > > > explain what motivates the changes? > > > > > > > > > > Hey groby, I updated a few things, let me know if it makes things more > > clear > > > > :) > > > > > > > > A little, but I'm still unclear why we need a policy :( > > > > > > > > I'm _guessing_ you want different behaviors than other translate clients, > > but > > > > I'm not entirely sure. Is there any doc outlining what this is for, to > help > > me > > > > understand? > > > > > > > > Also, adding napper@ - he's the TL of the translate team and should > probably > > > > know about this as well :) > > > > > > We don't want different behaviors, this is just to provide a pure obj-C > > wrapper. > > > > Not sure whether this has been mentioned earlier, but this is to allow other > > apps than Chrome on iOS to re-use the translation. > > Several points: > > * This is not just a wrapper for ObjC - it extends the API surface. It's not > clear to me why we need to do that. > > * Assuming we need to extend the API - if we do want a generic ability to set > policy, it belongs on the TranslatePrefs object, not the UI delegate. The UI > delegate is *only* responsible for handling interactions with the translate > infobar/bubble. > > * We don't usually make changes to Chrome just to accommodate "other apps". If > this is necessary, let's have a quick chat around that involving the Chrome > Translate Team, because they're likely on the hook for long-term maintenance. Maybe can we have quick VC to explain the background? Or do you prefer email conversation? Also, who should be involved in the VC or the mail thread? groby@, napper@, eugenebut@, sdefresne@?
Either is fine by me. On Tue, 23 May 2017 at 11:48 <ichikawa@chromium.org> wrote: > On 2017/05/22 17:19:41, groby wrote: > > On 2017/05/22 16:57:45, sdefresne wrote: > > > On 2017/05/22 16:13:24, jzw1 wrote: > > > > On 2017/05/22 14:44:42, groby wrote: > > > > > On 2017/05/22 03:56:26, jzw1 wrote: > > > > > > On 2017/05/18 05:20:09, groby wrote: > > > > > > > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > > > ios/web_view/public/cwv_translation_policy.h:40: // null > unless > > |type| > > > > is > > > > > > > > CWVTranslationPolicyAuto. > > > > > > > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > > > > > > > null -> Nil (since it is a Objective-C type). > > > > > > > > > > > > > > > > > > If you want to avoid capitalizing the N, then you can > reformulate > > > this > > > > > to > > > > > > > > > > > > > > > > > > // Indicates the target language to automatically > translate to. > > Only > > > > > > defined > > > > > > > > > // if |type| is CWVTranslationPolicyAuto. > > > > > > > > > > > > > > > > eugenebut@ said he perfers nil to Nil because Nil means a > dfferent > > > thing > > > > > in > > > > > > > > Objective-C. > > > > > > > > I personally agree with him to prefer Objective-C > correctness to > > > English > > > > > > > > correctness here, but if you do care, your proposed > refinement > > sounds > > > > > good. > > > > > > Or > > > > > > > > maybe just add "It is": > > > > > > > > > > > > > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > > > > > > > > > > > > > I'm currently not clear why we need a policy API - the bug > doesn't > > seem > > > to > > > > > say > > > > > > - > > > > > > > or why the delegate functions need to be parametrized. Could I > ask > you > > > to > > > > > > > explain what motivates the changes? > > > > > > > > > > > > Hey groby, I updated a few things, let me know if it makes > things more > > > clear > > > > > :) > > > > > > > > > > A little, but I'm still unclear why we need a policy :( > > > > > > > > > > I'm _guessing_ you want different behaviors than other translate > clients, > > > but > > > > > I'm not entirely sure. Is there any doc outlining what this is > for, to > > help > > > me > > > > > understand? > > > > > > > > > > Also, adding napper@ - he's the TL of the translate team and > should > > probably > > > > > know about this as well :) > > > > > > > > We don't want different behaviors, this is just to provide a pure > obj-C > > > wrapper. > > > > > > Not sure whether this has been mentioned earlier, but this is to allow > other > > > apps than Chrome on iOS to re-use the translation. > > > > Several points: > > > > * This is not just a wrapper for ObjC - it extends the API surface. It's > not > > clear to me why we need to do that. > > > > * Assuming we need to extend the API - if we do want a generic ability > to set > > policy, it belongs on the TranslatePrefs object, not the UI delegate. > The UI > > delegate is *only* responsible for handling interactions with the > translate > > infobar/bubble. > > > > * We don't usually make changes to Chrome just to accommodate "other > apps". If > > this is necessary, let's have a quick chat around that involving the > Chrome > > Translate Team, because they're likely on the hook for long-term > maintenance. > > Maybe can we have quick VC to explain the background? Or do you prefer > email > conversation? > > Also, who should be involved in the VC or the mail thread? groby@, napper@ > , > eugenebut@, sdefresne@? > > https://codereview.chromium.org/2872083003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/23 03:07:16, chromium-reviews wrote: > Either is fine by me. > > On Tue, 23 May 2017 at 11:48 <mailto:ichikawa@chromium.org> wrote: > > > On 2017/05/22 17:19:41, groby wrote: > > > On 2017/05/22 16:57:45, sdefresne wrote: > > > > On 2017/05/22 16:13:24, jzw1 wrote: > > > > > On 2017/05/22 14:44:42, groby wrote: > > > > > > On 2017/05/22 03:56:26, jzw1 wrote: > > > > > > > On 2017/05/18 05:20:09, groby wrote: > > > > > > > > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > > > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > > > > ios/web_view/public/cwv_translation_policy.h:40: // null > > unless > > > |type| > > > > > is > > > > > > > > > CWVTranslationPolicyAuto. > > > > > > > > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > > > > > > > > null -> Nil (since it is a Objective-C type). > > > > > > > > > > > > > > > > > > > > If you want to avoid capitalizing the N, then you can > > reformulate > > > > this > > > > > > to > > > > > > > > > > > > > > > > > > > > // Indicates the target language to automatically > > translate to. > > > Only > > > > > > > defined > > > > > > > > > > // if |type| is CWVTranslationPolicyAuto. > > > > > > > > > > > > > > > > > > eugenebut@ said he perfers nil to Nil because Nil means a > > dfferent > > > > thing > > > > > > in > > > > > > > > > Objective-C. > > > > > > > > > I personally agree with him to prefer Objective-C > > correctness to > > > > English > > > > > > > > > correctness here, but if you do care, your proposed > > refinement > > > sounds > > > > > > good. > > > > > > > Or > > > > > > > > > maybe just add "It is": > > > > > > > > > > > > > > > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > > > > > > > > > > > > > > > I'm currently not clear why we need a policy API - the bug > > doesn't > > > seem > > > > to > > > > > > say > > > > > > > - > > > > > > > > or why the delegate functions need to be parametrized. Could I > > ask > > you > > > > to > > > > > > > > explain what motivates the changes? > > > > > > > > > > > > > > Hey groby, I updated a few things, let me know if it makes > > things more > > > > clear > > > > > > :) > > > > > > > > > > > > A little, but I'm still unclear why we need a policy :( > > > > > > > > > > > > I'm _guessing_ you want different behaviors than other translate > > clients, > > > > but > > > > > > I'm not entirely sure. Is there any doc outlining what this is > > for, to > > > help > > > > me > > > > > > understand? > > > > > > > > > > > > Also, adding napper@ - he's the TL of the translate team and > > should > > > probably > > > > > > know about this as well :) > > > > > > > > > > We don't want different behaviors, this is just to provide a pure > > obj-C > > > > wrapper. > > > > > > > > Not sure whether this has been mentioned earlier, but this is to allow > > other > > > > apps than Chrome on iOS to re-use the translation. > > > > > > Several points: > > > > > > * This is not just a wrapper for ObjC - it extends the API surface. It's > > not > > > clear to me why we need to do that. > > > > > > * Assuming we need to extend the API - if we do want a generic ability > > to set > > > policy, it belongs on the TranslatePrefs object, not the UI delegate. > > The UI > > > delegate is *only* responsible for handling interactions with the > > translate > > > infobar/bubble. > > > > > > * We don't usually make changes to Chrome just to accommodate "other > > apps". If > > > this is necessary, let's have a quick chat around that involving the > > Chrome > > > Translate Team, because they're likely on the hook for long-term > > maintenance. > > > > Maybe can we have quick VC to explain the background? Or do you prefer > > email > > conversation? > > > > Also, who should be involved in the VC or the mail thread? groby@, napper@ > > , > > eugenebut@, sdefresne@? > > > > https://codereview.chromium.org/2872083003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. OK, let me set up a VC.
On 2017/05/23 03:54:42, Hiroshi Ichikawa wrote: > On 2017/05/23 03:07:16, chromium-reviews wrote: > > Either is fine by me. > > > > On Tue, 23 May 2017 at 11:48 <mailto:ichikawa@chromium.org> wrote: > > > > > On 2017/05/22 17:19:41, groby wrote: > > > > On 2017/05/22 16:57:45, sdefresne wrote: > > > > > On 2017/05/22 16:13:24, jzw1 wrote: > > > > > > On 2017/05/22 14:44:42, groby wrote: > > > > > > > On 2017/05/22 03:56:26, jzw1 wrote: > > > > > > > > On 2017/05/18 05:20:09, groby wrote: > > > > > > > > > On 2017/05/18 02:34:56, Hiroshi Ichikawa wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > > > > > File ios/web_view/public/cwv_translation_policy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2872083003/diff/200001/ios/web_view/public/cw... > > > > > > > > > > ios/web_view/public/cwv_translation_policy.h:40: // null > > > unless > > > > |type| > > > > > > is > > > > > > > > > > CWVTranslationPolicyAuto. > > > > > > > > > > On 2017/05/17 09:47:30, sdefresne OOO till 22-05-2017 wrote: > > > > > > > > > > > null -> Nil (since it is a Objective-C type). > > > > > > > > > > > > > > > > > > > > > > If you want to avoid capitalizing the N, then you can > > > reformulate > > > > > this > > > > > > > to > > > > > > > > > > > > > > > > > > > > > > // Indicates the target language to automatically > > > translate to. > > > > Only > > > > > > > > defined > > > > > > > > > > > // if |type| is CWVTranslationPolicyAuto. > > > > > > > > > > > > > > > > > > > > eugenebut@ said he perfers nil to Nil because Nil means a > > > dfferent > > > > > thing > > > > > > > in > > > > > > > > > > Objective-C. > > > > > > > > > > I personally agree with him to prefer Objective-C > > > correctness to > > > > > English > > > > > > > > > > correctness here, but if you do care, your proposed > > > refinement > > > > sounds > > > > > > > good. > > > > > > > > Or > > > > > > > > > > maybe just add "It is": > > > > > > > > > > > > > > > > > > > > // It is nil unless |type| is CWVTranslationPolicyAuto. > > > > > > > > > > > > > > > > > > I'm currently not clear why we need a policy API - the bug > > > doesn't > > > > seem > > > > > to > > > > > > > say > > > > > > > > - > > > > > > > > > or why the delegate functions need to be parametrized. Could I > > > ask > > > you > > > > > to > > > > > > > > > explain what motivates the changes? > > > > > > > > > > > > > > > > Hey groby, I updated a few things, let me know if it makes > > > things more > > > > > clear > > > > > > > :) > > > > > > > > > > > > > > A little, but I'm still unclear why we need a policy :( > > > > > > > > > > > > > > I'm _guessing_ you want different behaviors than other translate > > > clients, > > > > > but > > > > > > > I'm not entirely sure. Is there any doc outlining what this is > > > for, to > > > > help > > > > > me > > > > > > > understand? > > > > > > > > > > > > > > Also, adding napper@ - he's the TL of the translate team and > > > should > > > > probably > > > > > > > know about this as well :) > > > > > > > > > > > > We don't want different behaviors, this is just to provide a pure > > > obj-C > > > > > wrapper. > > > > > > > > > > Not sure whether this has been mentioned earlier, but this is to allow > > > other > > > > > apps than Chrome on iOS to re-use the translation. > > > > > > > > Several points: > > > > > > > > * This is not just a wrapper for ObjC - it extends the API surface. It's > > > not > > > > clear to me why we need to do that. > > > > > > > > * Assuming we need to extend the API - if we do want a generic ability > > > to set > > > > policy, it belongs on the TranslatePrefs object, not the UI delegate. > > > The UI > > > > delegate is *only* responsible for handling interactions with the > > > translate > > > > infobar/bubble. > > > > > > > > * We don't usually make changes to Chrome just to accommodate "other > > > apps". If > > > > this is necessary, let's have a quick chat around that involving the > > > Chrome > > > > Translate Team, because they're likely on the hook for long-term > > > maintenance. > > > > > > Maybe can we have quick VC to explain the background? Or do you prefer > > > email > > > conversation? > > > > > > Also, who should be involved in the VC or the mail thread? groby@, napper@ > > > , > > > eugenebut@, sdefresne@? > > > > > > https://codereview.chromium.org/2872083003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > OK, let me set up a VC. I removed all changes in translate_ui_delegate per our VC discussion. PTAL :)
https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:52: - (CWVTranslationLanguage*)languageWithCode:(NSString*)languageCode; Optional: Maybe this method should take const std::string& instead of NSString*? Looks you always call this method in this pattern: [self languageWithCode:base::SysUTF8ToNSString(s)] https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:84: for (std::vector<std::string>::const_iterator iter = language_codes.begin(); for (const std::string languageCode : language_codes) { https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:86: std::string languageCode = *iter; Be consistent with the variable name style between language_codes and languageCode. Not sure which should be used here, though... Probably languageCode style? https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:100: _supportedLanguages = [supportedLanguages copy]; Why do you copy here? https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:212: autoTargetLanguageCode = std::string(); How about returning [CWVTranslationPolicy translationPolicyAsk] directly here?
https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:52: - (CWVTranslationLanguage*)languageWithCode:(NSString*)languageCode; On 2017/05/25 06:44:22, Hiroshi Ichikawa wrote: > Optional: Maybe this method should take const std::string& instead of NSString*? > Looks you always call this method in this pattern: > [self languageWithCode:base::SysUTF8ToNSString(s)] Done. https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:84: for (std::vector<std::string>::const_iterator iter = language_codes.begin(); On 2017/05/25 06:44:22, Hiroshi Ichikawa wrote: > for (const std::string languageCode : language_codes) { Done. https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:86: std::string languageCode = *iter; On 2017/05/25 06:44:22, Hiroshi Ichikawa wrote: > Be consistent with the variable name style between language_codes and > languageCode. Not sure which should be used here, though... Probably > languageCode style? Done. https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:100: _supportedLanguages = [supportedLanguages copy]; On 2017/05/25 06:44:22, Hiroshi Ichikawa wrote: > Why do you copy here? the local var is a NSMutableArray so I usually copy it so it becomes a NSArray https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:212: autoTargetLanguageCode = std::string(); On 2017/05/25 06:44:22, Hiroshi Ichikawa wrote: > How about returning [CWVTranslationPolicy translationPolicyAsk] directly here? Done.
lgtm https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:84: for (std::vector<std::string>::const_iterator iter = language_codes.begin(); On 2017/05/25 06:55:37, jzw1 wrote: > On 2017/05/25 06:44:22, Hiroshi Ichikawa wrote: > > for (const std::string languageCode : language_codes) { > > Done. Oops sorry it's better to add "&": for (const std::string& languageCode : languageCodes) { https://codereview.chromium.org/2872083003/diff/240001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:100: _supportedLanguages = [supportedLanguages copy]; On 2017/05/25 06:55:37, jzw1 wrote: > On 2017/05/25 06:44:22, Hiroshi Ichikawa wrote: > > Why do you copy here? > > the local var is a NSMutableArray so I usually copy it so it becomes a NSArray Oh I see, good to know. I didn't know -[NSMutableArray copy] returns NSArray.
still lgtm https://codereview.chromium.org/2872083003/diff/280001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/280001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:26: - (instancetype)init NS_UNAVAILABLE; Sorry, for missing this earlier. Per Style Guide, the order of API is: class methods properties instance methods
https://codereview.chromium.org/2872083003/diff/280001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_policy.h (right): https://codereview.chromium.org/2872083003/diff/280001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_policy.h:26: - (instancetype)init NS_UNAVAILABLE; On 2017/05/25 14:38:53, Eugene But wrote: > Sorry, for missing this earlier. Per Style Guide, the order of API is: > class methods > properties > instance methods Done.
Still LGTM as well! PTAL at a few suggestions. https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:98: _supportedLanguages = [supportedLanguages copy]; |_supportedLanguages| won't change and isn't dependent on |webState|. Can this be move to the initializer (or even better, lazy load this array which will prevent doing this work if translate is never used.)? https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:219: DCHECK(pageHost.length > 0); Per style convention, no need for "> 0". https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:231: // TODO(crbug.com/706289): Support auto translation policies for websites. nit: Maybe add this TODO into translationPolicyForPageHost as well as a reminder to implement the equivalent there too. https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:252: if ([language.languageCode isEqualToString:languageCodeString]) { We call this method a lot and this isn't very efficient. WDYT if we make |_supportedLanguages| an NSDictionary instead and use the languageCode as the key for the CWVTranslationLanguage object? I did notice you are sorting the array on creation, but I assume that is for the delegate method called from |translate::TRANSLATE_STEP_BEFORE_TRANSLATE|? If we want to keep a sorted array, you can lazily create |sortedSupportedLanguages| if/when needed to send to that delegate method. I think that is reasonable and seems much cleaner than returning |allValues| of a dictionary.
On 2017/05/25 16:18:59, michaeldo wrote: > Still LGTM as well! PTAL at a few suggestions. > > https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... > File ios/web_view/internal/translate/cwv_translation_controller.mm (right): > > https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller.mm:98: > _supportedLanguages = [supportedLanguages copy]; > |_supportedLanguages| won't change and isn't dependent on |webState|. Can this > be move to the initializer (or even better, lazy load this array which will > prevent doing this work if translate is never used.)? > > https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller.mm:219: > DCHECK(pageHost.length > 0); > Per style convention, no need for "> 0". > > https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller.mm:231: // > TODO(crbug.com/706289): Support auto translation policies for websites. > nit: Maybe add this TODO into translationPolicyForPageHost as well as a reminder > to implement the equivalent there too. > > https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller.mm:252: if > ([language.languageCode isEqualToString:languageCodeString]) { > We call this method a lot and this isn't very efficient. WDYT if we make > |_supportedLanguages| an NSDictionary instead and use the languageCode as the > key for the CWVTranslationLanguage object? > > I did notice you are sorting the array on creation, but I assume that is for the > delegate method called from |translate::TRANSLATE_STEP_BEFORE_TRANSLATE|? If we > want to keep a sorted array, you can lazily create |sortedSupportedLanguages| > if/when needed to send to that delegate method. I think that is reasonable and > seems much cleaner than returning |allValues| of a dictionary. No translate component any more, so LGTM from that side :)
lgtm
https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:98: _supportedLanguages = [supportedLanguages copy]; On 2017/05/25 16:18:58, michaeldo wrote: > |_supportedLanguages| won't change and isn't dependent on |webState|. Can this > be move to the initializer (or even better, lazy load this array which will > prevent doing this work if translate is never used.)? ill lazy load this https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:219: DCHECK(pageHost.length > 0); On 2017/05/25 16:18:58, michaeldo wrote: > Per style convention, no need for "> 0". Done. https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:231: // TODO(crbug.com/706289): Support auto translation policies for websites. On 2017/05/25 16:18:58, michaeldo wrote: > nit: Maybe add this TODO into translationPolicyForPageHost as well as a reminder > to implement the equivalent there too. Done. https://codereview.chromium.org/2872083003/diff/300001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:252: if ([language.languageCode isEqualToString:languageCodeString]) { On 2017/05/25 16:18:58, michaeldo wrote: > We call this method a lot and this isn't very efficient. WDYT if we make > |_supportedLanguages| an NSDictionary instead and use the languageCode as the > key for the CWVTranslationLanguage object? > > I did notice you are sorting the array on creation, but I assume that is for the > delegate method called from |translate::TRANSLATE_STEP_BEFORE_TRANSLATE|? If we > want to keep a sorted array, you can lazily create |sortedSupportedLanguages| > if/when needed to send to that delegate method. I think that is reasonable and > seems much cleaner than returning |allValues| of a dictionary. This is great advice. Done :)
The CQ bit was checked by jzw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, ichikawa@chromium.org, eugenebut@chromium.org, napper@chromium.org, groby@chromium.org, michaeldo@chromium.org Link to the patchset: https://codereview.chromium.org/2872083003/#ps320001 (title: "refactored supported languages to use a NSDictionary")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jzw@chromium.org
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 jzw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org, groby@chromium.org, eugenebut@chromium.org, sdefresne@chromium.org, michaeldo@chromium.org, ichikawa@chromium.org Link to the patchset: https://codereview.chromium.org/2872083003/#ps340001 (title: "sync")
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": 340001, "attempt_start_ts": 1496019623033820,
"parent_rev": "bc9f245e0196fd1597d504f0e6edcd1e868a45e3", "commit_rev":
"c21bd08f739ae50633429f5a15ac7f62ebcdea8e"}
Message was sent while issue was closed.
Description was changed from ========== Added translation policy API. Translation policies are configured by the user to decide how to translate certain languages. This API is written to be purely Objective-C to hide C++ details from the client app. BUG=706289 ========== to ========== Added translation policy API. Translation policies are configured by the user to decide how to translate certain languages. This API is written to be purely Objective-C to hide C++ details from the client app. BUG=706289 Review-Url: https://codereview.chromium.org/2872083003 Cr-Commit-Position: refs/heads/master@{#475276} Committed: https://chromium.googlesource.com/chromium/src/+/c21bd08f739ae50633429f5a15ac... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/c21bd08f739ae50633429f5a15ac... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
