Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2917323002:
Remove the Allow/Block drop-down from grouped permission requests (Closed)
DescriptionRemove the Allow/Block drop-down from grouped permission requests
Grouped permission requests are being removed, except in the case of
Mic/Camera. In that case, there will not be Allow/Block drop-downs for
each permission requested. Instead there will be 2 individual
Allow/Block buttons as in the case of single permission requests.
BUG=728483
Review-Url: https://codereview.chromium.org/2917323002
Cr-Commit-Position: refs/heads/master@{#477129}
Committed: https://chromium.googlesource.com/chromium/src/+/176bcdca55786801816ab769ffbb9f9969ee98d3
Patch Set 1 #Patch Set 2 : Remove the Allow/Block drop-down from grouped permission requests #
Total comments: 22
Patch Set 3 : Remove the Allow/Block drop-down from grouped permission requests #
Total comments: 2
Patch Set 4 : Remove the Allow/Block drop-down from grouped permission requests #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 23 (14 generated)
The CQ bit was checked by raymes@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
raymes@chromium.org changed reviewers: + msw@chromium.org
msw: ptal, thanks!
The CQ bit was checked by raymes@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nits and qs. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc (right): https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:14: #include "chrome/browser/profiles/profile.h" nit: remove this https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:28: #include "ui/base/models/combobox_model.h" nit: remove https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:37: #include "ui/views/controls/button/menu_button.h" nit: remove https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:38: #include "ui/views/controls/button/menu_button_listener.h" nit: remove https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:39: #include "ui/views/controls/combobox/combobox.h" nit: remove https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:40: #include "ui/views/controls/combobox/combobox_listener.h" nit: remove https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:42: #include "ui/views/controls/menu/menu_runner.h" nit: remove https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:88: std::unique_ptr<PermissionMenuModel> menu_button_model_; nit: remove this? https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:96: const std::vector<PermissionRequest*>& requests) q: does it make sense to change this to just one request or update the DCHECK below to ensure there's exactly one item? (or are there still multiple requests in the mic+camera case?) https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:147: row_layout->AddView(new views::View()); What's this doing? Should we be just skipping/merging a column, or updating the layout to remove an unused column? https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:373: Profile* PermissionPromptImpl::GetProfile() { nit: remove this
Thanks! https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc (right): https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:14: #include "chrome/browser/profiles/profile.h" On 2017/06/05 20:22:20, msw wrote: > nit: remove this Done. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:28: #include "ui/base/models/combobox_model.h" On 2017/06/05 20:22:19, msw wrote: > nit: remove Done. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:37: #include "ui/views/controls/button/menu_button.h" On 2017/06/05 20:22:20, msw wrote: > nit: remove Done. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:38: #include "ui/views/controls/button/menu_button_listener.h" On 2017/06/05 20:22:20, msw wrote: > nit: remove Done. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:39: #include "ui/views/controls/combobox/combobox.h" On 2017/06/05 20:22:19, msw wrote: > nit: remove Done. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:40: #include "ui/views/controls/combobox/combobox_listener.h" On 2017/06/05 20:22:20, msw wrote: > nit: remove Done. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:42: #include "ui/views/controls/menu/menu_runner.h" On 2017/06/05 20:22:19, msw wrote: > nit: remove Done. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:88: std::unique_ptr<PermissionMenuModel> menu_button_model_; On 2017/06/05 20:22:20, msw wrote: > nit: remove this? Done. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:96: const std::vector<PermissionRequest*>& requests) On 2017/06/05 20:22:19, msw wrote: > q: does it make sense to change this to just one request or update the DCHECK > below to ensure there's exactly one item? (or are there still multiple requests > in the mic+camera case?) There can still be multiple requests for the mic+camera case (2). https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:147: row_layout->AddView(new views::View()); On 2017/06/05 20:22:19, msw wrote: > What's this doing? Should we be just skipping/merging a column, or updating the > layout to remove an unused column? Sorry I'm not a views expert by any means! That makes sense though. I removed the column and it still seems to work properly. https://codereview.chromium.org/2917323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:373: Profile* PermissionPromptImpl::GetProfile() { On 2017/06/05 20:22:19, msw wrote: > nit: remove this Done.
The CQ bit was checked by raymes@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/2917323002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc (right): https://codereview.chromium.org/2917323002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:105: // The row is laid out containing a leading-aligned label area. Actually, it seems like this layout could be greatly simplified: -Remove |row| and |row_layout| and |columns|. -Use |AddChildView(label_container)| to add that view directly. -Optionally remove |label_container| and instead use |label->SetBorder(views::CreateEmptyBorder(0, indent, 0, 0));|, then just use |AddChildView(label)|
Thanks! https://codereview.chromium.org/2917323002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc (right): https://codereview.chromium.org/2917323002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:105: // The row is laid out containing a leading-aligned label area. On 2017/06/05 23:14:10, msw wrote: > Actually, it seems like this layout could be greatly simplified: > -Remove |row| and |row_layout| and |columns|. > -Use |AddChildView(label_container)| to add that view directly. > -Optionally remove |label_container| and instead use > |label->SetBorder(views::CreateEmptyBorder(0, indent, 0, 0));|, then just use > |AddChildView(label)| Done. I didn't do the optional bit because I wasn't sure what to do with the icon. I'd rather not change that now though if possible :)
lgtm; I missed the icon, sorry!
The CQ bit was checked by raymes@chromium.org
On 2017/06/05 23:25:06, msw wrote: > lgtm; I missed the icon, sorry! Thanks for the fast responses :)
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": 60001, "attempt_start_ts": 1496705130977650,
"parent_rev": "8ab26031647c8c612b1a051868fdc18c5d7db3f9", "commit_rev":
"176bcdca55786801816ab769ffbb9f9969ee98d3"}
Message was sent while issue was closed.
Description was changed from ========== Remove the Allow/Block drop-down from grouped permission requests Grouped permission requests are being removed, except in the case of Mic/Camera. In that case, there will not be Allow/Block drop-downs for each permission requested. Instead there will be 2 individual Allow/Block buttons as in the case of single permission requests. BUG=728483 ========== to ========== Remove the Allow/Block drop-down from grouped permission requests Grouped permission requests are being removed, except in the case of Mic/Camera. In that case, there will not be Allow/Block drop-downs for each permission requested. Instead there will be 2 individual Allow/Block buttons as in the case of single permission requests. BUG=728483 Review-Url: https://codereview.chromium.org/2917323002 Cr-Commit-Position: refs/heads/master@{#477129} Committed: https://chromium.googlesource.com/chromium/src/+/176bcdca55786801816ab769ffbb... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/176bcdca55786801816ab769ffbb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
