Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2966453002:
Chrome Cleaner UI: Add logs upload permission checkbox to the dialog (Closed)
|
Created:
3 years, 5 months ago by alito Modified:
3 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, alito+watch_chromium.org, chromium-reviews, csharp+watch_chromium.org, darin (slow to review), ftirelo+watch_chromium.org, grt+watch_chromium.org, joenotcharles+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, qsr+mojo_chromium.org, stevenjb+watch-md-settings_chromium.org, tfarina, timvolodine, vakh+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChrome Cleaner UI: Add logs upload permission checkbox to the dialog
BUG=736465
Review-Url: https://codereview.chromium.org/2966453002
Cr-Commit-Position: refs/heads/master@{#483777}
Committed: https://chromium.googlesource.com/chromium/src/+/c6056e1924f90559fbf4649692a78b12b93e7506
Patch Set 1 #Patch Set 2 : Nits #Patch Set 3 : Rebase #
Total comments: 19
Patch Set 4 : Addressed comments #
Total comments: 2
Patch Set 5 : More comments #
Total comments: 11
Messages
Total messages: 40 (13 generated)
The CQ bit was checked by alito@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...
alito@chromium.org changed reviewers: + proberge@chromium.org, robertshield@chromium.org
This adds the permissions checkbox to the Chrome Cleaner dialog and propagates the value to the ChromeCleanerController class. The webui page will need to be updated separately.
https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h:228: // The logs permission checkboxes in the Chrome Clenaer dialog and webui page nit: Cleaner https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:151: profile_, ChromeCleanerController::UserResponse::kAcceptedWithoutLogs); Shouldn't this take the value of the toggle in the web ui?
https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h:228: // The logs permission checkboxes in the Chrome Clenaer dialog and webui page s/Clenaer/Cleaner https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h:229: // are meant to be checked by default. s/meant to be checked by default/opt out https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:270: controller_->SetLogsEnabled(true); Shouldn't the entire controller be reset to a new singleton instance instead? https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.h (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.h:34: void SetLogsEnabled(bool logs_enabled) override; What's the advantage of having both SetLogsEnabled and extra paramters to Accept/DetailsButtonClicked ? If SetLogsEnabled is called every time the checkbox state changes, wouldn't the Accept/DetailsButtonClicked always have the same logs_enabled value as the last SetLogsEnabled? https://codereview.chromium.org/2966453002/diff/40001/components/chrome_clean... File components/chrome_cleaner/public/interfaces/chrome_prompt.mojom (left): https://codereview.chromium.org/2966453002/diff/40001/components/chrome_clean... components/chrome_cleaner/public/interfaces/chrome_prompt.mojom:22: NOT_SHOWN = 1, Is removing NOT_SHOWN intended?
https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:151: profile_, ChromeCleanerController::UserResponse::kAcceptedWithoutLogs); On 2017/06/29 13:47:21, robertshield wrote: > Shouldn't this take the value of the toggle in the web ui? Nm, just saw your comment in the mail. Maybe a TODO?
https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:151: profile_, ChromeCleanerController::UserResponse::kAcceptedWithoutLogs); On 2017/06/29 13:47:21, robertshield wrote: > Shouldn't this take the value of the toggle in the web ui? Yes. @alito please add a TODO under my name above this line.
Patchset #4 (id:60001) has been deleted
Addressed all comments. PTAnL! https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h:228: // The logs permission checkboxes in the Chrome Clenaer dialog and webui page On 2017/06/29 13:47:21, robertshield wrote: > nit: Cleaner Done. https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h:228: // The logs permission checkboxes in the Chrome Clenaer dialog and webui page On 2017/06/29 13:55:52, proberge wrote: > s/Clenaer/Cleaner Done. https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h:229: // are meant to be checked by default. On 2017/06/29 13:55:52, proberge wrote: > s/meant to be checked by default/opt out Ah, thx. That's what I couldn't remember last night :) https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:270: controller_->SetLogsEnabled(true); On 2017/06/29 13:55:53, proberge wrote: > Shouldn't the entire controller be reset to a new singleton instance instead? Done. https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.h (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.h:34: void SetLogsEnabled(bool logs_enabled) override; On 2017/06/29 13:55:53, proberge wrote: > What's the advantage of having both SetLogsEnabled and extra paramters to > Accept/DetailsButtonClicked ? > > If SetLogsEnabled is called every time the checkbox state changes, wouldn't the > Accept/DetailsButtonClicked always have the same logs_enabled value as the last > SetLogsEnabled? The global state can change from the webui page without being reflected in the modal dialog. This ensures that when the user clicks a button on the modal dialog, the actual state of the checkbox at that time is propagated to the controller atomically. https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:151: profile_, ChromeCleanerController::UserResponse::kAcceptedWithoutLogs); On 2017/06/29 13:56:28, robertshield wrote: > On 2017/06/29 13:47:21, robertshield wrote: > > Shouldn't this take the value of the toggle in the web ui? > > Nm, just saw your comment in the mail. Maybe a TODO? Added a TODO assigned to proberge@. https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:151: profile_, ChromeCleanerController::UserResponse::kAcceptedWithoutLogs); On 2017/06/29 13:56:29, proberge wrote: > On 2017/06/29 13:47:21, robertshield wrote: > > Shouldn't this take the value of the toggle in the web ui? > > Yes. @alito please add a TODO under my name above this line. Done. https://codereview.chromium.org/2966453002/diff/40001/components/chrome_clean... File components/chrome_cleaner/public/interfaces/chrome_prompt.mojom (left): https://codereview.chromium.org/2966453002/diff/40001/components/chrome_clean... components/chrome_cleaner/public/interfaces/chrome_prompt.mojom:22: NOT_SHOWN = 1, On 2017/06/29 13:55:53, proberge wrote: > Is removing NOT_SHOWN intended? It is intentional. Chrome is not sending this to the Cleaner from anywhere and I don't see a need for it. I think this was introduced while we were still considering having a single unified reporter and cleaner binary.
lgtm https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:279: controller_->DismissRebootForTesting(); Are all instances of DismissRebootForTesting removed? If so, maybe we can clean it from the controller. https://codereview.chromium.org/2966453002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2966453002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:272: controller_->SetLogsEnabled(true); Can this line be removed now (since the instance should have been reset)?
Thanks PA, addressed all your comments. Robert? https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2966453002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:279: controller_->DismissRebootForTesting(); On 2017/06/29 17:58:36, proberge wrote: > Are all instances of DismissRebootForTesting removed? If so, maybe we can clean > it from the controller. Done. https://codereview.chromium.org/2966453002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2966453002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:272: controller_->SetLogsEnabled(true); On 2017/06/29 17:58:36, proberge wrote: > Can this line be removed now (since the instance should have been reset)? Done.
lgtm https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:176: controller_ = nullptr; double checking: there's no way that Show() would get called on the same instance after this right?
https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:176: controller_ = nullptr; On 2017/06/29 18:43:14, robertshield wrote: > double checking: there's no way that Show() would get called on the same > instance after this right? No. Show() is only called right after the instance is created in ShowChromeCleanerPrompt().
alito@chromium.org changed reviewers: + msw@chromium.org, tommycli@chromium.org
Adding OWNERS: msw@: chrome/browser/ui/views/* tommycli@: chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc PTAL!
On 2017/06/29 19:10:22, alito wrote: > Adding OWNERS: > > msw@: > chrome/browser/ui/views/* > > tommycli@: > chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc > > PTAL! lgtm
https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:107: views::INSETS_DIALOG_CONTENTS))); nit: can you just give the checkbox view an empty border with this inset, instead of nesting the checkbox within a view and using a layout manager to add some border padding? https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:138: controller_->Accept(/*logs_enabled=*/logs_permission_checkbox_->checked()); Consider just calling controller_->SetLogsEnabled before calling Accept (and DetailsButtonClicked) to just use one clear setter function rather than passing a commented bool argument to these other two functions.
Thanks Mike. See response and question inline. https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:107: views::INSETS_DIALOG_CONTENTS))); On 2017/06/29 21:52:48, msw wrote: > nit: can you just give the checkbox view an empty border with this inset, > instead of nesting the checkbox within a view and using a layout manager to add > some border padding? I tried several ways, but I cannot get it to work. I tried, among other things, 1) logs_permission_checkbox_->set_border(CreateEmptyBorder(...)); 2) std::unique_ptr<views::LabelButtonBorder> border = logs_permission_checkbox_->CreateDefaultBorder(); border->set_insets(logs_permission_checkbox_->border()->GetInsets() + ChromeLayoutProvider::Get()->GetInsetsMetric( views::INSETS_DIALOG_CONTENTS)); logs_permission_checkbox_->SetBorder(std::move(border)); 3) Adding the checkbox to a View to which I add an empty border None of them work (for example, the vertical size of the the footnote area remains unchanged irrespective of the added border's top and bottom insets). Any suggestions? https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:138: controller_->Accept(/*logs_enabled=*/logs_permission_checkbox_->checked()); On 2017/06/29 21:52:48, msw wrote: > Consider just calling controller_->SetLogsEnabled before calling Accept (and > DetailsButtonClicked) to just use one clear setter function rather than passing > a commented bool argument to these other two functions. I thought about that. If this dialog was the only place from which the user could accept the cleanup operation that is being offered, I would be all for it. But we are going to have a webui card in the settings page, and later, we are going to introduce a stand-alone page where users can manually request a cleanup. Given the possibly privacy implications of forgetting to call SetLogsEnabled(), I think it makes sense to keep it this way and force all UI surfaces to send the permission bit when the user accepts.
https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:107: views::INSETS_DIALOG_CONTENTS))); On 2017/06/30 00:00:09, alito wrote: > On 2017/06/29 21:52:48, msw wrote: > > nit: can you just give the checkbox view an empty border with this inset, > > instead of nesting the checkbox within a view and using a layout manager to > add > > some border padding? > > I tried several ways, but I cannot get it to work. > > I tried, among other things, > > 1) > logs_permission_checkbox_->set_border(CreateEmptyBorder(...)); > > 2) > std::unique_ptr<views::LabelButtonBorder> border = > logs_permission_checkbox_->CreateDefaultBorder(); > border->set_insets(logs_permission_checkbox_->border()->GetInsets() + > ChromeLayoutProvider::Get()->GetInsetsMetric( > views::INSETS_DIALOG_CONTENTS)); > logs_permission_checkbox_->SetBorder(std::move(border)); > > 3) > Adding the checkbox to a View to which I add an empty border > > None of them work (for example, the vertical size of the the footnote area > remains unchanged irrespective of the added border's top and bottom insets). Any > suggestions? > Hmm, that's strange. What you have is fine. https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:138: controller_->Accept(/*logs_enabled=*/logs_permission_checkbox_->checked()); On 2017/06/30 00:00:09, alito wrote: > On 2017/06/29 21:52:48, msw wrote: > > Consider just calling controller_->SetLogsEnabled before calling Accept (and > > DetailsButtonClicked) to just use one clear setter function rather than > passing > > a commented bool argument to these other two functions. > > I thought about that. If this dialog was the only place from which the user > could accept the cleanup operation that is being offered, I would be all for it. > But we are going to have a webui card in the settings page, and later, we are > going to introduce a stand-alone page where users can manually request a > cleanup. Given the possibly privacy implications of forgetting to call > SetLogsEnabled(), I think it makes sense to keep it this way and force all UI > surfaces to send the permission bit when the user accepts. Hmm, it seems like you really only need to call SetLogsEnabled when the checkbox is pressed, not on each Accept/DetailsButtonClicked call. It seems a better protection of privacy to only set that bit when the user explicitly toggles the checkbox, and not via bool arguments to separate functions. Tangentially: what should happen if the user toggles the checkbox and then clicks cancel/close?
https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:138: controller_->Accept(/*logs_enabled=*/logs_permission_checkbox_->checked()); On 2017/06/30 00:26:36, msw wrote: > On 2017/06/30 00:00:09, alito wrote: > > On 2017/06/29 21:52:48, msw wrote: > > > Consider just calling controller_->SetLogsEnabled before calling Accept (and > > > DetailsButtonClicked) to just use one clear setter function rather than > > passing > > > a commented bool argument to these other two functions. > > > > I thought about that. If this dialog was the only place from which the user > > could accept the cleanup operation that is being offered, I would be all for > it. > > But we are going to have a webui card in the settings page, and later, we are > > going to introduce a stand-alone page where users can manually request a > > cleanup. Given the possibly privacy implications of forgetting to call > > SetLogsEnabled(), I think it makes sense to keep it this way and force all UI > > surfaces to send the permission bit when the user accepts. > > Hmm, it seems like you really only need to call SetLogsEnabled when the checkbox > is pressed, not on each Accept/DetailsButtonClicked call. It seems a better > protection of privacy to only set that bit when the user explicitly toggles the > checkbox, and not via bool arguments to separate functions. Tangentially: what > should happen if the user toggles the checkbox and then clicks cancel/close? Some more details: a logs upload permission checkbox will also be present in Chrome Cleaner's webui page. It is possible (though it should be rare) for a user to have the webui page and the modal dialog open at the same time in different browser windows. Since the dialog's checkbox is not checked/unchecked if a user interacts with the checkbox on the webui page, it is not enough to rely only on calling SetLogsEnabled when the user interacts with the dialog's checkbox. We need to make sure that when the user clicks accept/details in the dialog, it is the state of the dialog's checkbox at that moment that determines the logs upload permission bit. Re your other question, if the user clicks cancel/close, the cleanup operation is canceled and no logs are uploaded (irrespective of the permission bit).
lgtm https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:138: controller_->Accept(/*logs_enabled=*/logs_permission_checkbox_->checked()); On 2017/06/30 00:54:02, alito wrote: > On 2017/06/30 00:26:36, msw wrote: > > On 2017/06/30 00:00:09, alito wrote: > > > On 2017/06/29 21:52:48, msw wrote: > > > > Consider just calling controller_->SetLogsEnabled before calling Accept > (and > > > > DetailsButtonClicked) to just use one clear setter function rather than > > > passing > > > > a commented bool argument to these other two functions. > > > > > > I thought about that. If this dialog was the only place from which the user > > > could accept the cleanup operation that is being offered, I would be all for > > it. > > > But we are going to have a webui card in the settings page, and later, we > are > > > going to introduce a stand-alone page where users can manually request a > > > cleanup. Given the possibly privacy implications of forgetting to call > > > SetLogsEnabled(), I think it makes sense to keep it this way and force all > UI > > > surfaces to send the permission bit when the user accepts. > > > > Hmm, it seems like you really only need to call SetLogsEnabled when the > checkbox > > is pressed, not on each Accept/DetailsButtonClicked call. It seems a better > > protection of privacy to only set that bit when the user explicitly toggles > the > > checkbox, and not via bool arguments to separate functions. Tangentially: what > > should happen if the user toggles the checkbox and then clicks cancel/close? > > Some more details: a logs upload permission checkbox will also be present in > Chrome Cleaner's webui page. It is possible (though it should be rare) for a > user to have the webui page and the modal dialog open at the same time in > different browser windows. > > Since the dialog's checkbox is not checked/unchecked if a user interacts with > the checkbox on the webui page, it is not enough to rely only on calling > SetLogsEnabled when the user interacts with the dialog's checkbox. We need to > make sure that when the user clicks accept/details in the dialog, it is the > state of the dialog's checkbox at that moment that determines the logs upload > permission bit. > > Re your other question, if the user clicks cancel/close, the cleanup operation > is canceled and no logs are uploaded (irrespective of the permission bit). Thanks for the additional details and thinking through edges cases.
still lgtm https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:138: controller_->Accept(/*logs_enabled=*/logs_permission_checkbox_->checked()); On 2017/06/30 01:15:58, msw wrote: > On 2017/06/30 00:54:02, alito wrote: > > On 2017/06/30 00:26:36, msw wrote: > > > On 2017/06/30 00:00:09, alito wrote: > > > > On 2017/06/29 21:52:48, msw wrote: > > > > > Consider just calling controller_->SetLogsEnabled before calling Accept > > (and > > > > > DetailsButtonClicked) to just use one clear setter function rather than > > > > passing > > > > > a commented bool argument to these other two functions. > > > > > > > > I thought about that. If this dialog was the only place from which the > user > > > > could accept the cleanup operation that is being offered, I would be all > for > > > it. > > > > But we are going to have a webui card in the settings page, and later, we > > are > > > > going to introduce a stand-alone page where users can manually request a > > > > cleanup. Given the possibly privacy implications of forgetting to call > > > > SetLogsEnabled(), I think it makes sense to keep it this way and force all > > UI > > > > surfaces to send the permission bit when the user accepts. > > > > > > Hmm, it seems like you really only need to call SetLogsEnabled when the > > checkbox > > > is pressed, not on each Accept/DetailsButtonClicked call. It seems a better > > > protection of privacy to only set that bit when the user explicitly toggles > > the > > > checkbox, and not via bool arguments to separate functions. Tangentially: > what > > > should happen if the user toggles the checkbox and then clicks cancel/close? > > > > Some more details: a logs upload permission checkbox will also be present in > > Chrome Cleaner's webui page. It is possible (though it should be rare) for a > > user to have the webui page and the modal dialog open at the same time in > > different browser windows. > > > > Since the dialog's checkbox is not checked/unchecked if a user interacts with > > the checkbox on the webui page, it is not enough to rely only on calling > > SetLogsEnabled when the user interacts with the dialog's checkbox. We need to > > make sure that when the user clicks accept/details in the dialog, it is the > > state of the dialog's checkbox at that moment that determines the logs upload > > permission bit. > > > > Re your other question, if the user clicks cancel/close, the cleanup operation > > is canceled and no logs are uploaded (irrespective of the permission bit). > > Thanks for the additional details and thinking through edges cases. Actually, I wonder if then there's no need for SetLogsEnabled to be public or called when the checkbox value changes, if it's like a form, the value only need be submitted by Accept and DetailsButtonClicked, and this view doesn't need to be a listener for the checkbox, it need only read the state when calling those functions. Either way, still lgtm
On 2017/06/30 01:18:40, msw wrote: > still lgtm > > https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... > File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): > > https://codereview.chromium.org/2966453002/diff/100001/chrome/browser/ui/view... > chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:138: > controller_->Accept(/*logs_enabled=*/logs_permission_checkbox_->checked()); > On 2017/06/30 01:15:58, msw wrote: > > On 2017/06/30 00:54:02, alito wrote: > > > On 2017/06/30 00:26:36, msw wrote: > > > > On 2017/06/30 00:00:09, alito wrote: > > > > > On 2017/06/29 21:52:48, msw wrote: > > > > > > Consider just calling controller_->SetLogsEnabled before calling > Accept > > > (and > > > > > > DetailsButtonClicked) to just use one clear setter function rather > than > > > > > passing > > > > > > a commented bool argument to these other two functions. > > > > > > > > > > I thought about that. If this dialog was the only place from which the > > user > > > > > could accept the cleanup operation that is being offered, I would be all > > for > > > > it. > > > > > But we are going to have a webui card in the settings page, and later, > we > > > are > > > > > going to introduce a stand-alone page where users can manually request a > > > > > cleanup. Given the possibly privacy implications of forgetting to call > > > > > SetLogsEnabled(), I think it makes sense to keep it this way and force > all > > > UI > > > > > surfaces to send the permission bit when the user accepts. > > > > > > > > Hmm, it seems like you really only need to call SetLogsEnabled when the > > > checkbox > > > > is pressed, not on each Accept/DetailsButtonClicked call. It seems a > better > > > > protection of privacy to only set that bit when the user explicitly > toggles > > > the > > > > checkbox, and not via bool arguments to separate functions. Tangentially: > > what > > > > should happen if the user toggles the checkbox and then clicks > cancel/close? > > > > > > Some more details: a logs upload permission checkbox will also be present in > > > Chrome Cleaner's webui page. It is possible (though it should be rare) for a > > > user to have the webui page and the modal dialog open at the same time in > > > different browser windows. > > > > > > Since the dialog's checkbox is not checked/unchecked if a user interacts > with > > > the checkbox on the webui page, it is not enough to rely only on calling > > > SetLogsEnabled when the user interacts with the dialog's checkbox. We need > to > > > make sure that when the user clicks accept/details in the dialog, it is the > > > state of the dialog's checkbox at that moment that determines the logs > upload > > > permission bit. > > > > > > Re your other question, if the user clicks cancel/close, the cleanup > operation > > > is canceled and no logs are uploaded (irrespective of the permission bit). > > > > Thanks for the additional details and thinking through edges cases. > > Actually, I wonder if then there's no need for SetLogsEnabled to be public or > called when the checkbox value changes, if it's like a form, the value only need > be submitted by Accept and DetailsButtonClicked, and this view doesn't need to > be a listener for the checkbox, it need only read the state when calling those > functions. Either way, still lgtm Thanks Mike for the review! Always good to think through edge cases. I'll land this as is for now, but will leave you with yet another tidbit of info :). Since the user can have multiple Cleaner webui pages open, we felt that those checkbox values should propagate. This is how the settings page works; if you have two tabs open and toggle one setting, the other tab is updated too. Similarly, the Cleaner webui will globally set the permission bit every time it is toggled so that it can be reflected in other tabs.
The CQ bit was checked by alito@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from proberge@chromium.org Link to the patchset: https://codereview.chromium.org/2966453002/#ps100001 (title: "More comments")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
alito@chromium.org changed reviewers: + wfh@chromium.org
Adding wfh@ for OWNERS approval for: components/chrome_cleaner/public/interfaces/chrome_prompt.mojom PTAL.
alito@chromium.org changed reviewers: + palmer@chromium.org - wfh@chromium.org
Adding palmer@ for OWNER's approval since wfh@ is traveling. PTAL at components/chrome_cleaner/public/interfaces/chrome_prompt.mojom
lgtm
Thanks for all the reviews!
The CQ bit was checked by alito@chromium.org
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": 100001, "attempt_start_ts": 1498848157707580,
"parent_rev": "72c64b20a7b001954f3445947efe01eb7bc96272", "commit_rev":
"c6056e1924f90559fbf4649692a78b12b93e7506"}
Message was sent while issue was closed.
Description was changed from ========== Chrome Cleaner UI: Add logs upload permission checkbox to the dialog BUG=736465 ========== to ========== Chrome Cleaner UI: Add logs upload permission checkbox to the dialog BUG=736465 Review-Url: https://codereview.chromium.org/2966453002 Cr-Commit-Position: refs/heads/master@{#483777} Committed: https://chromium.googlesource.com/chromium/src/+/c6056e1924f90559fbf4649692a7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c6056e1924f90559fbf4649692a7... |
