Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839093002:
Implemented new Translate API for purely Objective-C clients. (Closed)
DescriptionImplemented new Translate API for purely Objective-C clients.
BUG=706289
Review-Url: https://codereview.chromium.org/2839093002
Cr-Commit-Position: refs/heads/master@{#470236}
Committed: https://chromium.googlesource.com/chromium/src/+/140c4b6e2b2db009b9a82a6316497b919c5031fe
Patch Set 1 #Patch Set 2 : added CWV_EXPORT, header guards, and ARC guards. #Patch Set 3 : Added some error handling. #
Total comments: 71
Patch Set 4 : addressed comments #Patch Set 5 : address more issues #Patch Set 6 : added TODO #
Total comments: 10
Patch Set 7 : change error codes and renamed some things #
Total comments: 20
Patch Set 8 : more comments addressed #Patch Set 9 : missed a comment #Patch Set 10 : rename param #
Total comments: 8
Patch Set 11 : addressed final comments #
Total comments: 59
Patch Set 12 : Addressed michael's comments #
Total comments: 2
Patch Set 13 : changed comment #Patch Set 14 : merge #Messages
Total messages: 41 (9 generated)
Description was changed from ========== Implemented new Translate API for purely Objective-C clients. BUG=706289 ========== to ========== Implemented new Translate API for purely Objective-C clients. BUG=706289 ==========
jzw@chromium.org changed reviewers: + eugenebut@chromium.org, ichikawa@chromium.org, michaeldo@chromium.org
On 2017/04/26 07:37:28, jzw1 wrote: > mailto:jzw@chromium.org changed reviewers: > + mailto:eugenebut@chromium.org, mailto:ichikawa@chromium.org, mailto:michaeldo@chromium.org Hey guys, created an initial CL based on our discussions. PTAL.
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... ios/web_view/internal/cwv_web_view.mm:290: - (CWVTranslationController*)translationController { When -resetWebStateWithSessionStorage: is called after the first call to -translationController, I believe you need to update _translationController with the new _webState. FYI You may do something similar to my CL: https://codereview.chromium.org/2842953002/ https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:45: @protocol CWVTranslationControllerDelegate<NSObject> How about marking this protocol as @optional, and call each method only if it is defined? I guess it's a common pattern of delegates? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Begins translation on the current page from |sourceLanguage| to Can you add a comment that this must not be called before -translationController:didFinishLanguageDetectionWithResult:error: is called? Also, can you document what happens if I call it before? Error? No-op? Also, can you document whether we can call this method twice before the first one completes? It can be TODO if you are not sure. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:82: // Reverts any translations done back to the original page language. Can you add a comment that this must not be called before -translationController:didFinishLanguageDetectionWithResult:error: is called? Also, can you document what happens if I call it before? Error? No-op? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_language.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_language.h:17: // The ISO language code. en for English, es for Spanish, etc... Can you add a link to https://cloud.google.com/translate/docs/languages to clarify the spec? I believe this is the one used here... https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/shell/shel... File ios/web_view/shell/shell_view_controller.m (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/shell/shel... ios/web_view/shell/shell_view_controller.m:158: _translateController = [[TranslateController alloc] init]; Can you maybe rename this class and the variable name to _translateDelegate or something? It's confusing with _webView.translationController now.
Sorry, I had a chance to only look at public API https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:10: Do you want to use nullability macro here? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:17: - (instancetype)initWithSourceLanguage:(CWVTranslationLanguage*)sourceLanguage Does this method needs to be public? Client don't need to call it, right? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:27: // The assumed target language based on OS locale. Do you want to be more specific (f.e. say that this is the language that the page will be translated to.) https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, readonly) CWVTranslationLanguage* targetLanguage; Should this be something like, suggestedTranslationLanguage? target language is ambiguous. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:29: // The list of languages that can be used in translation. nit: "The list of supported languages that can be used in translation" ? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:15: // Possible errors during translation. s/errors/error codes ? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:16: typedef NS_ENUM(NSInteger, CWVTranslationError) { Is there a need for enum? Should these constants be just integers? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:18: CWVTranslationErrorNone = 0, Is there a need for this item? This error code can never be used, right? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:20: CWVTranslationErrorNetwork, Normally large negative numbers are used for error codes. Do you want to use one? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:45: @protocol CWVTranslationControllerDelegate<NSObject> On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > How about marking this protocol as @optional, and call each method only if it is > defined? I guess it's a common pattern of delegates? +1 https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:50: // |error| will be filled if language detection failed. Do you want to create a constant for Error Domain? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:55: // Called when a translation is in progress. Used to show loading indicator. s/is in progress/has started ? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:55: // Called when a translation is in progress. Used to show loading indicator. >> Used to show loading indicator. Please do not make assumptions how this is used. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:78: // |targetLanguage|. Could you please specify that |targetLanguage| should be in supported languages. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:84: // passed to |translatePageFromLanguage:toLanguage:| above. What happens if this is called for a page which has not been translated? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_language.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_language.h:20: // The language name in the current OS locale. How about "Localized language name"?
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, readonly) CWVTranslationLanguage* targetLanguage; On 2017/04/26 20:15:30, Eugene But wrote: > Should this be something like, suggestedTranslationLanguage? target language is > ambiguous. I believe suggestedTranslationLanguage is also ambiguous, not clear whether it's a language to translate from or translate into. Not really sure what is the good name for it, but some ideas: suggestedTargetLanguage suggestedTranslationTargetLanguage estimatedUserLanguage
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... ios/web_view/internal/cwv_web_view.mm:290: - (CWVTranslationController*)translationController { On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > When -resetWebStateWithSessionStorage: is called after the first call to > -translationController, I believe you need to update _translationController with > the new _webState. > > FYI You may do something similar to my CL: > https://codereview.chromium.org/2842953002/ Are you OK if i just nil it out _translationController inside resetWebState? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:10: On 2017/04/26 20:15:30, Eugene But wrote: > Do you want to use nullability macro here? Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:17: - (instancetype)initWithSourceLanguage:(CWVTranslationLanguage*)sourceLanguage On 2017/04/26 20:15:30, Eugene But wrote: > Does this method needs to be public? Client don't need to call it, right? moved to internal header file. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:27: // The assumed target language based on OS locale. On 2017/04/26 20:15:30, Eugene But wrote: > Do you want to be more specific (f.e. say that this is the language that the > page will be translated to.) Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, readonly) CWVTranslationLanguage* targetLanguage; On 2017/04/27 02:41:54, Hiroshi Ichikawa wrote: > On 2017/04/26 20:15:30, Eugene But wrote: > > Should this be something like, suggestedTranslationLanguage? target language > is > > ambiguous. > > I believe suggestedTranslationLanguage is also ambiguous, not clear whether it's > a language to translate from or translate into. Not really sure what is the good > name for it, but some ideas: > > suggestedTargetLanguage > suggestedTranslationTargetLanguage > estimatedUserLanguage I will rename sourceLanguage to pageLanguage and targetLanguage to OSLanguage. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:29: // The list of languages that can be used in translation. On 2017/04/26 20:15:30, Eugene But wrote: > nit: "The list of supported languages that can be used in translation" ? Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:15: // Possible errors during translation. On 2017/04/26 20:15:30, Eugene But wrote: > s/errors/error codes ? Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:16: typedef NS_ENUM(NSInteger, CWVTranslationError) { On 2017/04/26 20:15:31, Eugene But wrote: > Is there a need for enum? Should these constants be just integers? I'm just copying the error codes over from translate::TranslateErrors::Type https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:18: CWVTranslationErrorNone = 0, On 2017/04/26 20:15:30, Eugene But wrote: > Is there a need for this item? This error code can never be used, right? I'm just copying the error codes over from translate::TranslateErrors::Type https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:20: CWVTranslationErrorNetwork, On 2017/04/26 20:15:31, Eugene But wrote: > Normally large negative numbers are used for error codes. Do you want to use > one? I'm just copying the error codes over from translate::TranslateErrors::Type https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:45: @protocol CWVTranslationControllerDelegate<NSObject> On 2017/04/26 20:15:31, Eugene But wrote: > On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > > How about marking this protocol as @optional, and call each method only if it > is > > defined? I guess it's a common pattern of delegates? > > +1 Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:50: // |error| will be filled if language detection failed. On 2017/04/26 20:15:31, Eugene But wrote: > Do you want to create a constant for Error Domain? Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:55: // Called when a translation is in progress. Used to show loading indicator. On 2017/04/26 20:15:31, Eugene But wrote: > s/is in progress/has started ? Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:55: // Called when a translation is in progress. Used to show loading indicator. On 2017/04/26 20:15:31, Eugene But wrote: > >> Used to show loading indicator. > Please do not make assumptions how this is used. Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:78: // |targetLanguage|. On 2017/04/26 20:15:31, Eugene But wrote: > Could you please specify that |targetLanguage| should be in supported languages. Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:82: // Reverts any translations done back to the original page language. On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > Can you add a comment that this must not be called before > -translationController:didFinishLanguageDetectionWithResult:error: is called? > Also, can you document what happens if I call it before? Error? No-op? I'll investigate this soon after I build out some more UI to test this. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:84: // passed to |translatePageFromLanguage:toLanguage:| above. On 2017/04/26 20:15:30, Eugene But wrote: > What happens if this is called for a page which has not been translated? I'll report back in a future patch https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_language.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_language.h:17: // The ISO language code. en for English, es for Spanish, etc... On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > Can you add a link to https://cloud.google.com/translate/docs/languages to > clarify the spec? I believe this is the one used here... Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_language.h:20: // The language name in the current OS locale. On 2017/04/26 20:15:31, Eugene But wrote: > How about "Localized language name"? Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/shell/shel... File ios/web_view/shell/shell_view_controller.m (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/shell/shel... ios/web_view/shell/shell_view_controller.m:158: _translateController = [[TranslateController alloc] init]; On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > Can you maybe rename this class and the variable name to _translateDelegate or > something? It's confusing with _webView.translationController now. Done.
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... ios/web_view/internal/cwv_web_view.mm:290: - (CWVTranslationController*)translationController { On 2017/04/27 04:54:01, jzw1 wrote: > On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > > When -resetWebStateWithSessionStorage: is called after the first call to > > -translationController, I believe you need to update _translationController > with > > the new _webState. > > > > FYI You may do something similar to my CL: > > https://codereview.chromium.org/2842953002/ > > Are you OK if i just nil it out _translationController inside resetWebState? I believe it doesn't work. Two cases: 1. The user stores |translationController| to a variable before -resetWebState is called. Then the controller keeps referring to the old WebState. 2. The user assigns a delegate to |translationController| before -resetWebState is called. Then the delegate won't be called after -resetWebState because the delegate is not inherited to the new |translationController|. One solution I can think of is to allow reassigning CWVTranslationController.translateClient and reassign it in -resetWebState as I do in: https://codereview.chromium.org/2842953002/ https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Begins translation on the current page from |sourceLanguage| to On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > Can you add a comment that this must not be called before > -translationController:didFinishLanguageDetectionWithResult:error: is called? > Also, can you document what happens if I call it before? Error? No-op? > > Also, can you document whether we can call this method twice before the first > one completes? It can be TODO if you are not sure. Reminder: This hasn't been resolved yet. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:82: // Reverts any translations done back to the original page language. On 2017/04/27 04:54:02, jzw1 wrote: > On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > > Can you add a comment that this must not be called before > > -translationController:didFinishLanguageDetectionWithResult:error: is called? > > Also, can you document what happens if I call it before? Error? No-op? > > I'll investigate this soon after I build out some more UI to test this. Anyway this must not be called before -translationController:didFinishLanguageDetectionWithResult:error: is called, right? Can you comment it? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:84: // passed to |translatePageFromLanguage:toLanguage:| above. On 2017/04/27 04:54:02, jzw1 wrote: > On 2017/04/26 20:15:30, Eugene But wrote: > > What happens if this is called for a page which has not been translated? > > I'll report back in a future patch Can you add a TODO comment for it?
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... ios/web_view/internal/cwv_web_view.mm:290: - (CWVTranslationController*)translationController { On 2017/04/27 05:35:42, Hiroshi Ichikawa wrote: > On 2017/04/27 04:54:01, jzw1 wrote: > > On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > > > When -resetWebStateWithSessionStorage: is called after the first call to > > > -translationController, I believe you need to update _translationController > > with > > > the new _webState. > > > > > > FYI You may do something similar to my CL: > > > https://codereview.chromium.org/2842953002/ > > > > Are you OK if i just nil it out _translationController inside resetWebState? > > I believe it doesn't work. Two cases: > > 1. The user stores |translationController| to a variable before -resetWebState > is called. Then the controller keeps referring to the old WebState. > > 2. The user assigns a delegate to |translationController| before -resetWebState > is called. Then the delegate won't be called after -resetWebState because the > delegate is not inherited to the new |translationController|. > > One solution I can think of is to allow reassigning > CWVTranslationController.translateClient and reassign it in -resetWebState as I > do in: > https://codereview.chromium.org/2842953002/ What if I just set a webState on translationController? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Begins translation on the current page from |sourceLanguage| to On 2017/04/27 05:35:42, Hiroshi Ichikawa wrote: > On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > > Can you add a comment that this must not be called before > > -translationController:didFinishLanguageDetectionWithResult:error: is called? > > Also, can you document what happens if I call it before? Error? No-op? > > > > Also, can you document whether we can call this method twice before the first > > one completes? It can be TODO if you are not sure. > > Reminder: This hasn't been resolved yet. Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:82: // Reverts any translations done back to the original page language. On 2017/04/27 05:35:42, Hiroshi Ichikawa wrote: > On 2017/04/27 04:54:02, jzw1 wrote: > > On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > > > Can you add a comment that this must not be called before > > > -translationController:didFinishLanguageDetectionWithResult:error: is > called? > > > Also, can you document what happens if I call it before? Error? No-op? > > > > I'll investigate this soon after I build out some more UI to test this. > > Anyway this must not be called before > -translationController:didFinishLanguageDetectionWithResult:error: is called, > right? Can you comment it? Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:84: // passed to |translatePageFromLanguage:toLanguage:| above. On 2017/04/27 05:35:42, Hiroshi Ichikawa wrote: > On 2017/04/27 04:54:02, jzw1 wrote: > > On 2017/04/26 20:15:30, Eugene But wrote: > > > What happens if this is called for a page which has not been translated? > > > > I'll report back in a future patch > > Can you add a TODO comment for it? Done.
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/internal/c... ios/web_view/internal/cwv_web_view.mm:290: - (CWVTranslationController*)translationController { On 2017/04/27 07:03:18, jzw1 wrote: > On 2017/04/27 05:35:42, Hiroshi Ichikawa wrote: > > On 2017/04/27 04:54:01, jzw1 wrote: > > > On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > > > > When -resetWebStateWithSessionStorage: is called after the first call to > > > > -translationController, I believe you need to update > _translationController > > > with > > > > the new _webState. > > > > > > > > FYI You may do something similar to my CL: > > > > https://codereview.chromium.org/2842953002/ > > > > > > Are you OK if i just nil it out _translationController inside resetWebState? > > > > I believe it doesn't work. Two cases: > > > > 1. The user stores |translationController| to a variable before -resetWebState > > is called. Then the controller keeps referring to the old WebState. > > > > 2. The user assigns a delegate to |translationController| before > -resetWebState > > is called. Then the delegate won't be called after -resetWebState because the > > delegate is not inherited to the new |translationController|. > > > > One solution I can think of is to allow reassigning > > CWVTranslationController.translateClient and reassign it in -resetWebState as > I > > do in: > > https://codereview.chromium.org/2842953002/ > > What if I just set a webState on translationController? Yeah it looks calling -[CWVTranslationController setWebState:] on -resetWebState should work. Looks like -setWebState: already does the required job. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Begins translation on the current page from |sourceLanguage| to On 2017/04/27 07:03:18, jzw1 wrote: > On 2017/04/27 05:35:42, Hiroshi Ichikawa wrote: > > On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > > > Can you add a comment that this must not be called before > > > -translationController:didFinishLanguageDetectionWithResult:error: is > called? > > > Also, can you document what happens if I call it before? Error? No-op? > > > > > > Also, can you document whether we can call this method twice before the > first > > > one completes? It can be TODO if you are not sure. > > > > Reminder: This hasn't been resolved yet. > > Done. Looks like this part of the comment hasn't been resolved yet: > > Also, can you document whether we can call this method twice before the first > > one completes? It can be TODO if you are not sure.
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Begins translation on the current page from |sourceLanguage| to On 2017/04/27 07:17:23, Hiroshi Ichikawa wrote: > On 2017/04/27 07:03:18, jzw1 wrote: > > On 2017/04/27 05:35:42, Hiroshi Ichikawa wrote: > > > On 2017/04/26 08:40:57, Hiroshi Ichikawa wrote: > > > > Can you add a comment that this must not be called before > > > > -translationController:didFinishLanguageDetectionWithResult:error: is > > called? > > > > Also, can you document what happens if I call it before? Error? No-op? > > > > > > > > Also, can you document whether we can call this method twice before the > > first > > > > one completes? It can be TODO if you are not sure. > > > > > > Reminder: This hasn't been resolved yet. > > > > Done. > > Looks like this part of the comment hasn't been resolved yet: > > > > Also, can you document whether we can call this method twice before > the first > > > one completes? It can be TODO if you are not sure. Done.
lgtm
On 2017/04/27 07:28:10, Hiroshi Ichikawa wrote: > lgtm appreciate it. I'll wait for eugene/michael's LGTM. In the mean time, I'll find out about those TODOs.
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, readonly) CWVTranslationLanguage* targetLanguage; On 2017/04/27 04:54:01, jzw1 wrote: > On 2017/04/27 02:41:54, Hiroshi Ichikawa wrote: > > On 2017/04/26 20:15:30, Eugene But wrote: > > > Should this be something like, suggestedTranslationLanguage? target language > > is > > > ambiguous. > > > > I believe suggestedTranslationLanguage is also ambiguous, not clear whether > it's > > a language to translate from or translate into. Not really sure what is the > good > > name for it, but some ideas: > > > > suggestedTargetLanguage > > suggestedTranslationTargetLanguage > > estimatedUserLanguage > > I will rename sourceLanguage to pageLanguage and targetLanguage to OSLanguage. is targetLanguage always the same as OS language? If so, do we want to have it in our API (i.e. iOS SDK has API for getting OS language)? If no, then we should probably use different terminology than OSLanguage (|suggestedTranslationTargetLanguage| sounds like a good name to me). https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:16: typedef NS_ENUM(NSInteger, CWVTranslationError) { On 2017/04/27 04:54:02, jzw1 wrote: > On 2017/04/26 20:15:31, Eugene But wrote: > > Is there a need for enum? Should these constants be just integers? > > I'm just copying the error codes over from translate::TranslateErrors::Type Looks like iOS SDK do not use named enums and use something like this: |NS_ENUM(NSInteger)|. Do you want to use their approach? We don't have a goal to copy translate::TranslateErrors::Type. Right? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:18: CWVTranslationErrorNone = 0, On 2017/04/27 04:54:02, jzw1 wrote: > On 2017/04/26 20:15:30, Eugene But wrote: > > Is there a need for this item? This error code can never be used, right? > > I'm just copying the error codes over from translate::TranslateErrors::Type That code is C++ code and 0 in C++ means no error. In Objective-C nil NSError is used for non error cases so it sounds like we don't need this error code. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:20: CWVTranslationErrorNetwork, On 2017/04/27 04:54:02, jzw1 wrote: > On 2017/04/26 20:15:31, Eugene But wrote: > > Normally large negative numbers are used for error codes. Do you want to use > > one? > > I'm just copying the error codes over from translate::TranslateErrors::Type If the goal is to use the same error codes as translate::TranslateErrors::Type, then I think we can't just assume that translate::TranslateErrors::Type enum is stable (i.e. someone can add an error code to translate::TranslateErrors::Type and it will break this code). How about doing this: in the header: extern NSInteger CWVTranslationErrorInitializationError; in the implementation NSInteger CWVTranslationErrorInitializationError = translate::TranslateErrors::Type::INITIALIZATION_ERROR https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_language.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_language.h:20: // The language name in the current OS locale. On 2017/04/27 04:54:02, jzw1 wrote: > On 2017/04/26 20:15:31, Eugene But wrote: > > How about "Localized language name"? > > Done. I think "The language name in the current OS locale." means a different thing that "Localized language name". Locale is not a language. I suggested "Localized language name" to follow NSLocalizedString naming pattern. https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_language_detection_result.mm (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_language_detection_result.mm:28: _supportedLanguages = supportedLanguages; [supportedLanguages copy] ? https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller_internal.h (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:20: @property(nonatomic, assign) web::WebState* webState; Could you please write comment for these methods. They are non-trivial and per Style Guide all non-trivial methods need comments. https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:86: // TODO(jzw): Document what happens if you call this out of order or many times. In iOS code we use TODO(crbug.com/xxx) format, because people can leave but the bugs stay :) https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/shell/tra... File ios/web_view/shell/translate_delegate.h (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/shell/tra... ios/web_view/shell/translate_delegate.h:11: @interface TranslateDelegate : NSObject<CWVTranslationControllerDelegate> Should this be TranslationDelegate?
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, readonly) CWVTranslationLanguage* targetLanguage; On 2017/04/27 13:08:08, Eugene But wrote: > On 2017/04/27 04:54:01, jzw1 wrote: > > On 2017/04/27 02:41:54, Hiroshi Ichikawa wrote: > > > On 2017/04/26 20:15:30, Eugene But wrote: > > > > Should this be something like, suggestedTranslationLanguage? target > language > > > is > > > > ambiguous. > > > > > > I believe suggestedTranslationLanguage is also ambiguous, not clear whether > > it's > > > a language to translate from or translate into. Not really sure what is the > > good > > > name for it, but some ideas: > > > > > > suggestedTargetLanguage > > > suggestedTranslationTargetLanguage > > > estimatedUserLanguage > > > > I will rename sourceLanguage to pageLanguage and targetLanguage to OSLanguage. > > is targetLanguage always the same as OS language? If so, do we want to have it > in our API (i.e. iOS SDK has API for getting OS language)? If no, then we should > probably use different terminology than OSLanguage > (|suggestedTranslationTargetLanguage| sounds like a good name to me). Internally it returns the first non empty lang in the following order: 1. Override UI Language. (probably just debug?) 2. ULP Language? 3. Browser UI langauge. (OS Language) 4. First language in all accepted languages. 5. Nothing at all. So as of right now, it'll only be the OS Language, unless we expose some ways to provide #1 or #2. I feel that providing the OS Language is just a convenience for the client so they don't have to create their own CWVTranslationLanguage object. wdyt? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:16: typedef NS_ENUM(NSInteger, CWVTranslationError) { On 2017/04/27 13:08:08, Eugene But wrote: > On 2017/04/27 04:54:02, jzw1 wrote: > > On 2017/04/26 20:15:31, Eugene But wrote: > > > Is there a need for enum? Should these constants be just integers? > > > > I'm just copying the error codes over from translate::TranslateErrors::Type > > Looks like iOS SDK do not use named enums and use something like this: > |NS_ENUM(NSInteger)|. Do you want to use their approach? > We don't have a goal to copy translate::TranslateErrors::Type. Right? I'll get rid of the name. Since any of these errors could be returned by the underlying C++ code, we should just pass them on to the client right? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:18: CWVTranslationErrorNone = 0, On 2017/04/27 13:08:08, Eugene But wrote: > On 2017/04/27 04:54:02, jzw1 wrote: > > On 2017/04/26 20:15:30, Eugene But wrote: > > > Is there a need for this item? This error code can never be used, right? > > > > I'm just copying the error codes over from translate::TranslateErrors::Type > That code is C++ code and 0 in C++ means no error. In Objective-C nil NSError is > used for non error cases so it sounds like we don't need this error code. Done. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:20: CWVTranslationErrorNetwork, On 2017/04/27 13:08:08, Eugene But wrote: > On 2017/04/27 04:54:02, jzw1 wrote: > > On 2017/04/26 20:15:31, Eugene But wrote: > > > Normally large negative numbers are used for error codes. Do you want to use > > > one? > > > > I'm just copying the error codes over from translate::TranslateErrors::Type > If the goal is to use the same error codes as translate::TranslateErrors::Type, > then I think we can't just assume that translate::TranslateErrors::Type enum is > stable (i.e. someone can add an error code to translate::TranslateErrors::Type > and it will break this code). > > How about doing this: > > in the header: > extern NSInteger CWVTranslationErrorInitializationError; > > in the implementation > NSInteger CWVTranslationErrorInitializationError = > translate::TranslateErrors::Type::INITIALIZATION_ERROR > > Good point. I'll make this change. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_language.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_language.h:20: // The language name in the current OS locale. On 2017/04/27 13:08:08, Eugene But wrote: > On 2017/04/27 04:54:02, jzw1 wrote: > > On 2017/04/26 20:15:31, Eugene But wrote: > > > How about "Localized language name"? > > > > Done. > I think "The language name in the current OS locale." means a different thing > that "Localized language name". Locale is not a language. I suggested "Localized > language name" to follow NSLocalizedString naming pattern. I think what you are saying is you prefer localizedLanguageName over localizedName, yes? https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_language_detection_result.mm (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_language_detection_result.mm:28: _supportedLanguages = supportedLanguages; On 2017/04/27 13:08:08, Eugene But wrote: > [supportedLanguages copy] ? it was copied on its way in, do you prefer to have it in here? https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller_internal.h (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:20: @property(nonatomic, assign) web::WebState* webState; On 2017/04/27 13:08:08, Eugene But wrote: > Could you please write comment for these methods. They are non-trivial and per > Style Guide all non-trivial methods need comments. Done. https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:86: // TODO(jzw): Document what happens if you call this out of order or many times. On 2017/04/27 13:08:08, Eugene But wrote: > In iOS code we use TODO(crbug.com/xxx) format, because people can leave but the > bugs stay :) Done. https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/shell/tra... File ios/web_view/shell/translate_delegate.h (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/shell/tra... ios/web_view/shell/translate_delegate.h:11: @interface TranslateDelegate : NSObject<CWVTranslationControllerDelegate> On 2017/04/27 13:08:09, Eugene But wrote: > Should this be TranslationDelegate? Done.
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_language.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_language.h:20: // The language name in the current OS locale. On 2017/04/28 02:34:53, jzw1 wrote: > On 2017/04/27 13:08:08, Eugene But wrote: > > On 2017/04/27 04:54:02, jzw1 wrote: > > > On 2017/04/26 20:15:31, Eugene But wrote: > > > > How about "Localized language name"? > > > > > > Done. > > I think "The language name in the current OS locale." means a different thing > > that "Localized language name". Locale is not a language. I suggested > "Localized > > language name" to follow NSLocalizedString naming pattern. > > I think what you are saying is you prefer localizedLanguageName over > localizedName, yes? I believe Eugene is talking about the comment, not the property name. His suggestion is to write "Localized language name" instead of "The language name in the current OS locale.".
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, readonly) CWVTranslationLanguage* targetLanguage; On 2017/04/28 02:34:53, jzw1 wrote: > On 2017/04/27 13:08:08, Eugene But wrote: > > On 2017/04/27 04:54:01, jzw1 wrote: > > > On 2017/04/27 02:41:54, Hiroshi Ichikawa wrote: > > > > On 2017/04/26 20:15:30, Eugene But wrote: > > > > > Should this be something like, suggestedTranslationLanguage? target > > language > > > > is > > > > > ambiguous. > > > > > > > > I believe suggestedTranslationLanguage is also ambiguous, not clear > whether > > > it's > > > > a language to translate from or translate into. Not really sure what is > the > > > good > > > > name for it, but some ideas: > > > > > > > > suggestedTargetLanguage > > > > suggestedTranslationTargetLanguage > > > > estimatedUserLanguage > > > > > > I will rename sourceLanguage to pageLanguage and targetLanguage to > OSLanguage. > > > > is targetLanguage always the same as OS language? If so, do we want to have it > > in our API (i.e. iOS SDK has API for getting OS language)? If no, then we > should > > probably use different terminology than OSLanguage > > (|suggestedTranslationTargetLanguage| sounds like a good name to me). > > Internally it returns the first non empty lang in the following order: > 1. Override UI Language. (probably just debug?) > 2. ULP Language? > 3. Browser UI langauge. (OS Language) > 4. First language in all accepted languages. > 5. Nothing at all. > > So as of right now, it'll only be the OS Language, unless we expose some ways to > provide #1 or #2. I feel that providing the OS Language is just a convenience > for the client so they don't have to create their own CWVTranslationLanguage > object. wdyt? So looks like the fact that result will be OS language is just an implementation detail, which may change in the future. With that do you think we should use OSLanguage as a name? https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:16: typedef NS_ENUM(NSInteger, CWVTranslationError) { On 2017/04/28 02:34:53, jzw1 wrote: > On 2017/04/27 13:08:08, Eugene But wrote: > > On 2017/04/27 04:54:02, jzw1 wrote: > > > On 2017/04/26 20:15:31, Eugene But wrote: > > > > Is there a need for enum? Should these constants be just integers? > > > > > > I'm just copying the error codes over from translate::TranslateErrors::Type > > > > Looks like iOS SDK do not use named enums and use something like this: > > |NS_ENUM(NSInteger)|. Do you want to use their approach? > > We don't have a goal to copy translate::TranslateErrors::Type. Right? > > I'll get rid of the name. Since any of these errors could be returned by the > underlying C++ code, we should just pass them on to the client right? Sure. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_language.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_language.h:20: // The language name in the current OS locale. On 2017/04/28 02:34:53, jzw1 wrote: > On 2017/04/27 13:08:08, Eugene But wrote: > > On 2017/04/27 04:54:02, jzw1 wrote: > > > On 2017/04/26 20:15:31, Eugene But wrote: > > > > How about "Localized language name"? > > > > > > Done. > > I think "The language name in the current OS locale." means a different thing > > that "Localized language name". Locale is not a language. I suggested > "Localized > > language name" to follow NSLocalizedString naming pattern. > > I think what you are saying is you prefer localizedLanguageName over > localizedName, yes? No. My comment is related to "The language name in the current OS locale.". Old and new method names look good to me. https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_language_detection_result.mm (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_language_detection_result.mm:28: _supportedLanguages = supportedLanguages; On 2017/04/28 02:34:54, jzw1 wrote: > On 2017/04/27 13:08:08, Eugene But wrote: > > [supportedLanguages copy] ? > > it was copied on its way in, do you prefer to have it in here? I don't think this code should make assumptions about calling code, so maybe copying NSArray is better: https://google.github.io/styleguide/objcguide.xml#Setters_copy_NSStrings https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:140: - (NSArray<CWVTranslationLanguage*>*)supportedLanguages { Please add predeclarations and comments for all private methods https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller_internal.h (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:23: // Called by WebViewTranslateClient to pass on translation state. In the comments please do not document who calls this method (this interface should not make such assumptions). Instead please explain the arguments and what the method does. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.h (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:44: translate::TranslateManager* GetTranslateManager() { Please be consistent with naming style GetTranslateManager vs. set_cwv_translation_controller https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:78: base::WeakNSObject<CWVTranslationController> cwv_translation_controller_; cwv_ prefix is unnecessary https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:72: if (!cwv_translation_controller_.get()) { no need for .get() https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:18: extern NSErrorDomain const kCWVTranslationErrorDomain; Per Style Guide we don't need k prefix for public constants? https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:19: // Possible error codes during translation. nit: Add a linebreak after kCWVTranslationErrorDomain? https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:27: // does not know. No need for linebreak. This code fits 80 symbols https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:68: // Exposes everything needed for translation features. nit: Do you want to explain what this class allows in more concrete way? https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/shell/tra... File ios/web_view/shell/translation_delegate.h (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/shell/tra... ios/web_view/shell/translation_delegate.h:11: @interface TranslationDelegate : NSObject<CWVTranslationControllerDelegate> Sorry, missed earlier. All classes in this directory are prefixes with Shell
PTAL https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, readonly) CWVTranslationLanguage* targetLanguage; On 2017/04/28 03:07:28, Eugene But wrote: > On 2017/04/28 02:34:53, jzw1 wrote: > > On 2017/04/27 13:08:08, Eugene But wrote: > > > On 2017/04/27 04:54:01, jzw1 wrote: > > > > On 2017/04/27 02:41:54, Hiroshi Ichikawa wrote: > > > > > On 2017/04/26 20:15:30, Eugene But wrote: > > > > > > Should this be something like, suggestedTranslationLanguage? target > > > language > > > > > is > > > > > > ambiguous. > > > > > > > > > > I believe suggestedTranslationLanguage is also ambiguous, not clear > > whether > > > > it's > > > > > a language to translate from or translate into. Not really sure what is > > the > > > > good > > > > > name for it, but some ideas: > > > > > > > > > > suggestedTargetLanguage > > > > > suggestedTranslationTargetLanguage > > > > > estimatedUserLanguage > > > > > > > > I will rename sourceLanguage to pageLanguage and targetLanguage to > > OSLanguage. > > > > > > is targetLanguage always the same as OS language? If so, do we want to have > it > > > in our API (i.e. iOS SDK has API for getting OS language)? If no, then we > > should > > > probably use different terminology than OSLanguage > > > (|suggestedTranslationTargetLanguage| sounds like a good name to me). > > > > Internally it returns the first non empty lang in the following order: > > 1. Override UI Language. (probably just debug?) > > 2. ULP Language? > > 3. Browser UI langauge. (OS Language) > > 4. First language in all accepted languages. > > 5. Nothing at all. > > > > So as of right now, it'll only be the OS Language, unless we expose some ways > to > > provide #1 or #2. I feel that providing the OS Language is just a convenience > > for the client so they don't have to create their own CWVTranslationLanguage > > object. wdyt? > So looks like the fact that result will be OS language is just an implementation > detail, which may change in the future. With that do you think we should use > OSLanguage as a name? renamed to suggestedTranslationLanguage and updated comments. https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_language.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_language.h:20: // The language name in the current OS locale. On 2017/04/28 03:07:28, Eugene But wrote: > On 2017/04/28 02:34:53, jzw1 wrote: > > On 2017/04/27 13:08:08, Eugene But wrote: > > > On 2017/04/27 04:54:02, jzw1 wrote: > > > > On 2017/04/26 20:15:31, Eugene But wrote: > > > > > How about "Localized language name"? > > > > > > > > Done. > > > I think "The language name in the current OS locale." means a different > thing > > > that "Localized language name". Locale is not a language. I suggested > > "Localized > > > language name" to follow NSLocalizedString naming pattern. > > > > I think what you are saying is you prefer localizedLanguageName over > > localizedName, yes? > No. My comment is related to "The language name in the current OS locale.". Old > and new method names look good to me. ahh ok sorry for understanding. ill update the comment and use the old name. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:140: - (NSArray<CWVTranslationLanguage*>*)supportedLanguages { On 2017/04/28 03:07:29, Eugene But wrote: > Please add predeclarations and comments for all private methods Added interface extension and declared commented methods at the top. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller_internal.h (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:23: // Called by WebViewTranslateClient to pass on translation state. On 2017/04/28 03:07:29, Eugene But wrote: > In the comments please do not document who calls this method (this interface > should not make such assumptions). Instead please explain the arguments and what > the method does. Done. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.h (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:44: translate::TranslateManager* GetTranslateManager() { On 2017/04/28 03:07:29, Eugene But wrote: > Please be consistent with naming style GetTranslateManager vs. > set_cwv_translation_controller Done. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:78: base::WeakNSObject<CWVTranslationController> cwv_translation_controller_; On 2017/04/28 03:07:29, Eugene But wrote: > cwv_ prefix is unnecessary Done. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:72: if (!cwv_translation_controller_.get()) { On 2017/04/28 03:07:29, Eugene But wrote: > no need for .get() Done. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:18: extern NSErrorDomain const kCWVTranslationErrorDomain; On 2017/04/28 03:07:29, Eugene But wrote: > Per Style Guide we don't need k prefix for public constants? Done https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:19: // Possible error codes during translation. On 2017/04/28 03:07:29, Eugene But wrote: > nit: Add a linebreak after kCWVTranslationErrorDomain? Done. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:27: // does not know. On 2017/04/28 03:07:29, Eugene But wrote: > No need for linebreak. This code fits 80 symbols Done. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:68: // Exposes everything needed for translation features. On 2017/04/28 03:07:29, Eugene But wrote: > nit: Do you want to explain what this class allows in more concrete way? Done. https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/shell/tra... File ios/web_view/shell/translation_delegate.h (right): https://codereview.chromium.org/2839093002/diff/120001/ios/web_view/shell/tra... ios/web_view/shell/translation_delegate.h:11: @interface TranslationDelegate : NSObject<CWVTranslationControllerDelegate> On 2017/04/28 03:07:29, Eugene But wrote: > Sorry, missed earlier. All classes in this directory are prefixes with Shell Done.
https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, readonly) CWVTranslationLanguage* targetLanguage; On 2017/04/28 05:43:35, jzw1 wrote: > On 2017/04/28 03:07:28, Eugene But wrote: > > On 2017/04/28 02:34:53, jzw1 wrote: > > > On 2017/04/27 13:08:08, Eugene But wrote: > > > > On 2017/04/27 04:54:01, jzw1 wrote: > > > > > On 2017/04/27 02:41:54, Hiroshi Ichikawa wrote: > > > > > > On 2017/04/26 20:15:30, Eugene But wrote: > > > > > > > Should this be something like, suggestedTranslationLanguage? target > > > > language > > > > > > is > > > > > > > ambiguous. > > > > > > > > > > > > I believe suggestedTranslationLanguage is also ambiguous, not clear > > > whether > > > > > it's > > > > > > a language to translate from or translate into. Not really sure what > is > > > the > > > > > good > > > > > > name for it, but some ideas: > > > > > > > > > > > > suggestedTargetLanguage > > > > > > suggestedTranslationTargetLanguage > > > > > > estimatedUserLanguage > > > > > > > > > > I will rename sourceLanguage to pageLanguage and targetLanguage to > > > OSLanguage. > > > > > > > > is targetLanguage always the same as OS language? If so, do we want to > have > > it > > > > in our API (i.e. iOS SDK has API for getting OS language)? If no, then we > > > should > > > > probably use different terminology than OSLanguage > > > > (|suggestedTranslationTargetLanguage| sounds like a good name to me). > > > > > > Internally it returns the first non empty lang in the following order: > > > 1. Override UI Language. (probably just debug?) > > > 2. ULP Language? > > > 3. Browser UI langauge. (OS Language) > > > 4. First language in all accepted languages. > > > 5. Nothing at all. > > > > > > So as of right now, it'll only be the OS Language, unless we expose some > ways > > to > > > provide #1 or #2. I feel that providing the OS Language is just a > convenience > > > for the client so they don't have to create their own CWVTranslationLanguage > > > object. wdyt? > > So looks like the fact that result will be OS language is just an > implementation > > detail, which may change in the future. With that do you think we should use > > OSLanguage as a name? > > renamed to suggestedTranslationLanguage and updated comments. As I told in earlier comment, I feel suggestedTranslationLanguage is still a bit ambiguous, and prefer suggestedTranslationTargetLanguage. But it's up to you.
On 2017/04/28 05:50:06, Hiroshi Ichikawa wrote: > https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... > File ios/web_view/public/cwv_language_detection_result.h (right): > > https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... > ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, > readonly) CWVTranslationLanguage* targetLanguage; > On 2017/04/28 05:43:35, jzw1 wrote: > > On 2017/04/28 03:07:28, Eugene But wrote: > > > On 2017/04/28 02:34:53, jzw1 wrote: > > > > On 2017/04/27 13:08:08, Eugene But wrote: > > > > > On 2017/04/27 04:54:01, jzw1 wrote: > > > > > > On 2017/04/27 02:41:54, Hiroshi Ichikawa wrote: > > > > > > > On 2017/04/26 20:15:30, Eugene But wrote: > > > > > > > > Should this be something like, suggestedTranslationLanguage? > target > > > > > language > > > > > > > is > > > > > > > > ambiguous. > > > > > > > > > > > > > > I believe suggestedTranslationLanguage is also ambiguous, not clear > > > > whether > > > > > > it's > > > > > > > a language to translate from or translate into. Not really sure what > > is > > > > the > > > > > > good > > > > > > > name for it, but some ideas: > > > > > > > > > > > > > > suggestedTargetLanguage > > > > > > > suggestedTranslationTargetLanguage > > > > > > > estimatedUserLanguage > > > > > > > > > > > > I will rename sourceLanguage to pageLanguage and targetLanguage to > > > > OSLanguage. > > > > > > > > > > is targetLanguage always the same as OS language? If so, do we want to > > have > > > it > > > > > in our API (i.e. iOS SDK has API for getting OS language)? If no, then > we > > > > should > > > > > probably use different terminology than OSLanguage > > > > > (|suggestedTranslationTargetLanguage| sounds like a good name to me). > > > > > > > > Internally it returns the first non empty lang in the following order: > > > > 1. Override UI Language. (probably just debug?) > > > > 2. ULP Language? > > > > 3. Browser UI langauge. (OS Language) > > > > 4. First language in all accepted languages. > > > > 5. Nothing at all. > > > > > > > > So as of right now, it'll only be the OS Language, unless we expose some > > ways > > > to > > > > provide #1 or #2. I feel that providing the OS Language is just a > > convenience > > > > for the client so they don't have to create their own > CWVTranslationLanguage > > > > object. wdyt? > > > So looks like the fact that result will be OS language is just an > > implementation > > > detail, which may change in the future. With that do you think we should use > > > OSLanguage as a name? > > > > renamed to suggestedTranslationLanguage and updated comments. > > As I told in earlier comment, I feel suggestedTranslationLanguage is still a bit > ambiguous, and prefer suggestedTranslationTargetLanguage. But it's up to you. How about your other suggestion "suggestedTargetLanguage"? I feel adding "translation" in the name is a little redundant, but I will make sure to mention translation in the comments.
On 2017/04/28 05:58:08, jzw1 wrote: > On 2017/04/28 05:50:06, Hiroshi Ichikawa wrote: > > > https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... > > File ios/web_view/public/cwv_language_detection_result.h (right): > > > > > https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... > > ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, > > readonly) CWVTranslationLanguage* targetLanguage; > > On 2017/04/28 05:43:35, jzw1 wrote: > > > On 2017/04/28 03:07:28, Eugene But wrote: > > > > On 2017/04/28 02:34:53, jzw1 wrote: > > > > > On 2017/04/27 13:08:08, Eugene But wrote: > > > > > > On 2017/04/27 04:54:01, jzw1 wrote: > > > > > > > On 2017/04/27 02:41:54, Hiroshi Ichikawa wrote: > > > > > > > > On 2017/04/26 20:15:30, Eugene But wrote: > > > > > > > > > Should this be something like, suggestedTranslationLanguage? > > target > > > > > > language > > > > > > > > is > > > > > > > > > ambiguous. > > > > > > > > > > > > > > > > I believe suggestedTranslationLanguage is also ambiguous, not > clear > > > > > whether > > > > > > > it's > > > > > > > > a language to translate from or translate into. Not really sure > what > > > is > > > > > the > > > > > > > good > > > > > > > > name for it, but some ideas: > > > > > > > > > > > > > > > > suggestedTargetLanguage > > > > > > > > suggestedTranslationTargetLanguage > > > > > > > > estimatedUserLanguage > > > > > > > > > > > > > > I will rename sourceLanguage to pageLanguage and targetLanguage to > > > > > OSLanguage. > > > > > > > > > > > > is targetLanguage always the same as OS language? If so, do we want to > > > have > > > > it > > > > > > in our API (i.e. iOS SDK has API for getting OS language)? If no, then > > we > > > > > should > > > > > > probably use different terminology than OSLanguage > > > > > > (|suggestedTranslationTargetLanguage| sounds like a good name to me). > > > > > > > > > > Internally it returns the first non empty lang in the following order: > > > > > 1. Override UI Language. (probably just debug?) > > > > > 2. ULP Language? > > > > > 3. Browser UI langauge. (OS Language) > > > > > 4. First language in all accepted languages. > > > > > 5. Nothing at all. > > > > > > > > > > So as of right now, it'll only be the OS Language, unless we expose some > > > ways > > > > to > > > > > provide #1 or #2. I feel that providing the OS Language is just a > > > convenience > > > > > for the client so they don't have to create their own > > CWVTranslationLanguage > > > > > object. wdyt? > > > > So looks like the fact that result will be OS language is just an > > > implementation > > > > detail, which may change in the future. With that do you think we should > use > > > > OSLanguage as a name? > > > > > > renamed to suggestedTranslationLanguage and updated comments. > > > > As I told in earlier comment, I feel suggestedTranslationLanguage is still a > bit > > ambiguous, and prefer suggestedTranslationTargetLanguage. But it's up to you. > > How about your other suggestion "suggestedTargetLanguage"? I feel adding > "translation" in the name is a little redundant, but I will make sure to mention > translation in the comments. +1 assuming "target language" is clear enough to mean "the language it translates into" in the context of translation (anyway the term is used in many other locations), but it's up to Eugene.
On 2017/04/28 06:02:13, Hiroshi Ichikawa wrote: > On 2017/04/28 05:58:08, jzw1 wrote: > > On 2017/04/28 05:50:06, Hiroshi Ichikawa wrote: > > > > > > https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... > > > File ios/web_view/public/cwv_language_detection_result.h (right): > > > > > > > > > https://codereview.chromium.org/2839093002/diff/40001/ios/web_view/public/cwv... > > > ios/web_view/public/cwv_language_detection_result.h:28: @property(nonatomic, > > > readonly) CWVTranslationLanguage* targetLanguage; > > > On 2017/04/28 05:43:35, jzw1 wrote: > > > > On 2017/04/28 03:07:28, Eugene But wrote: > > > > > On 2017/04/28 02:34:53, jzw1 wrote: > > > > > > On 2017/04/27 13:08:08, Eugene But wrote: > > > > > > > On 2017/04/27 04:54:01, jzw1 wrote: > > > > > > > > On 2017/04/27 02:41:54, Hiroshi Ichikawa wrote: > > > > > > > > > On 2017/04/26 20:15:30, Eugene But wrote: > > > > > > > > > > Should this be something like, suggestedTranslationLanguage? > > > target > > > > > > > language > > > > > > > > > is > > > > > > > > > > ambiguous. > > > > > > > > > > > > > > > > > > I believe suggestedTranslationLanguage is also ambiguous, not > > clear > > > > > > whether > > > > > > > > it's > > > > > > > > > a language to translate from or translate into. Not really sure > > what > > > > is > > > > > > the > > > > > > > > good > > > > > > > > > name for it, but some ideas: > > > > > > > > > > > > > > > > > > suggestedTargetLanguage > > > > > > > > > suggestedTranslationTargetLanguage > > > > > > > > > estimatedUserLanguage > > > > > > > > > > > > > > > > I will rename sourceLanguage to pageLanguage and targetLanguage to > > > > > > OSLanguage. > > > > > > > > > > > > > > is targetLanguage always the same as OS language? If so, do we want > to > > > > have > > > > > it > > > > > > > in our API (i.e. iOS SDK has API for getting OS language)? If no, > then > > > we > > > > > > should > > > > > > > probably use different terminology than OSLanguage > > > > > > > (|suggestedTranslationTargetLanguage| sounds like a good name to > me). > > > > > > > > > > > > Internally it returns the first non empty lang in the following order: > > > > > > 1. Override UI Language. (probably just debug?) > > > > > > 2. ULP Language? > > > > > > 3. Browser UI langauge. (OS Language) > > > > > > 4. First language in all accepted languages. > > > > > > 5. Nothing at all. > > > > > > > > > > > > So as of right now, it'll only be the OS Language, unless we expose > some > > > > ways > > > > > to > > > > > > provide #1 or #2. I feel that providing the OS Language is just a > > > > convenience > > > > > > for the client so they don't have to create their own > > > CWVTranslationLanguage > > > > > > object. wdyt? > > > > > So looks like the fact that result will be OS language is just an > > > > implementation > > > > > detail, which may change in the future. With that do you think we should > > use > > > > > OSLanguage as a name? > > > > > > > > renamed to suggestedTranslationLanguage and updated comments. > > > > > > As I told in earlier comment, I feel suggestedTranslationLanguage is still a > > bit > > > ambiguous, and prefer suggestedTranslationTargetLanguage. But it's up to > you. > > > > How about your other suggestion "suggestedTargetLanguage"? I feel adding > > "translation" in the name is a little redundant, but I will make sure to > mention > > translation in the comments. > > +1 assuming "target language" is clear enough to mean "the language it > translates into" in the context of translation (anyway the term is used in many > other locations), but it's up to Eugene. suggestedTargetLanguage is fine as long as the comments explain the property. I will review this CL shortly.
lgtm! Thank you for the patience. https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_language_detection_result.mm (right): https://codereview.chromium.org/2839093002/diff/100001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_language_detection_result.mm:28: _supportedLanguages = supportedLanguages; On 2017/04/28 03:07:28, Eugene But wrote: > On 2017/04/28 02:34:54, jzw1 wrote: > > On 2017/04/27 13:08:08, Eugene But wrote: > > > [supportedLanguages copy] ? > > > > it was copied on its way in, do you prefer to have it in here? > I don't think this code should make assumptions about calling code, so maybe > copying NSArray is better: > https://google.github.io/styleguide/objcguide.xml#Setters_copy_NSStrings Please look at this comment https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:45: // Private interface. nit: Do we need this comment? Seems obvious from the code. https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller_internal.h (right): https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:18: @interface CWVTranslationController () nit: Should this be |CWVTranslationController (Internal)|? https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.h (right): https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:44: translate::TranslateManager* get_translate_manager() { s/get_translate_manager/translate_manager https://google.github.io/styleguide/cppguide.html#Function_Names https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:72: if (!translation_controller_) { There is no need for this check calling method on nil is totally safe in Objective-C
https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:45: // Private interface. On 2017/04/28 07:18:07, Eugene But wrote: > nit: Do we need this comment? Seems obvious from the code. haha i guess not. https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller_internal.h (right): https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:18: @interface CWVTranslationController () On 2017/04/28 07:18:07, Eugene But wrote: > nit: Should this be |CWVTranslationController (Internal)|? No. I believe we want it to be an extension since they are used to extend the internal implementation, where as categories are used to add things not critical to the internal implementation. https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... Also, I was following existing examples in ios/web_view. https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.h (right): https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:44: translate::TranslateManager* get_translate_manager() { On 2017/04/28 07:18:07, Eugene But wrote: > s/get_translate_manager/translate_manager > > https://google.github.io/styleguide/cppguide.html#Function_Names Done, thanks for link. https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.mm (right): https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.mm:72: if (!translation_controller_) { On 2017/04/28 07:18:07, Eugene But wrote: > There is no need for this check calling method on nil is totally safe in > Objective-C I see. I need to read more about these wrapper classes :P
On 2017/04/28 09:55:33, jzw1 wrote: > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > File ios/web_view/internal/translate/cwv_translation_controller.mm (right): > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller.mm:45: // Private > interface. > On 2017/04/28 07:18:07, Eugene But wrote: > > nit: Do we need this comment? Seems obvious from the code. > > haha i guess not. > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > File ios/web_view/internal/translate/cwv_translation_controller_internal.h > (right): > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > ios/web_view/internal/translate/cwv_translation_controller_internal.h:18: > @interface CWVTranslationController () > On 2017/04/28 07:18:07, Eugene But wrote: > > nit: Should this be |CWVTranslationController (Internal)|? > > No. I believe we want it to be an extension since they are used to extend the > internal implementation, where as categories are used to add things not critical > to the internal implementation. > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... > Also, I was following existing examples in ios/web_view. > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > File ios/web_view/internal/translate/web_view_translate_client.h (right): > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > ios/web_view/internal/translate/web_view_translate_client.h:44: > translate::TranslateManager* get_translate_manager() { > On 2017/04/28 07:18:07, Eugene But wrote: > > s/get_translate_manager/translate_manager > > > > https://google.github.io/styleguide/cppguide.html#Function_Names > > Done, thanks for link. > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > File ios/web_view/internal/translate/web_view_translate_client.mm (right): > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > ios/web_view/internal/translate/web_view_translate_client.mm:72: if > (!translation_controller_) { > On 2017/04/28 07:18:07, Eugene But wrote: > > There is no need for this check calling method on nil is totally safe in > > Objective-C > > I see. I need to read more about these wrapper classes :P appreciate the review Eugene! Michael, would you like to do a pass too?
On 2017/04/28 09:56:45, jzw1 wrote: > On 2017/04/28 09:55:33, jzw1 wrote: > > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > > File ios/web_view/internal/translate/cwv_translation_controller.mm (right): > > > > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > > ios/web_view/internal/translate/cwv_translation_controller.mm:45: // Private > > interface. > > On 2017/04/28 07:18:07, Eugene But wrote: > > > nit: Do we need this comment? Seems obvious from the code. > > > > haha i guess not. > > > > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > > File ios/web_view/internal/translate/cwv_translation_controller_internal.h > > (right): > > > > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > > ios/web_view/internal/translate/cwv_translation_controller_internal.h:18: > > @interface CWVTranslationController () > > On 2017/04/28 07:18:07, Eugene But wrote: > > > nit: Should this be |CWVTranslationController (Internal)|? > > > > No. I believe we want it to be an extension since they are used to extend the > > internal implementation, where as categories are used to add things not > critical > > to the internal implementation. > > > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... > > Also, I was following existing examples in ios/web_view. > > > > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > > File ios/web_view/internal/translate/web_view_translate_client.h (right): > > > > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > > ios/web_view/internal/translate/web_view_translate_client.h:44: > > translate::TranslateManager* get_translate_manager() { > > On 2017/04/28 07:18:07, Eugene But wrote: > > > s/get_translate_manager/translate_manager > > > > > > https://google.github.io/styleguide/cppguide.html#Function_Names > > > > Done, thanks for link. > > > > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > > File ios/web_view/internal/translate/web_view_translate_client.mm (right): > > > > > https://codereview.chromium.org/2839093002/diff/180001/ios/web_view/internal/... > > ios/web_view/internal/translate/web_view_translate_client.mm:72: if > > (!translation_controller_) { > > On 2017/04/28 07:18:07, Eugene But wrote: > > > There is no need for this check calling method on nil is totally safe in > > > Objective-C > > > > I see. I need to read more about these wrapper classes :P > > appreciate the review Eugene! Michael, would you like to do a pass too? I think Mike will be able to look at this on Monday.
Sorry for the delay, I wasn't able to get to this last week. This looks great overall. I did a pass through everything with only a few major comments. I did the best I could to catch up on prior discussions but let me know if I am contradicting any prior decision with my comments. Thank you! https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:88: @synthesize translationController = _translationController; Please move up to sort alphabetically. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_language_detection_result.mm (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_language_detection_result.mm:5: #import "ios/web_view/public/cwv_language_detection_result.h" No need for this import. _internal.h import is OK, but please add space after it so it matches the style of the primary header import. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:5: #import "ios/web_view/public/cwv_translation_controller.h" nit: You can replace this import with _internal.h file. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:22: NSErrorDomain const CWVTranslationErrorDomain = @"com.cwv.translation.errors"; It looks like Chrome uses Error domains of the style "ChromeXYZError". However since this is a library, I like the reverse DNS style, but we should use a better name as we don't own "cwv dot com" domain name. (Written out to prevent auto-linking.) What do you think about "org.chromium.chromewebview.TranslationErrorDomain"? https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:72: _translateClient->set_translation_controller(self); Do we need to do any cleanup here if webState was already set. Or at least we should document in the header that this should only be set once? https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:138: #pragma mark - Public Methodse s/Methodse/Methods https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:142: _translateUIDelegate->Translate(); It looks like these parameters are dropped. This won't allow client to fix mis-detected language or to translate to a language other than the assumed target language. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller_internal.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:10: #include "components/translate/core/browser/translate_step.h" Please replace translate_step and web_state with "class" forward declarations. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:13: #import "ios/web_view/internal/translate/web_view_translate_client.h" Is web_view_translate_client.h import needed? https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_language.mm (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_language.mm:9: #import "ios/web_view/internal/translate/cwv_translation_language_internal.h" We can replace the import of cwv_translation_language.h on line 5 with the internal import. (This is what we do in cwv_website_data_store.mm) https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:31: @class CWVTranslationController; Please move above class PrefService above to line 18. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:41: translation_controller_.reset(controller); Is the assumption in the removed comment about the controller outliving this WebViewTranslateClient still valid? https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:78: base::WeakNSObject<CWVTranslationController> translation_controller_; Please add comment https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_language_detection_result.h:22: // Can be used to translate to |suggestedTargetLanguage|. No need for second line of comment. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_language_detection_result.h:25: // The suggested language to translate to. What about this specifying where this comes from. For example: The suggested target translation language based on the user's device language. Please feel free to be more specific though, I didn't check where this value is filled in from. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_language_detection_result.h:26: // Can be used to translate from |detectedPageLanguage|. No need for second line of comment. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_language_detection_result.h:30: @property(nonatomic, readonly) copy attribute here? https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:44: @protocol CWVTranslationControllerDelegate<NSObject> Please move delegate definition to it's own file. Ref: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:49: // |error| will be filled if language detection failed. s/filled/nonnull https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:51: didFinishLanguageDetectionWithResult:(CWVLanguageDetectionResult*)result I think didFinishLanguageDetectionWithResult should be required, because below comment on translatePageFromLanguage:toLanguage: states the client can't call that method until after receiving this method. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:59: // Called when translation finishes. |error| will be filled if it failed. s/filled/nonnull https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:67: // Main interface for access to translation features. "Main interface" sounds like there are other non-main interfaces. What about simplifying all 3 lines of comment to just this line: // Allows page translation from one language to another. The revert ability and the delegate can be clearly seen from the header methods so I don't think we need to explain in the comments. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_web_view.h:11: @class CWVTranslationController; Please move up to sort alphabetically. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... File ios/web_view/shell/shell_translation_delegate.m (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... ios/web_view/shell/shell_translation_delegate.m:7: #import <ChromeWebView/ChromeWebView.h> Can we remove ChromeWebView import, it is already imported in header? https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... ios/web_view/shell/shell_translation_delegate.m:41: self.beforeTranslateActionSheet = nil; Please use __weak self reference inside action handler because self holds strong reference to this alert. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... ios/web_view/shell/shell_translation_delegate.m:49: self.beforeTranslateActionSheet = nil; Please use __weak self reference inside action handler because self holds strong reference to this alert. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... ios/web_view/shell/shell_translation_delegate.m:62: - (void)translationController:(CWVTranslationController*)controller No need for empty implementations because these delegate methods are optional.
https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:88: @synthesize translationController = _translationController; On 2017/05/01 18:25:36, michaeldo wrote: > Please move up to sort alphabetically. Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_language_detection_result.mm (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_language_detection_result.mm:5: #import "ios/web_view/public/cwv_language_detection_result.h" On 2017/05/01 18:25:36, michaeldo wrote: > No need for this import. _internal.h import is OK, but please add space after it > so it matches the style of the primary header import. Ah ok. Was wondering about that :) https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:5: #import "ios/web_view/public/cwv_translation_controller.h" On 2017/05/01 18:25:37, michaeldo wrote: > nit: You can replace this import with _internal.h file. Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:22: NSErrorDomain const CWVTranslationErrorDomain = @"com.cwv.translation.errors"; On 2017/05/01 18:25:37, michaeldo wrote: > It looks like Chrome uses Error domains of the style "ChromeXYZError". However > since this is a library, I like the reverse DNS style, but we should use a > better name as we don't own "cwv dot com" domain name. (Written out to prevent > auto-linking.) What do you think about > "org.chromium.chromewebview.TranslationErrorDomain"? Sounds good. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:72: _translateClient->set_translation_controller(self); On 2017/05/01 18:25:37, michaeldo wrote: > Do we need to do any cleanup here if webState was already set. Or at least we > should document in the header that this should only be set once? the webstate can be set again via CWVWebView#resetWebStateWithSessionStorage. I'm not entirely sure what cleanup needs to be done here, I'm hoping ARC takes care of it for me? https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:138: #pragma mark - Public Methodse On 2017/05/01 18:25:37, michaeldo wrote: > s/Methodse/Methods Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:142: _translateUIDelegate->Translate(); On 2017/05/01 18:25:37, michaeldo wrote: > It looks like these parameters are dropped. This won't allow client to fix > mis-detected language or to translate to a language other than the assumed > target language. Actually I just haven't used these yet. I'll add a TODO here though. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller_internal.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:10: #include "components/translate/core/browser/translate_step.h" On 2017/05/01 18:25:37, michaeldo wrote: > Please replace translate_step and web_state with "class" forward declarations. How do I forward declare the translate_step enum? I'm looking at translate_client.h and it seems they also import it directly. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:13: #import "ios/web_view/internal/translate/web_view_translate_client.h" On 2017/05/01 18:25:37, michaeldo wrote: > Is web_view_translate_client.h import needed? Thanks for catch https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_language.mm (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_language.mm:9: #import "ios/web_view/internal/translate/cwv_translation_language_internal.h" On 2017/05/01 18:25:37, michaeldo wrote: > We can replace the import of cwv_translation_language.h on line 5 with the > internal import. (This is what we do in cwv_website_data_store.mm) Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:31: @class CWVTranslationController; On 2017/05/01 18:25:37, michaeldo wrote: > Please move above class PrefService above to line 18. Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:41: translation_controller_.reset(controller); On 2017/05/01 18:25:37, michaeldo wrote: > Is the assumption in the removed comment about the controller outliving this > WebViewTranslateClient still valid? It is. I'll add it back in. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:78: base::WeakNSObject<CWVTranslationController> translation_controller_; On 2017/05/01 18:25:37, michaeldo wrote: > Please add comment Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_language_detection_result.h:22: // Can be used to translate to |suggestedTargetLanguage|. On 2017/05/01 18:25:37, michaeldo wrote: > No need for second line of comment. Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_language_detection_result.h:25: // The suggested language to translate to. On 2017/05/01 18:25:37, michaeldo wrote: > What about this specifying where this comes from. For example: > > The suggested target translation language based on the user's device language. > > Please feel free to be more specific though, I didn't check where this value is > filled in from. I believe Eugene didn't want to be too specific since this can change according to preferences. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_language_detection_result.h:26: // Can be used to translate from |detectedPageLanguage|. On 2017/05/01 18:25:37, michaeldo wrote: > No need for second line of comment. Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_language_detection_result.h:30: @property(nonatomic, readonly) On 2017/05/01 18:25:37, michaeldo wrote: > copy attribute here? Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:44: @protocol CWVTranslationControllerDelegate<NSObject> On 2017/05/01 18:25:38, michaeldo wrote: > Please move delegate definition to it's own file. > > Ref: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:49: // |error| will be filled if language detection failed. On 2017/05/01 18:25:38, michaeldo wrote: > s/filled/nonnull Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:51: didFinishLanguageDetectionWithResult:(CWVLanguageDetectionResult*)result On 2017/05/01 18:25:37, michaeldo wrote: > I think didFinishLanguageDetectionWithResult should be required, because below > comment on translatePageFromLanguage:toLanguage: states the client can't call > that method until after receiving this method. That's fair. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:59: // Called when translation finishes. |error| will be filled if it failed. On 2017/05/01 18:25:38, michaeldo wrote: > s/filled/nonnull Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:67: // Main interface for access to translation features. On 2017/05/01 18:25:38, michaeldo wrote: > "Main interface" sounds like there are other non-main interfaces. What about > simplifying all 3 lines of comment to just this line: > > // Allows page translation from one language to another. > > The revert ability and the delegate can be clearly seen from the header methods > so I don't think we need to explain in the comments. Fine by me. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_web_view.h:11: @class CWVTranslationController; On 2017/05/01 18:25:38, michaeldo wrote: > Please move up to sort alphabetically. Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... File ios/web_view/shell/shell_translation_delegate.m (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... ios/web_view/shell/shell_translation_delegate.m:7: #import <ChromeWebView/ChromeWebView.h> On 2017/05/01 18:25:38, michaeldo wrote: > Can we remove ChromeWebView import, it is already imported in header? Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... ios/web_view/shell/shell_translation_delegate.m:41: self.beforeTranslateActionSheet = nil; On 2017/05/01 18:25:38, michaeldo wrote: > Please use __weak self reference inside action handler because self holds strong > reference to this alert. Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... ios/web_view/shell/shell_translation_delegate.m:49: self.beforeTranslateActionSheet = nil; On 2017/05/01 18:25:38, michaeldo wrote: > Please use __weak self reference inside action handler because self holds strong > reference to this alert. Done. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/shell/she... ios/web_view/shell/shell_translation_delegate.m:62: - (void)translationController:(CWVTranslationController*)controller On 2017/05/01 18:25:38, michaeldo wrote: > No need for empty implementations because these delegate methods are optional. Done.
LGTM. Thank you very much! https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:72: _translateClient->set_translation_controller(self); On 2017/05/08 03:36:28, jzw1 wrote: > On 2017/05/01 18:25:37, michaeldo wrote: > > Do we need to do any cleanup here if webState was already set. Or at least we > > should document in the header that this should only be set once? > > the webstate can be set again via CWVWebView#resetWebStateWithSessionStorage. > I'm not entirely sure what cleanup needs to be done here, I'm hoping ARC takes > care of it for me? If the previous webState is still alive somewhere, |self| will still be set as the |translation_controller|. I think we should nil out the translation_controller if |_webState| is set at the top of this method AND if FromWebState returns a translateClient. (No need to create one, even though it almost certainly already exists.) It seems like this could lead to problems otherwise where we are set as the translation_controller for 2 web_states. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller.mm:142: _translateUIDelegate->Translate(); On 2017/05/08 03:36:28, jzw1 wrote: > On 2017/05/01 18:25:37, michaeldo wrote: > > It looks like these parameters are dropped. This won't allow client to fix > > mis-detected language or to translate to a language other than the assumed > > target language. > > Actually I just haven't used these yet. I'll add a TODO here though. Sounds good. Thank you. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/cwv_translation_controller_internal.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/cwv_translation_controller_internal.h:10: #include "components/translate/core/browser/translate_step.h" On 2017/05/08 03:36:28, jzw1 wrote: > On 2017/05/01 18:25:37, michaeldo wrote: > > Please replace translate_step and web_state with "class" forward declarations. > > How do I forward declare the translate_step enum? I'm looking at > translate_client.h and it seems they also import it directly. Sorry, didn't realize it was for the enum. You can't as far as I know, but if it forces "lots" of full includes, you can move the enum to it's own header. No need to do that now, but that is one possibility. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:41: translation_controller_.reset(controller); On 2017/05/08 03:36:28, jzw1 wrote: > On 2017/05/01 18:25:37, michaeldo wrote: > > Is the assumption in the removed comment about the controller outliving this > > WebViewTranslateClient still valid? > > It is. I'll add it back in. Thank you. https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... File ios/web_view/public/cwv_language_detection_result.h (right): https://codereview.chromium.org/2839093002/diff/200001/ios/web_view/public/cw... ios/web_view/public/cwv_language_detection_result.h:25: // The suggested language to translate to. On 2017/05/08 03:36:28, jzw1 wrote: > On 2017/05/01 18:25:37, michaeldo wrote: > > What about this specifying where this comes from. For example: > > > > The suggested target translation language based on the user's device language. > > > > Please feel free to be more specific though, I didn't check where this value > is > > filled in from. > > I believe Eugene didn't want to be too specific since this can change according > to preferences. Ok, no prob. That's a good point. https://codereview.chromium.org/2839093002/diff/220001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.h (right): https://codereview.chromium.org/2839093002/diff/220001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:40: // This |controller| will outlive this class. Keeping the original wording seems clearer to me: // |controller| is assumed to outlive this WebViewTranslateClient.
thanks everyone. https://codereview.chromium.org/2839093002/diff/220001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_client.h (right): https://codereview.chromium.org/2839093002/diff/220001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_client.h:40: // This |controller| will outlive this class. On 2017/05/08 15:15:48, michaeldo wrote: > Keeping the original wording seems clearer to me: > // |controller| is assumed to outlive this WebViewTranslateClient. Done.
The CQ bit was checked by jzw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ichikawa@chromium.org, eugenebut@chromium.org, michaeldo@chromium.org Link to the patchset: https://codereview.chromium.org/2839093002/#ps240001 (title: "changed comment")
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by jzw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeldo@chromium.org, eugenebut@chromium.org, ichikawa@chromium.org Link to the patchset: https://codereview.chromium.org/2839093002/#ps260001 (title: "merge")
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": 260001, "attempt_start_ts": 1494301906418880,
"parent_rev": "b15619b5317b2e975659641424723d8518c9046c", "commit_rev":
"140c4b6e2b2db009b9a82a6316497b919c5031fe"}
Message was sent while issue was closed.
Description was changed from ========== Implemented new Translate API for purely Objective-C clients. BUG=706289 ========== to ========== Implemented new Translate API for purely Objective-C clients. BUG=706289 Review-Url: https://codereview.chromium.org/2839093002 Cr-Commit-Position: refs/heads/master@{#470236} Committed: https://chromium.googlesource.com/chromium/src/+/140c4b6e2b2db009b9a82a631649... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/140c4b6e2b2db009b9a82a631649... |
