Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2965303002:
Expose method to reset translate settings. (Closed)
DescriptionExpose method to reset translate settings.
Had to refactor browser states to be global instances and force off the record
browser state to use an in-memory store instead of a json file persisted on disk.
BUG=739987
Review-Url: https://codereview.chromium.org/2965303002
Cr-Commit-Position: refs/heads/master@{#485392}
Committed: https://chromium.googlesource.com/chromium/src/+/9aa5149dcd24c4c14fc6fbd0b7fda7cb200bf07d
Patch Set 1 #
Total comments: 4
Patch Set 2 : move to class method #
Total comments: 5
Patch Set 3 : make webstates global and use in-memory store for off the record #
Total comments: 2
Patch Set 4 : no policies for incognito #
Total comments: 4
Patch Set 5 : change early returns to asserts #
Total comments: 8
Patch Set 6 : change assert comment #Patch Set 7 : oops #
Total comments: 2
Patch Set 8 : update comment #
Messages
Total messages: 30 (7 generated)
Description was changed from ========== expose method to reset translate settings BUG=739987 ========== to ========== expose method to reset translate settings BUG=739987 ==========
jzw@chromium.org changed reviewers: + eugenebut@chromium.org, ichikawa@chromium.org, michaeldo@chromium.org
On 2017/07/07 06:15:28, jzw1 wrote: > mailto:jzw@chromium.org changed reviewers: > + mailto:eugenebut@chromium.org, mailto:ichikawa@chromium.org, mailto:michaeldo@chromium.org Hey guys PTAL.
https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... ios/web_view/public/cwv_web_view_configuration.h:28: - (void)resetTranslationPreferences; Shouldn't this be a method of CWVTranslationController instead, at the same place as other translation policy methods?
Problem is you can't get to a cwvtranslationcontroller without having a cwvwebview, which we won't have in settings. What do you think about a class method though? On Fri, Jul 7, 2017 at 00:53 <ichikawa@chromium.org> wrote: > > > https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... > File ios/web_view/public/cwv_web_view_configuration.h (right): > > > https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... > ios/web_view/public/cwv_web_view_configuration.h:28: - > (void)resetTranslationPreferences; > Shouldn't this be a method of CWVTranslationController instead, at the > same place as other translation policy methods? > > https://codereview.chromium.org/2965303002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... ios/web_view/public/cwv_web_view_configuration.h:28: - (void)resetTranslationPreferences; On 2017/07/07 07:53:43, Hiroshi Ichikawa wrote: > Shouldn't this be a method of CWVTranslationController instead, at the same > place as other translation policy methods? I agree, we should move it, it is ok if it's a class method there. Can we also expand on the comment here to specify what is cleared in relation to the other API. For example, does this clear translation_policies set in CWVTranslationController? It isn't currently clear.
https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... ios/web_view/public/cwv_web_view_configuration.h:28: - (void)resetTranslationPreferences; On 2017/07/07 15:52:20, michaeldo wrote: > On 2017/07/07 07:53:43, Hiroshi Ichikawa wrote: > > Shouldn't this be a method of CWVTranslationController instead, at the same > > place as other translation policy methods? > > I agree, we should move it, it is ok if it's a class method there. > > Can we also expand on the comment here to specify what is cleared in relation to > the other API. For example, does this clear translation_policies set in > CWVTranslationController? It isn't currently clear. +1
https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2965303002/diff/1/ios/web_view/public/cwv_web... ios/web_view/public/cwv_web_view_configuration.h:28: - (void)resetTranslationPreferences; On 2017/07/07 16:40:46, Eugene But wrote: > On 2017/07/07 15:52:20, michaeldo wrote: > > On 2017/07/07 07:53:43, Hiroshi Ichikawa wrote: > > > Shouldn't this be a method of CWVTranslationController instead, at the same > > > place as other translation policy methods? > > > > I agree, we should move it, it is ok if it's a class method there. > > > > Can we also expand on the comment here to specify what is cleared in relation > to > > the other API. For example, does this clear translation_policies set in > > CWVTranslationController? It isn't currently clear. > +1 Done.
https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:75: [CWVWebViewConfiguration defaultConfiguration].browserState; Sorry I didn't think of this before, but will this apply to other already created and in use configurations? If not, we should explain in the method comment that is will only apply to configurations created after it is called.
https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:75: [CWVWebViewConfiguration defaultConfiguration].browserState; What if clients want to reset policies for incognito configuration? Would it be better to make |resetTranslationPolicies| instance method and pass prefs object to TranslationController init?
On 2017/07/07 20:10:28, Eugene But wrote: > https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... > File ios/web_view/internal/translate/cwv_translation_controller.mm (right): > > https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... > ios/web_view/internal/translate/cwv_translation_controller.mm:75: > [CWVWebViewConfiguration defaultConfiguration].browserState; > What if clients want to reset policies for incognito configuration? Would it be > better to make |resetTranslationPolicies| instance method and pass prefs object > to TranslationController init? Hey guys I sent an email thread about this change. Let's discuss there.
https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:75: [CWVWebViewConfiguration defaultConfiguration].browserState; On 2017/07/07 20:10:28, Eugene But wrote: > What if clients want to reset policies for incognito configuration? Would it be > better to make |resetTranslationPolicies| instance method and pass prefs object > to TranslationController init? OK guys I've changed it so: 1. global instance for incognito and non-incognito browse states (2 total). 2. incognito browse state uses in memory store. this means translate preferences are not shared between incognito and non-incognito.
Description was changed from ========== expose method to reset translate settings BUG=739987 ========== to ========== Expose method to reset translate settings. Had to refactor browser states to be global instances and force off the record browser state to use an in-memory store instead of a json file persisted on disk. BUG=739987 ==========
https://codereview.chromium.org/2965303002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2965303002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:55: // Resets all translation policies to CWVTranslationPolicyAsk. Can you document that this method only clears the policies for [CWVWebViewConfiguration defaultConfiguration], but not for [CWVWebViewConfiguration incognitoConfiguration]? Maybe we should also provide a way to clear the policies for incognito windows? They are cleared when the app is terminated, but maybe we also want to clear the incognito policies when the user explicitly asks to clear the policies?
https://codereview.chromium.org/2965303002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2965303002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:55: // Resets all translation policies to CWVTranslationPolicyAsk. On 2017/07/10 01:35:00, Hiroshi Ichikawa wrote: > Can you document that this method only clears the policies for > [CWVWebViewConfiguration defaultConfiguration], but not for > [CWVWebViewConfiguration incognitoConfiguration]? > > Maybe we should also provide a way to clear the policies for incognito windows? > They are cleared when the app is terminated, but maybe we also want to clear the > incognito policies when the user explicitly asks to clear the policies? What if we clear both in this method? On second thought, maybe incognito users shouldn't be able to configure any policies.
On 2017/07/10 03:21:31, jzw1 wrote: > https://codereview.chromium.org/2965303002/diff/40001/ios/web_view/public/cwv... > File ios/web_view/public/cwv_translation_controller.h (right): > > https://codereview.chromium.org/2965303002/diff/40001/ios/web_view/public/cwv... > ios/web_view/public/cwv_translation_controller.h:55: // Resets all translation > policies to CWVTranslationPolicyAsk. > On 2017/07/10 01:35:00, Hiroshi Ichikawa wrote: > > Can you document that this method only clears the policies for > > [CWVWebViewConfiguration defaultConfiguration], but not for > > [CWVWebViewConfiguration incognitoConfiguration]? > > > > Maybe we should also provide a way to clear the policies for incognito > windows? > > They are cleared when the app is terminated, but maybe we also want to clear > the > > incognito policies when the user explicitly asks to clear the policies? > > What if we clear both in this method? On second thought, maybe incognito users > shouldn't be able to configure any policies. Confirmed with Alan that incognito users shouldn't be able to configure the policies i.e., we shouldn't provide an option to "Always translate" / "Never translate". But I guess it's tricky to enforce it in API level? So you can keep this method to only clear the policies in defaultConfiguration, but you should still document that it doesn't affect incognitoConfiguration, as a message to a potential client of this API who happens to update policies in incognitoConfiguration.
On 2017/07/10 03:49:40, Hiroshi Ichikawa wrote: > On 2017/07/10 03:21:31, jzw1 wrote: > > > https://codereview.chromium.org/2965303002/diff/40001/ios/web_view/public/cwv... > > File ios/web_view/public/cwv_translation_controller.h (right): > > > > > https://codereview.chromium.org/2965303002/diff/40001/ios/web_view/public/cwv... > > ios/web_view/public/cwv_translation_controller.h:55: // Resets all translation > > policies to CWVTranslationPolicyAsk. > > On 2017/07/10 01:35:00, Hiroshi Ichikawa wrote: > > > Can you document that this method only clears the policies for > > > [CWVWebViewConfiguration defaultConfiguration], but not for > > > [CWVWebViewConfiguration incognitoConfiguration]? > > > > > > Maybe we should also provide a way to clear the policies for incognito > > windows? > > > They are cleared when the app is terminated, but maybe we also want to clear > > the > > > incognito policies when the user explicitly asks to clear the policies? > > > > What if we clear both in this method? On second thought, maybe incognito users > > shouldn't be able to configure any policies. > > Confirmed with Alan that incognito users shouldn't be able to configure the > policies i.e., we shouldn't provide an option to "Always translate" / "Never > translate". > > But I guess it's tricky to enforce it in API level? So you can keep this method > to only clear the policies in defaultConfiguration, but you should still > document that it doesn't affect incognitoConfiguration, as a message to a > potential client of this API who happens to update policies in > incognitoConfiguration. I updated the comment but also enforced it on the API level, wdyt?
lgtm https://codereview.chromium.org/2965303002/diff/60001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2965303002/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Early returns in incognito mode. Optional: Maybe better to make it an assertion failure instead of silently ignoring the call? The client shouldn't call this method on incognito mode, right? https://codereview.chromium.org/2965303002/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:85: // Early returns in incognito mode. Ditto.
https://codereview.chromium.org/2965303002/diff/60001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2965303002/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Early returns in incognito mode. On 2017/07/10 05:58:39, Hiroshi Ichikawa wrote: > Optional: Maybe better to make it an assertion failure instead of silently > ignoring the call? The client shouldn't call this method on incognito mode, > right? Done. https://codereview.chromium.org/2965303002/diff/60001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:85: // Early returns in incognito mode. On 2017/07/10 05:58:39, Hiroshi Ichikawa wrote: > Ditto. Done.
https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Asserts in incognito mode. Should be "Causes assertion failure in incognito mode", or maybe you can just say "Not supported in incognito mode". The current sentence can be read as "Asserts that it is in incognito mode" which means opposite. https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:85: // Asserts in incognito mode. Ditto.
https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:75: [CWVWebViewConfiguration defaultConfiguration].browserState; On 2017/07/08 01:09:41, jzw1 wrote: > On 2017/07/07 20:10:28, Eugene But wrote: > > What if clients want to reset policies for incognito configuration? Would it > be > > better to make |resetTranslationPolicies| instance method and pass prefs > object > > to TranslationController init? > > OK guys I've changed it so: > 1. global instance for incognito and non-incognito browse states (2 total). > 2. incognito browse state uses in memory store. > > this means translate preferences are not shared between incognito and > non-incognito. All methods related to translate policies are instance methods. Is there a reason why only |reset| method is a class method? This looks inconsistent to me. https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/internal/w... File ios/web_view/internal/web_view_browser_state.mm (right): https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/internal/w... ios/web_view/internal/web_view_browser_state.mm:62: user_pref_store = new InMemoryPrefStore(); nit: s/new/base::MakeUnique https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Asserts in incognito mode. What is the value of asserting in Incognito mode? If apps want to be able to set policies in incognito mode we should allow that in API. I think it should be up to application developer to decide whether or not they want to allow translation policies for Incognito. WDYT?
https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... File ios/web_view/internal/translate/cwv_translation_controller.mm (right): https://codereview.chromium.org/2965303002/diff/20001/ios/web_view/internal/t... ios/web_view/internal/translate/cwv_translation_controller.mm:75: [CWVWebViewConfiguration defaultConfiguration].browserState; On 2017/07/10 16:17:43, Eugene But wrote: > On 2017/07/08 01:09:41, jzw1 wrote: > > On 2017/07/07 20:10:28, Eugene But wrote: > > > What if clients want to reset policies for incognito configuration? Would it > > be > > > better to make |resetTranslationPolicies| instance method and pass prefs > > object > > > to TranslationController init? > > > > OK guys I've changed it so: > > 1. global instance for incognito and non-incognito browse states (2 total). > > 2. incognito browse state uses in memory store. > > > > this means translate preferences are not shared between incognito and > > non-incognito. > All methods related to translate policies are instance methods. Is there a > reason why only |reset| method is a class method? This looks inconsistent to me. The reason is because you can't get to an instance of CWVTranslationController without having a CWVWebView, but you should still be able to clear translate preferences without any existing CWVWebView. https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/internal/w... File ios/web_view/internal/web_view_browser_state.mm (right): https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/internal/w... ios/web_view/internal/web_view_browser_state.mm:62: user_pref_store = new InMemoryPrefStore(); On 2017/07/10 16:17:43, Eugene But wrote: > nit: s/new/base::MakeUnique Does this mean you want to use std::unique_ptrs instead? https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/public/cwv... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:77: // Asserts in incognito mode. On 2017/07/10 16:17:43, Eugene But wrote: > What is the value of asserting in Incognito mode? If apps want to be able to set > policies in incognito mode we should allow that in API. I think it should be up > to application developer to decide whether or not they want to allow translation > policies for Incognito. WDYT? I think it makes sense to not share translate preferences between incognito and non-incognito, even though Chrome iOS shares them. If we want to go with Chrome iOS route, we'd have to refactor browser states to have a parent child relationship between non-incognito and incognito browser states. I'd want to discourage setting policies for incognito because they are only stored in-memory, and I don't really see value in letting users store temporary values. https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/public/cwv... ios/web_view/public/cwv_translation_controller.h:85: // Asserts in incognito mode. On 2017/07/10 06:22:26, Hiroshi Ichikawa wrote: > Ditto. Done.
Thanks for the explanation. lgtm https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/internal/w... File ios/web_view/internal/web_view_browser_state.mm (right): https://codereview.chromium.org/2965303002/diff/80001/ios/web_view/internal/w... ios/web_view/internal/web_view_browser_state.mm:62: user_pref_store = new InMemoryPrefStore(); On 2017/07/10 18:51:40, jzw1 wrote: > On 2017/07/10 16:17:43, Eugene But wrote: > > nit: s/new/base::MakeUnique > > Does this mean you want to use std::unique_ptrs instead? Sorry, did not notice that this is not std::unique_ptrs https://codereview.chromium.org/2965303002/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2965303002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:55: // Resets all translation policies to CWVTranslationPolicyAsk. s/to CWVTranslationPolicyAsk/to default (CWVTranslationPolicyAsk) ?
https://codereview.chromium.org/2965303002/diff/120001/ios/web_view/public/cw... File ios/web_view/public/cwv_translation_controller.h (right): https://codereview.chromium.org/2965303002/diff/120001/ios/web_view/public/cw... ios/web_view/public/cwv_translation_controller.h:55: // Resets all translation policies to CWVTranslationPolicyAsk. On 2017/07/10 20:45:49, Eugene But wrote: > s/to CWVTranslationPolicyAsk/to default (CWVTranslationPolicyAsk) ? 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 Link to the patchset: https://codereview.chromium.org/2965303002/#ps140001 (title: "update comment")
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": 140001, "attempt_start_ts": 1499720428537370,
"parent_rev": "019b6bf2e9c7c01c02057ace21e590806eb47342", "commit_rev":
"9aa5149dcd24c4c14fc6fbd0b7fda7cb200bf07d"}
Message was sent while issue was closed.
Description was changed from ========== Expose method to reset translate settings. Had to refactor browser states to be global instances and force off the record browser state to use an in-memory store instead of a json file persisted on disk. BUG=739987 ========== to ========== Expose method to reset translate settings. Had to refactor browser states to be global instances and force off the record browser state to use an in-memory store instead of a json file persisted on disk. BUG=739987 Review-Url: https://codereview.chromium.org/2965303002 Cr-Commit-Position: refs/heads/master@{#485392} Committed: https://chromium.googlesource.com/chromium/src/+/9aa5149dcd24c4c14fc6fbd0b7fd... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/9aa5149dcd24c4c14fc6fbd0b7fd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
