Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2983453002:
Expose way to enable/disable translate. (Closed)
DescriptionExpose way to enable/disable translate.
Review-Url: https://codereview.chromium.org/2983453002
Cr-Commit-Position: refs/heads/master@{#486833}
Committed: https://chromium.googlesource.com/chromium/src/+/03b800c37e9643a72abb82fb9dc1050648ce3aa1
Patch Set 1 #
Total comments: 5
Patch Set 2 : Refactor settings #
Total comments: 17
Patch Set 3 : Just wrap PrefService #Patch Set 4 : update ChromeWebView.h #Patch Set 5 : comment non obvious param #
Total comments: 5
Patch Set 6 : addressed michaeldo's comments #Messages
Total messages: 26 (6 generated)
jzw@chromium.org changed reviewers: + eugenebut@chromium.org, ichikawa@chromium.org, michaeldo@chromium.org
On 2017/07/13 06:33:32, jzw1 wrote: > mailto:jzw@chromium.org changed reviewers: > + mailto:eugenebut@chromium.org, mailto:ichikawa@chromium.org, mailto:michaeldo@chromium.org Hey guys, PTAL. I feel it's kind of weird having these as class methods and also mentioning that they only apply for non-incognito settings. That's why I originally put these methods on CWVWebViewConfiguration, but I also understand that it's weird not to put these methods on CWVTranslationController. How would you guys feel if I made it so these class methods takes a CWVWebViewConfiguration as an argument? Or is the existing patterns OK?
On 2017/07/13 06:35:44, jzw1 wrote: > On 2017/07/13 06:33:32, jzw1 wrote: > > mailto:jzw@chromium.org changed reviewers: > > + mailto:eugenebut@chromium.org, mailto:ichikawa@chromium.org, > mailto:michaeldo@chromium.org > > Hey guys, PTAL. I feel it's kind of weird having these as class methods and also > mentioning that they only apply for non-incognito settings. That's why I > originally put these methods on CWVWebViewConfiguration, but I also understand > that it's weird not to put these methods on CWVTranslationController. How would > you guys feel if I made it so these class methods takes a > CWVWebViewConfiguration as an argument? Or is the existing patterns OK? I believe we should provide a way to disable it in incognito mode too. If the user disable it in the settings, we should disable it for both the default mode and incognito mode. How about separating these two: - CWVTranslationController, a wrapper of TranslateClient. CWVWebView has a CWVTranslationController. - CWVTranslationConfiguration, a wrapper of TranslatePrefs. CWVWebViewConfiguration has a CWVTranslationConfiguration. And move translateEnabled to CWVTranslationConfiguration?
https://codereview.chromium.org/2983453002/diff/1/ios/web_view/public/cwv_tra... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2983453002/diff/1/ios/web_view/public/cwv_tra... ios/web_view/public/cwv_translation_controller.h:60: + (BOOL)isTranslateEnabled; Optional: Maybe it can be just "isEnabled" because it's obvious that it's for translate from the class it belongs to. Also, how about defining a @property for this?
On 2017/07/13 07:26:33, Hiroshi Ichikawa wrote:
> On 2017/07/13 06:35:44, jzw1 wrote:
> > On 2017/07/13 06:33:32, jzw1 wrote:
> > > mailto:jzw@chromium.org changed reviewers:
> > > + mailto:eugenebut@chromium.org, mailto:ichikawa@chromium.org,
> > mailto:michaeldo@chromium.org
> >
> > Hey guys, PTAL. I feel it's kind of weird having these as class methods and
> also
> > mentioning that they only apply for non-incognito settings. That's why I
> > originally put these methods on CWVWebViewConfiguration, but I also
understand
> > that it's weird not to put these methods on CWVTranslationController. How
> would
> > you guys feel if I made it so these class methods takes a
> > CWVWebViewConfiguration as an argument? Or is the existing patterns OK?
>
> I believe we should provide a way to disable it in incognito mode too. If the
> user disable it in the settings, we should disable it for both the default
mode
> and incognito mode.
>
> How about separating these two:
> - CWVTranslationController, a wrapper of TranslateClient. CWVWebView has a
> CWVTranslationController.
> - CWVTranslationConfiguration, a wrapper of TranslatePrefs.
> CWVWebViewConfiguration has a CWVTranslationConfiguration.
>
> And move translateEnabled to CWVTranslationConfiguration?
Thanks Hiroshi for the suggestion, I also think this is the best approach and
|resetTranslationPolicies| should move also to the new object. Just to clarify I
think the two methods introduced here and |resetTranslationPolicies| should move
to a new object CWVTranslationConfiguration.
The client can access the translation configuration through the
CWVWebViewConfiguration with something like
webViewConfiguration.translateConfiguration and that method can just be a getter
which returns the proper object initialized with |self| which is the
CWVWebViewConfiguration:
// inside CWVWebViewConfiguration
- (CWVTranslationConfiguration)translateConfiguration {
return [[CWVTranslationConfiguration alloc]
initWithWebViewConfiguration:self];
}
On 2017/07/13 15:01:43, michaeldo wrote:
> On 2017/07/13 07:26:33, Hiroshi Ichikawa wrote:
> > On 2017/07/13 06:35:44, jzw1 wrote:
> > > On 2017/07/13 06:33:32, jzw1 wrote:
> > > > mailto:jzw@chromium.org changed reviewers:
> > > > + mailto:eugenebut@chromium.org, mailto:ichikawa@chromium.org,
> > > mailto:michaeldo@chromium.org
> > >
> > > Hey guys, PTAL. I feel it's kind of weird having these as class methods
and
> > also
> > > mentioning that they only apply for non-incognito settings. That's why I
> > > originally put these methods on CWVWebViewConfiguration, but I also
> understand
> > > that it's weird not to put these methods on CWVTranslationController. How
> > would
> > > you guys feel if I made it so these class methods takes a
> > > CWVWebViewConfiguration as an argument? Or is the existing patterns OK?
> >
> > I believe we should provide a way to disable it in incognito mode too. If
the
> > user disable it in the settings, we should disable it for both the default
> mode
> > and incognito mode.
> >
> > How about separating these two:
> > - CWVTranslationController, a wrapper of TranslateClient. CWVWebView has a
> > CWVTranslationController.
> > - CWVTranslationConfiguration, a wrapper of TranslatePrefs.
> > CWVWebViewConfiguration has a CWVTranslationConfiguration.
> >
> > And move translateEnabled to CWVTranslationConfiguration?
>
> Thanks Hiroshi for the suggestion, I also think this is the best approach and
> |resetTranslationPolicies| should move also to the new object. Just to clarify
I
> think the two methods introduced here and |resetTranslationPolicies| should
move
> to a new object CWVTranslationConfiguration.
>
> The client can access the translation configuration through the
> CWVWebViewConfiguration with something like
> webViewConfiguration.translateConfiguration and that method can just be a
getter
> which returns the proper object initialized with |self| which is the
> CWVWebViewConfiguration:
>
> // inside CWVWebViewConfiguration
> - (CWVTranslationConfiguration)translateConfiguration {
> return [[CWVTranslationConfiguration alloc]
> initWithWebViewConfiguration:self];
> }
Great. I'll do that.
https://codereview.chromium.org/2983453002/diff/1/ios/web_view/public/cwv_tra... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2983453002/diff/1/ios/web_view/public/cwv_tra... ios/web_view/public/cwv_translation_controller.h:59: // Enables or disables the translate feature. Only applies to non-incognito. Why does it work only for non-incognito? If I disable translation in app prefs, as a user I would not want to see it in Incognito. https://codereview.chromium.org/2983453002/diff/1/ios/web_view/public/cwv_tra... ios/web_view/public/cwv_translation_controller.h:60: + (BOOL)isTranslateEnabled; On 2017/07/13 07:28:59, Hiroshi Ichikawa wrote: > Optional: Maybe it can be just "isEnabled" because it's obvious that it's for > translate from the class it belongs to. Also, how about defining a @property for > this? Or isTranslationEnabled/setTranslationEnabled: to be consistent with resetTranslationPolicies?
https://codereview.chromium.org/2983453002/diff/1/ios/web_view/public/cwv_tra... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2983453002/diff/1/ios/web_view/public/cwv_tra... ios/web_view/public/cwv_translation_controller.h:59: // Enables or disables the translate feature. Only applies to non-incognito. On 2017/07/13 17:52:58, Eugene But wrote: > Why does it work only for non-incognito? If I disable translation in app prefs, > as a user I would not want to see it in Incognito. Refactored so that the user can configure for both incognito and non-incognito. This isn't perfect because incognito settings are only stored temporary in memory and do not inherit any settings from non-incognito like Chrome does. If we wanted to go that route, we would need to refactor our browser states some more... https://codereview.chromium.org/2983453002/diff/1/ios/web_view/public/cwv_tra... ios/web_view/public/cwv_translation_controller.h:60: + (BOOL)isTranslateEnabled; On 2017/07/13 17:52:58, Eugene But wrote: > On 2017/07/13 07:28:59, Hiroshi Ichikawa wrote: > > Optional: Maybe it can be just "isEnabled" because it's obvious that it's for > > translate from the class it belongs to. Also, how about defining a @property > for > > this? > Or isTranslationEnabled/setTranslationEnabled: to be consistent with > resetTranslationPolicies? I've renamed these in the new class CWVTranslationConfiguration.
I like the general direction. Have a few comments. https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_configuration.mm (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_configuration.mm:43: prefs::kAcceptLanguages, nullptr); nit: Do you want to document nullptr with comments or using a local variable? https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_configuration_internal.h (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_configuration_internal.h:12: // Internal methods to hide C++ details. nit: Do we need this comment? Technically category does not exist to hide C++ details, but to hide private initializer. https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_configuration_internal.h:15: // Takes a PrefService object used to modify settings. Please document that callers are responsible for keeping |prefService| alive until CWVTranslationConfiguration is alive. https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_web_main_delegate.mm (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_web_main_delegate.mm:13: // Objective-C dummy object used to locate the containing NSBundle. This should probably be a part of a different CL https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_configuration.h (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_configuration.h:17: @property(nonatomic, assign) BOOL enabled; @property(nonatomic, assign, getter=isEnabled) BOOL enabled; https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_configuration.h:19: // Disable default initializer. Do we need this comment? https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view_configuration.h:30: CWVTranslationConfiguration* translationConfiguration; In Objective-C, configuration objects are used to configure other objects (e.g. initialize them). So I don't think we should use configuration name here. How about CWVTranslationPreferences or just CWVPreferences (similar to WKPreferences)?
https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_web_main_delegate.mm (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_web_main_delegate.mm:13: // Objective-C dummy object used to locate the containing NSBundle. On 2017/07/14 00:22:43, Eugene But wrote: > This should probably be a part of a different CL +1 https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view_configuration.h:30: CWVTranslationConfiguration* translationConfiguration; On 2017/07/14 00:22:43, Eugene But wrote: > In Objective-C, configuration objects are used to configure other objects (e.g. > initialize them). So I don't think we should use configuration name here. How > about CWVTranslationPreferences or just CWVPreferences (similar to > WKPreferences)? I would slightly prefer CWVTranslationPreferences because I prefer to isolate translation specific features to its own classes. But either is fine for me.
On 2017/07/14 01:18:39, Hiroshi Ichikawa wrote: > https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/w... > File ios/web_view/internal/web_view_web_main_delegate.mm (right): > > https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/w... > ios/web_view/internal/web_view_web_main_delegate.mm:13: // Objective-C dummy > object used to locate the containing NSBundle. > On 2017/07/14 00:22:43, Eugene But wrote: > > This should probably be a part of a different CL > > +1 > > https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... > File ios/web_view/public/cwv_web_view_configuration.h (right): > > https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... > ios/web_view/public/cwv_web_view_configuration.h:30: > CWVTranslationConfiguration* translationConfiguration; > On 2017/07/14 00:22:43, Eugene But wrote: > > In Objective-C, configuration objects are used to configure other objects > (e.g. > > initialize them). So I don't think we should use configuration name here. How > > about CWVTranslationPreferences or just CWVPreferences (similar to > > WKPreferences)? > > I would slightly prefer CWVTranslationPreferences because I prefer to isolate > translation specific features to its own classes. But either is fine for me. Thanks for all the suggestions guys, I'm going to create CWVPreferences class that wraps PrefService, that way we can configure anything we'd like in the future too.
https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_configuration.mm (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_configuration.mm:43: prefs::kAcceptLanguages, nullptr); On 2017/07/14 00:22:42, Eugene But wrote: > nit: Do you want to document nullptr with comments or using a local variable? I think it's OK to omit. The initializer in translate_prefs.h says: // |preferred_languages_pref| is only used on Chrome OS, other platforms must // pass NULL. https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_configuration_internal.h (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_configuration_internal.h:12: // Internal methods to hide C++ details. On 2017/07/14 00:22:43, Eugene But wrote: > nit: Do we need this comment? Technically category does not exist to hide C++ > details, but to hide private initializer. Done. https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_configuration_internal.h:15: // Takes a PrefService object used to modify settings. On 2017/07/14 00:22:43, Eugene But wrote: > Please document that callers are responsible for keeping |prefService| alive > until CWVTranslationConfiguration is alive. Done. https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_web_main_delegate.mm (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_web_main_delegate.mm:13: // Objective-C dummy object used to locate the containing NSBundle. On 2017/07/14 01:18:39, Hiroshi Ichikawa wrote: > On 2017/07/14 00:22:43, Eugene But wrote: > > This should probably be a part of a different CL > > +1 Done. https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_configuration.h (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_configuration.h:17: @property(nonatomic, assign) BOOL enabled; On 2017/07/14 00:22:43, Eugene But wrote: > @property(nonatomic, assign, getter=isEnabled) BOOL enabled; Done. https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_configuration.h:19: // Disable default initializer. On 2017/07/14 00:22:43, Eugene But wrote: > Do we need this comment? Done. https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view_configuration.h:30: CWVTranslationConfiguration* translationConfiguration; On 2017/07/14 01:18:39, Hiroshi Ichikawa wrote: > On 2017/07/14 00:22:43, Eugene But wrote: > > In Objective-C, configuration objects are used to configure other objects > (e.g. > > initialize them). So I don't think we should use configuration name here. How > > about CWVTranslationPreferences or just CWVPreferences (similar to > > WKPreferences)? > > I would slightly prefer CWVTranslationPreferences because I prefer to isolate > translation specific features to its own classes. But either is fine for me. Going to call this new thing CWVPreferences.
lgtm https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_configuration.mm (right): https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_configuration.mm:43: prefs::kAcceptLanguages, nullptr); On 2017/07/14 02:09:49, jzw1 wrote: > On 2017/07/14 00:22:42, Eugene But wrote: > > nit: Do you want to document nullptr with comments or using a local variable? > > I think it's OK to omit. The initializer in translate_prefs.h says: > // |preferred_languages_pref| is only used on Chrome OS, other platforms must > // pass NULL. No, it should be still commented. It can be like this: translate::TranslatePrefs translatePrefs(_prefService, prefs::kAcceptLanguages, /*preferred_languages_pref=*/nullptr); Otherwise people would wonder what this nullptr is. See "Function Argument Comments" in: https://google.github.io/styleguide/cppguide.html#Implementation_Comments As a general rule, ideally, people should be able to understand the code without referring to the definition of the classes/functions called from there.
On 2017/07/14 02:55:49, Hiroshi Ichikawa wrote: > lgtm > > https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... > File ios/web_view/internal/translate/cwv_translation_configuration.mm (right): > > https://codereview.chromium.org/2983453002/diff/20001/ios/web_view/internal/t... > ios/web_view/internal/translate/cwv_translation_configuration.mm:43: > prefs::kAcceptLanguages, nullptr); > On 2017/07/14 02:09:49, jzw1 wrote: > > On 2017/07/14 00:22:42, Eugene But wrote: > > > nit: Do you want to document nullptr with comments or using a local > variable? > > > > I think it's OK to omit. The initializer in translate_prefs.h says: > > // |preferred_languages_pref| is only used on Chrome OS, other platforms > must > > // pass NULL. > > No, it should be still commented. It can be like this: > > translate::TranslatePrefs translatePrefs(_prefService, > prefs::kAcceptLanguages, > /*preferred_languages_pref=*/nullptr); > > Otherwise people would wonder what this nullptr is. See "Function Argument > Comments" in: > https://google.github.io/styleguide/cppguide.html#Implementation_Comments > > As a general rule, ideally, people should be able to understand the code without > referring to the definition of the classes/functions called from there. Ah I see. it makes sense I never had to do that in ObjC since we had named params. Thanks for the link!
lgtm!
lgtm https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... File ios/web_view/internal/cwv_preferences.mm (right): https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... ios/web_view/internal/cwv_preferences.mm:5: #import "ios/web_view/public/cwv_preferences.h" No need for cwv_preferences import. Other objects like this in web_view/ only include the _internal.h header. Ref: https://cs.chromium.org/chromium/src/ios/web_view/internal/cwv_html_element.m... https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... ios/web_view/internal/cwv_preferences.mm:21: @dynamic translationEnabled; I don't think @dynamic is needed here because both the setter and getters are defined in this method, is it? Ref: http://www.ios-blog.co.uk/tutorials/objective-c/synthesize-vs-dynamic/
https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... File ios/web_view/internal/cwv_preferences.mm (right): https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... ios/web_view/internal/cwv_preferences.mm:5: #import "ios/web_view/public/cwv_preferences.h" On 2017/07/14 16:11:08, michaeldo wrote: > No need for cwv_preferences import. Other objects like this in web_view/ only > include the _internal.h header. > > Ref: > https://cs.chromium.org/chromium/src/ios/web_view/internal/cwv_html_element.m... Done. https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... ios/web_view/internal/cwv_preferences.mm:21: @dynamic translationEnabled; On 2017/07/14 16:11:08, michaeldo wrote: > I don't think @dynamic is needed here because both the setter and getters are > defined in this method, is it? > > Ref: http://www.ios-blog.co.uk/tutorials/objective-c/synthesize-vs-dynamic/ Oh yeah, I guess I've been doing it wrong for a long time :P
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, michaeldo@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2983453002/#ps100001 (title: "addressed michaeldo's comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... File ios/web_view/internal/cwv_preferences.mm (right): https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... ios/web_view/internal/cwv_preferences.mm:21: @dynamic translationEnabled; On 2017/07/14 17:37:19, jzw1 wrote: > On 2017/07/14 16:11:08, michaeldo wrote: > > I don't think @dynamic is needed here because both the setter and getters are > > defined in this method, is it? > > > > Ref: http://www.ios-blog.co.uk/tutorials/objective-c/synthesize-vs-dynamic/ > > Oh yeah, I guess I've been doing it wrong for a long time :P Thanks. No worries, I was only trying to learn why it was used here myself since I've only used it with CoreData. :)
On 2017/07/14 17:39:33, michaeldo wrote: > https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... > File ios/web_view/internal/cwv_preferences.mm (right): > > https://codereview.chromium.org/2983453002/diff/80001/ios/web_view/internal/c... > ios/web_view/internal/cwv_preferences.mm:21: @dynamic translationEnabled; > On 2017/07/14 17:37:19, jzw1 wrote: > > On 2017/07/14 16:11:08, michaeldo wrote: > > > I don't think @dynamic is needed here because both the setter and getters > are > > > defined in this method, is it? > > > > > > Ref: http://www.ios-blog.co.uk/tutorials/objective-c/synthesize-vs-dynamic/ > > > > Oh yeah, I guess I've been doing it wrong for a long time :P > > Thanks. No worries, I was only trying to learn why it was used here myself since > I've only used it with CoreData. :) I think I'm used to doing it because the projects I've worked on usually have auto synthesize, so if override and don't use the ivar, it causes a compile error?
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500053882943710,
"parent_rev": "1b25fddc97fd2840cbe0d248fc0b05eda3ba0594", "commit_rev":
"9398662526db21a8c155d980174cf40c670859af"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500053882943710,
"parent_rev": "a8b25aabb32e60a0ef8b7613fddcd3c4bc4f9874", "commit_rev":
"03b800c37e9643a72abb82fb9dc1050648ce3aa1"}
Message was sent while issue was closed.
Description was changed from ========== Expose way to enable/disable translate. ========== to ========== Expose way to enable/disable translate. Review-Url: https://codereview.chromium.org/2983453002 Cr-Commit-Position: refs/heads/master@{#486833} Committed: https://chromium.googlesource.com/chromium/src/+/03b800c37e9643a72abb82fb9dc1... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/03b800c37e9643a72abb82fb9dc1... |
