Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
974463002:
Respect prefs::kAllowDeletingBrowserHistory in the Clear History UI on Android. (Closed)
DescriptionRespect prefs::kAllowDeletingBrowserHistory in the Clear History UI on Android.
BUG=443095
Committed: https://crrev.com/935a20a1398e28ba5ec3298ae97c941810e618c0
Cr-Commit-Position: refs/heads/master@{#319618}
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : Addressed Comments #
Total comments: 3
Patch Set 4 : Nits #Patch Set 5 : s/selected/clicked + comment #
Total comments: 2
Patch Set 6 : #Patch Set 7 : Update toast message #
Total comments: 2
Messages
Total messages: 24 (7 generated)
knn@chromium.org changed reviewers: + bauerb@chromium.org
Please Have a Look. I am finally using the ListView as just a list and handling the CheckBox manually in the OnClick method.
https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:118: return EnumSet.of(DialogOption.CLEAR_CACHE, DialogOption.CLEAR_COOKIES_AND_SITE_DATA); Maybe start with the base EnumSet, then add CLEAR_HISTORY if supported? https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:153: ((CheckedTextView) mDialog.getListView().getChildAt(whichButton)).toggle(); Why do you need to do this manually? https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:154: if (mSelectedOptions.contains(selectedOption)) { I would be a bit more at ease if we would keep using |isChecked| here, or at least assert that they are always in sync. https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:213: mDialog.getListView().post(new Runnable() { Any particular reason you're doing this on the next turn of the event loop? If yes, you might want to add a comment to explain. https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/stri... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/stri... chrome/android/java/strings/android_chrome_strings.grd:360: <message name="IDS_CAN_NOT_CLEAR_BROWSING_HISTORY_TOAST" desc='Message on the toast explaining that the user cannot "clear browsing history".'> No quotes for "clear browsing history". We should probably also use double quotes for the description. (Up to you whether you want to change that for the messages above as well.)
PTAL https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:118: return EnumSet.of(DialogOption.CLEAR_CACHE, DialogOption.CLEAR_COOKIES_AND_SITE_DATA); On 2015/03/03 14:26:20, Bernhard Bauer wrote: > Maybe start with the base EnumSet, then add CLEAR_HISTORY if supported? Done. https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:153: ((CheckedTextView) mDialog.getListView().getChildAt(whichButton)).toggle(); On 2015/03/03 14:26:20, Bernhard Bauer wrote: > Why do you need to do this manually? Yes. The ListView is forbidden from managing the Checkbox as we want to handle Browsing History separately. https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:154: if (mSelectedOptions.contains(selectedOption)) { On 2015/03/03 14:26:21, Bernhard Bauer wrote: > I would be a bit more at ease if we would keep using |isChecked| here, or at > least assert that they are always in sync. isChecked is set by the ListView which does not always manage the Checkbox. https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:213: mDialog.getListView().post(new Runnable() { On 2015/03/03 14:26:20, Bernhard Bauer wrote: > Any particular reason you're doing this on the next turn of the event loop? If > yes, you might want to add a comment to explain. The children of the list view are not created until the first time it is drawn. Will do. https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/stri... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/stri... chrome/android/java/strings/android_chrome_strings.grd:360: <message name="IDS_CAN_NOT_CLEAR_BROWSING_HISTORY_TOAST" desc='Message on the toast explaining that the user cannot "clear browsing history".'> On 2015/03/03 14:26:21, Bernhard Bauer wrote: > No quotes for "clear browsing history". We should probably also use double > quotes for the description. (Up to you whether you want to change that for the > messages above as well.) You mean quotes around "clear browsing data" in the previous strings are also not required? I'll update them as well unless this is a grit style hint for translators.
https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:154: if (mSelectedOptions.contains(selectedOption)) { On 2015/03/03 15:03:43, knn wrote: > On 2015/03/03 14:26:21, Bernhard Bauer wrote: > > I would be a bit more at ease if we would keep using |isChecked| here, or at > > least assert that they are always in sync. > > isChecked is set by the ListView which does not always manage the Checkbox. OK... But presumably we could still get the state from the button in that case? I'm just worried about this getting out of sync somehow, and then we'll end up doing the exact opposite thing of what we're supposed to do. https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/stri... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/stri... chrome/android/java/strings/android_chrome_strings.grd:360: <message name="IDS_CAN_NOT_CLEAR_BROWSING_HISTORY_TOAST" desc='Message on the toast explaining that the user cannot "clear browsing history".'> On 2015/03/03 15:03:43, knn wrote: > On 2015/03/03 14:26:21, Bernhard Bauer wrote: > > No quotes for "clear browsing history". We should probably also use double > > quotes for the description. (Up to you whether you want to change that for the > > messages above as well.) > > You mean quotes around "clear browsing data" in the previous strings are also > not required? I'll update them as well unless this is a grit style hint for > translators. In the other strings, "clear browsing data" names the action that is currently happening. In this string, you are adding quotes around parts of the sentence (which makes it seem like you're quoting something, or browsing history isn't really cleared). As a test, in the descriptions above, you could replace the words in quotes with the name of something else (e.g. "[...] when waiting for the Olympic Games to complete"), and it would still make (at least grammatical) sense. You can't do that with this sentence. The thing I was refering to with the strings above is that they use single quotes for the overall description (presumably because double quotes are already used in the description). The right thing to do there IMO would be to use double quotes, and escape the quotes inside. But like I said, up to you whether you want to do that, or leave it as it is. https://codereview.chromium.org/974463002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:149: // Manually managing the Checkbox to handle disabled ones. Nit: "handle disabled options".
PTAL https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:154: if (mSelectedOptions.contains(selectedOption)) { On 2015/03/03 15:25:29, Bernhard Bauer wrote: > On 2015/03/03 15:03:43, knn wrote: > > On 2015/03/03 14:26:21, Bernhard Bauer wrote: > > > I would be a bit more at ease if we would keep using |isChecked| here, or at > > > least assert that they are always in sync. > > > > isChecked is set by the ListView which does not always manage the Checkbox. > > OK... But presumably we could still get the state from the button in that case? > I'm just worried about this getting out of sync somehow, and then we'll end up > doing the exact opposite thing of what we're supposed to do. The ListView gets the isChecked value from its internal state which we disabled when we manage the checkbox manually. So isChecked will always be false. We manage both the UI (checkbox toggle) and the internal state (mSelectedOptions) in a single method call so it should stay in sync. Updating Comments to make it clearer. https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/stri... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/stri... chrome/android/java/strings/android_chrome_strings.grd:360: <message name="IDS_CAN_NOT_CLEAR_BROWSING_HISTORY_TOAST" desc='Message on the toast explaining that the user cannot "clear browsing history".'> On 2015/03/03 15:25:29, Bernhard Bauer wrote: > On 2015/03/03 15:03:43, knn wrote: > > On 2015/03/03 14:26:21, Bernhard Bauer wrote: > > > No quotes for "clear browsing history". We should probably also use double > > > quotes for the description. (Up to you whether you want to change that for > the > > > messages above as well.) > > > > You mean quotes around "clear browsing data" in the previous strings are also > > not required? I'll update them as well unless this is a grit style hint for > > translators. > > In the other strings, "clear browsing data" names the action that is currently > happening. In this string, you are adding quotes around parts of the sentence > (which makes it seem like you're quoting something, or browsing history isn't > really cleared). > > As a test, in the descriptions above, you could replace the words in quotes with > the name of something else (e.g. "[...] when waiting for the Olympic Games to > complete"), and it would still make (at least grammatical) sense. You can't do > that with this sentence. > > The thing I was refering to with the strings above is that they use single > quotes for the overall description (presumably because double quotes are already > used in the description). The right thing to do there IMO would be to use double > quotes, and escape the quotes inside. But like I said, up to you whether you > want to do that, or leave it as it is. Thanks for explaining. I think this inconsistency is more readable so I'll leave it as it is. https://codereview.chromium.org/974463002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:149: // Manually managing the Checkbox to handle disabled ones. On 2015/03/03 15:25:29, Bernhard Bauer wrote: > Nit: "handle disabled options". Done. https://codereview.chromium.org/974463002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:149: // Manually managing the Checkbox to handle disabled ones. On 2015/03/03 15:25:29, Bernhard Bauer wrote: > Nit: "handle disabled options". Done.
https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:154: if (mSelectedOptions.contains(selectedOption)) { On 2015/03/03 15:55:56, knn wrote: > On 2015/03/03 15:25:29, Bernhard Bauer wrote: > > On 2015/03/03 15:03:43, knn wrote: > > > On 2015/03/03 14:26:21, Bernhard Bauer wrote: > > > > I would be a bit more at ease if we would keep using |isChecked| here, or > at > > > > least assert that they are always in sync. > > > > > > isChecked is set by the ListView which does not always manage the Checkbox. > > > > OK... But presumably we could still get the state from the button in that > case? > > I'm just worried about this getting out of sync somehow, and then we'll end up > > doing the exact opposite thing of what we're supposed to do. > > The ListView gets the isChecked value from its internal state which we disabled > when we manage the checkbox manually. So isChecked will always be false. > > We manage both the UI (checkbox toggle) and the internal state > (mSelectedOptions) in a single method call so it should stay in sync. > > Updating Comments to make it clearer. Right. What I meant was this: * If we are not managing the state ourselves, isChecked has a meaningful value, so we might as well use it. * If we are managing the state, we can get the correct value from the child of the list view, which we just toggled. If |isChecked| is false in that case, we could just overwrite its value.
PTAL. https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:154: if (mSelectedOptions.contains(selectedOption)) { On 2015/03/03 16:00:45, Bernhard Bauer wrote: > On 2015/03/03 15:55:56, knn wrote: > > On 2015/03/03 15:25:29, Bernhard Bauer wrote: > > > On 2015/03/03 15:03:43, knn wrote: > > > > On 2015/03/03 14:26:21, Bernhard Bauer wrote: > > > > > I would be a bit more at ease if we would keep using |isChecked| here, > or > > at > > > > > least assert that they are always in sync. > > > > > > > > isChecked is set by the ListView which does not always manage the > Checkbox. > > > > > > OK... But presumably we could still get the state from the button in that > > case? > > > I'm just worried about this getting out of sync somehow, and then we'll end > up > > > doing the exact opposite thing of what we're supposed to do. > > > > The ListView gets the isChecked value from its internal state which we > disabled > > when we manage the checkbox manually. So isChecked will always be false. > > > > We manage both the UI (checkbox toggle) and the internal state > > (mSelectedOptions) in a single method call so it should stay in sync. > > > > Updating Comments to make it clearer. > > Right. What I meant was this: > * If we are not managing the state ourselves, isChecked has a meaningful value, > so we might as well use it. > * If we are managing the state, we can get the correct value from the child of > the list view, which we just toggled. If |isChecked| is false in that case, we > could just overwrite its value. Done.
https://codereview.chromium.org/974463002/diff/70001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/70001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:161: if (clickedCheckBox.isChecked()) { Is this the wrong way around now?
https://codereview.chromium.org/974463002/diff/70001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/70001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:161: if (clickedCheckBox.isChecked()) { On 2015/03/03 16:30:28, Bernhard Bauer wrote: > Is this the wrong way around now? Indeed. My bad.
lgtm
The CQ bit was checked by knn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/974463002/#ps110001 (title: "Update toast message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/974463002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by knn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/974463002/110001
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/935a20a1398e28ba5ec3298ae97c941810e618c0 Cr-Commit-Position: refs/heads/master@{#319618}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:110001) has been created in https://codereview.chromium.org/1059453003/ by knn@chromium.org. The reason for reverting is: Need to investigate AlertDialog's getListView method when a custom view is added to it..
Message was sent while issue was closed.
newt@chromium.org changed reviewers: + newt@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/974463002/diff/110001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java (right): https://codereview.chromium.org/974463002/diff/110001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:149: (CheckedTextView) mDialog.getListView().getChildAt(whichButton); This is broken. ListView.getChildAt() returns the Nth *currently visible* child of the ListView, which is definitely not what you want. https://codereview.chromium.org/974463002/diff/110001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java:223: mDialog.getListView().post(new Runnable() { Broken too, for similar reasons. Please add me as a reviewer on the CL to fix this. Thanks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
