Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888313003:
Update strings for history description in CBD (Closed)
DescriptionUpdate strings for history description in CBD
Change history strings for the new Clear Browsing Data dialog
according to discussions with legal and UX.
Add a string with myactivity.google.com for history dialog and use
it depending on TABS_IN_CBD flag.
Use ActiveDataTypes instead of PreferredDataTypes with
HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS to determine
history sync status.
BUG=681523
Review-Url: https://codereview.chromium.org/2888313003
Cr-Commit-Position: refs/heads/master@{#474600}
Committed: https://chromium.googlesource.com/chromium/src/+/b8ccee5d81a839c22d62e4ea7aeec251f0772684
Patch Set 1 #Patch Set 2 : cbd history string #
Total comments: 5
Patch Set 3 : try fix test #Patch Set 4 : add active datatype #
Messages
Total messages: 33 (26 generated)
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussion with UX. BUG=681523 ========== to ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussion with UX. Also change history.google.com to myactivity.google.com. Use ActiveDataTypes instead of PreferredDataTypes. BUG=681523 ==========
Description was changed from ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussion with UX. Also change history.google.com to myactivity.google.com. Use ActiveDataTypes instead of PreferredDataTypes. BUG=681523 ========== to ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussion with UX. Also change history.google.com to myactivity.google.com. Use ActiveDataTypes instead of PreferredDataTypes with HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS. BUG=681523 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussion with UX. Also change history.google.com to myactivity.google.com. Use ActiveDataTypes instead of PreferredDataTypes with HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS. BUG=681523 ========== to ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussion with UX. Also change history.google.com to myactivity.google.com in history dialog. Use ActiveDataTypes instead of PreferredDataTypes with HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS. BUG=681523 ==========
Description was changed from ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussion with UX. Also change history.google.com to myactivity.google.com in history dialog. Use ActiveDataTypes instead of PreferredDataTypes with HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS. BUG=681523 ========== to ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussions legal and UX. Add a string with myactivity.google.com for history dialog and use it depending on TABS_IN_CBD flag. Use ActiveDataTypes instead of PreferredDataTypes with HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS to determine history sync status. BUG=681523 ==========
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussions legal and UX. Add a string with myactivity.google.com for history dialog and use it depending on TABS_IN_CBD flag. Use ActiveDataTypes instead of PreferredDataTypes with HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS to determine history sync status. BUG=681523 ========== to ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussions with legal and UX. Add a string with myactivity.google.com for history dialog and use it depending on TABS_IN_CBD flag. Use ActiveDataTypes instead of PreferredDataTypes with HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS to determine history sync status. BUG=681523 ==========
dullweber@chromium.org changed reviewers: + yfriedman@chromium.org
yfriedman@: Please review this small change. Thanks
https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java (right): https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:339: boolean flagEnabled = ChromeFeatureList.isEnabled(ChromeFeatureList.TABS_IN_CBD); is native guaranteed to be loaded at this point? I think probably yes but want to make sure you've considered it https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:678: Your Google Account may have other forms of browsing history at <ph name="BEGIN_LINK"><link></ph>myactivity.google.com<ph name="END_LINK"></link></ph>. this only differs in the placeholder value, right? If so, you shouldn't need a new string since this is only visible at translation time and not to users
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java (right): https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:339: boolean flagEnabled = ChromeFeatureList.isEnabled(ChromeFeatureList.TABS_IN_CBD); On 2017/05/24 14:18:43, Yaron wrote: > is native guaranteed to be loaded at this point? I think probably yes but want > to make sure you've considered it Thanks for pointing at this, I don't know how the initialization of the native library exactly works but I think it should be initialized: HistoryAdapter is created in the constructor of HistoryManager. The HistoryManager constructor calls historyAdapter.initialize(), which makes a call to BrowsingHistoryBridge::queryHistory, which calls nativeQueryHistory(). That should mean that native calls are possible when HistoryAdapter is created, right? Other than that, the HistoryActivity, which creates the HistoryManager should only be created when a user selects history from the menu, so that is definitely late enough? https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:678: Your Google Account may have other forms of browsing history at <ph name="BEGIN_LINK"><link></ph>myactivity.google.com<ph name="END_LINK"></link></ph>. On 2017/05/24 14:18:43, Yaron wrote: > this only differs in the placeholder value, right? If so, you shouldn't need a > new string since this is only visible at translation time and not to users In this case the url is not a placeholder. MY_ACTIVITY_LINK/GOOGLE_HISTORY_LINK are only used for the onClick handler in HistoryAdapter::setPrivacyDisclaimerText. I could turn the url into a placeholder to simplify translation in the future but I don't know if that is typically done this way for static strings?
lgtm https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java (right): https://codereview.chromium.org/2888313003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:339: boolean flagEnabled = ChromeFeatureList.isEnabled(ChromeFeatureList.TABS_IN_CBD); On 2017/05/24 14:54:52, dullweber wrote: > On 2017/05/24 14:18:43, Yaron wrote: > > is native guaranteed to be loaded at this point? I think probably yes but want > > to make sure you've considered it > > Thanks for pointing at this, I don't know how the initialization of the native > library exactly works but I think it should be initialized: > > HistoryAdapter is created in the constructor of HistoryManager. The > HistoryManager constructor calls historyAdapter.initialize(), which makes a call > to BrowsingHistoryBridge::queryHistory, which calls nativeQueryHistory(). > That should mean that native calls are possible when HistoryAdapter is created, > right? > yep. > Other than that, the HistoryActivity, which creates the HistoryManager should > only be created when a user selects history from the menu, so that is definitely > late enough? not always true. if the activity is in recents and chrome gets killed while in the background this could be an issue when user comes back to chrome. But looking at HistortyActivity, if you follow the hierarchy, it looks to do synchronous init in onCreate so this is ok.
The CQ bit was unchecked by dullweber@chromium.org
The CQ bit was checked by dullweber@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dullweber@chromium.org
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
The CQ bit was checked by dullweber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/2888313003/#ps200001 (title: "add active datatype")
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": 200001, "attempt_start_ts": 1495695015258830,
"parent_rev": "746e2408f9f35f9acc8d9ecb991e1de9be729db8", "commit_rev":
"b8ccee5d81a839c22d62e4ea7aeec251f0772684"}
Message was sent while issue was closed.
Description was changed from ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussions with legal and UX. Add a string with myactivity.google.com for history dialog and use it depending on TABS_IN_CBD flag. Use ActiveDataTypes instead of PreferredDataTypes with HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS to determine history sync status. BUG=681523 ========== to ========== Update strings for history description in CBD Change history strings for the new Clear Browsing Data dialog according to discussions with legal and UX. Add a string with myactivity.google.com for history dialog and use it depending on TABS_IN_CBD flag. Use ActiveDataTypes instead of PreferredDataTypes with HISTORY_DELETE_DIRECTIVES instead of TYPED_URLS to determine history sync status. BUG=681523 Review-Url: https://codereview.chromium.org/2888313003 Cr-Commit-Position: refs/heads/master@{#474600} Committed: https://chromium.googlesource.com/chromium/src/+/b8ccee5d81a839c22d62e4ea7aee... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:200001) as https://chromium.googlesource.com/chromium/src/+/b8ccee5d81a839c22d62e4ea7aee... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
