Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2011963002:
PermissionSelectorView: use Combobox on MacViews builds. (Closed)
|
Created:
4 years, 7 months ago by Elly Fong-Jones Modified:
4 years, 6 months ago Reviewers:
palmer CC:
chromium-reviews, tfarina, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPermissionSelectorView: use Combobox on MacViews builds.
The Cocoa version of this dialog uses the cocoa equivalent of Comboboxes. Also,
Comboboxes on MacViews have appropriate styling, whereas MenuButtons don't and
changing them to have it seems quite involved.
This CL involves adapting the permission selector's MenuModel to a
ComboboxModel, then creating a Combobox instead of a MenuButton on Mac.
BUG=605161
Committed: https://crrev.com/5acf0c2022fc685f2250a54eea3b6e2bfc90dc60
Cr-Commit-Position: refs/heads/master@{#399034}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Fix nits #Patch Set 3 : Document DCHECK #Patch Set 4 : Give up on SetPermissionInfo #
Messages
Total messages: 27 (11 generated)
Description was changed from ========== PermissionSelectorView: use Combobox on MacViews builds. The Cocoa version of this dialog uses the cocoa equivalent of Comboboxes. Also, Comboboxes on MacViews have appropriate styling, whereas MenuButtons don't and changing them to have it seems quite involved. This CL involves adapting the permission selector's MenuModel to a ComboboxModel, then creating a Combobox instead of a MenuButton on Mac. BUG=605161 ========== to ========== PermissionSelectorView: use Combobox on MacViews builds. The Cocoa version of this dialog uses the cocoa equivalent of Comboboxes. Also, Comboboxes on MacViews have appropriate styling, whereas MenuButtons don't and changing them to have it seems quite involved. This CL involves adapting the permission selector's MenuModel to a ComboboxModel, then creating a Combobox instead of a MenuButton on Mac. BUG=605161 ==========
ellyjones@chromium.org changed reviewers: + palmer@chromium.org
palmer: ptal? :)
Some nits. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permission_selector_view.cc (right): https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:118: // This class adapts a PermissionMenuModel into a ui::ComboboxModel so that Nit: Demarcate identifiers in comments with |foo|. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:134: base::string16 GetItemAt(int index) override; Nit: Since this is not a collection of string16s, I wouldn't call them the "items". Since it's a convenience wrapper/way to expose model_->GetLabelAt, I'd call this |GetLabelAt|, too. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:146: for (int i = 0; i < model_->GetItemCount(); i++) { Nit: I think the Chromium style calls for ++i. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:149: checked_index = i; I think you can early-return here instead: return i; and then return -1 at the bottom. And then you don't need (what is currently) line 145. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:294: // Technically, the MenuButton has the same problem, but MenuButton doesn't Nit: Blank line before a new paragraph. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permission_selector_view.h (right): https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.h:53: const WebsiteSettingsUI::PermissionInfo& perm); Nit: Identifiers in interfaces should be full words. You could go with |info| (promoted to a 'full word' by convention, I suppose; but that is something). https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.h:59: std::unique_ptr<internal::ComboboxModelAdapter> combobox_model_adaptor_; Nit: Standardize on "adaptor" vs. "adapter". Since |internal::ComboboxModelAdapter| is already established, I'd go with -er. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.h:63: internal::PermissionCombobox* combobox_; // Owned by the views heirarchy. Nit: Typo: "hierarchy"
palmer: ptal? :) https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permission_selector_view.cc (right): https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:118: // This class adapts a PermissionMenuModel into a ui::ComboboxModel so that On 2016/05/25 18:34:38, palmer wrote: > Nit: Demarcate identifiers in comments with |foo|. Done. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:134: base::string16 GetItemAt(int index) override; On 2016/05/25 18:34:38, palmer wrote: > Nit: Since this is not a collection of string16s, I wouldn't call them the > "items". Since it's a convenience wrapper/way to expose model_->GetLabelAt, I'd > call this |GetLabelAt|, too. Unfortunately, these two names are defined in ui::ComboboxModel :( https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:146: for (int i = 0; i < model_->GetItemCount(); i++) { On 2016/05/25 18:34:39, palmer wrote: > Nit: I think the Chromium style calls for ++i. Done. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:149: checked_index = i; On 2016/05/25 18:34:38, palmer wrote: > I think you can early-return here instead: > > return i; > > and then return -1 at the bottom. And then you don't need (what is currently) > line 145. I avoided early return so that I could DCHECK that there's only one checked item, which isn't something the PermissionMenuModel normally guarantees. Do you think it's better to leave it as is with the DCHECK, or pull the DCHECK and early return here? https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:294: // Technically, the MenuButton has the same problem, but MenuButton doesn't On 2016/05/25 18:34:38, palmer wrote: > Nit: Blank line before a new paragraph. Done. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permission_selector_view.h (right): https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.h:53: const WebsiteSettingsUI::PermissionInfo& perm); On 2016/05/25 18:34:39, palmer wrote: > Nit: Identifiers in interfaces should be full words. You could go with |info| > (promoted to a 'full word' by convention, I suppose; but that is something). Done. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.h:59: std::unique_ptr<internal::ComboboxModelAdapter> combobox_model_adaptor_; On 2016/05/25 18:34:39, palmer wrote: > Nit: Standardize on "adaptor" vs. "adapter". Since > |internal::ComboboxModelAdapter| is already established, I'd go with -er. Done. https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.h:63: internal::PermissionCombobox* combobox_; // Owned by the views heirarchy. On 2016/05/25 18:34:39, palmer wrote: > Nit: Typo: "hierarchy" Done.
lgtm https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permission_selector_view.cc (right): https://codereview.chromium.org/2011963002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_view.cc:149: checked_index = i; > I avoided early return so that I could DCHECK that there's only one checked > item, which isn't something the PermissionMenuModel normally guarantees. Ahh, I see. Maybe add a comment to that effect. > Do you > think it's better to leave it as is with the DCHECK, or pull the DCHECK and > early return here? Leave it as-is, I'd say.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org Link to the patchset: https://codereview.chromium.org/2011963002/#ps40001 (title: "Document DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011963002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011963002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
what's the status of this patch? It will be helpful to me for fixing crbug.com/615308
On 2016/06/01 21:22:44, Evan Stade wrote: > what's the status of this patch? It will be helpful to me for fixing > crbug.com/615308 I am trying to figure out whether the broken unit tests represent a serious design error or just tests that should be updated. Right now, one of the tests constructs a PermissionInfo whose setting is CONTENT_SETTING_DEFAULT instead of one of the ALLOW/BLOCK/ASK options, and that results in *none* of the options in the MenuModel being checked. The reason this gets reasonable behavior with the MenuButton is that the text is explicitly set to the result of PermissionActionToUIString(), but for Comboboxes, that doesn't make any sense, so I'm trying to figure out how to deal with this. One approach that I was considering was to try to guess which option is the "default" (always the 0th element, maybe?). What do you think?
On 2016/06/02 14:42:03, Elly Jones wrote: > On 2016/06/01 21:22:44, Evan Stade wrote: > > what's the status of this patch? It will be helpful to me for fixing > > crbug.com/615308 > > I am trying to figure out whether the broken unit tests represent a serious > design error or just tests that should be updated. Right now, one of the tests > constructs a PermissionInfo whose setting is CONTENT_SETTING_DEFAULT instead of > one of the ALLOW/BLOCK/ASK options, and that results in *none* of the options in > the MenuModel being checked. The reason this gets reasonable behavior with the > MenuButton is that the text is explicitly set to the result of > PermissionActionToUIString(), but for Comboboxes, that doesn't make any sense, > so I'm trying to figure out how to deal with this. One approach that I was > considering was to try to guess which option is the "default" (always the 0th > element, maybe?). What do you think? After much further thought and experimentation, I think that this test simply doesn't work / make sense with a Combobox, so I disabled it for Mac. palmer@, does that sound legit? (see comment in the source as well)
The CQ bit was checked by ellyjones@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011963002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org Link to the patchset: https://codereview.chromium.org/2011963002/#ps60001 (title: "Give up on SetPermissionInfo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011963002/60001
Still LGTM.
Message was sent while issue was closed.
Description was changed from ========== PermissionSelectorView: use Combobox on MacViews builds. The Cocoa version of this dialog uses the cocoa equivalent of Comboboxes. Also, Comboboxes on MacViews have appropriate styling, whereas MenuButtons don't and changing them to have it seems quite involved. This CL involves adapting the permission selector's MenuModel to a ComboboxModel, then creating a Combobox instead of a MenuButton on Mac. BUG=605161 ========== to ========== PermissionSelectorView: use Combobox on MacViews builds. The Cocoa version of this dialog uses the cocoa equivalent of Comboboxes. Also, Comboboxes on MacViews have appropriate styling, whereas MenuButtons don't and changing them to have it seems quite involved. This CL involves adapting the permission selector's MenuModel to a ComboboxModel, then creating a Combobox instead of a MenuButton on Mac. BUG=605161 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== PermissionSelectorView: use Combobox on MacViews builds. The Cocoa version of this dialog uses the cocoa equivalent of Comboboxes. Also, Comboboxes on MacViews have appropriate styling, whereas MenuButtons don't and changing them to have it seems quite involved. This CL involves adapting the permission selector's MenuModel to a ComboboxModel, then creating a Combobox instead of a MenuButton on Mac. BUG=605161 ========== to ========== PermissionSelectorView: use Combobox on MacViews builds. The Cocoa version of this dialog uses the cocoa equivalent of Comboboxes. Also, Comboboxes on MacViews have appropriate styling, whereas MenuButtons don't and changing them to have it seems quite involved. This CL involves adapting the permission selector's MenuModel to a ComboboxModel, then creating a Combobox instead of a MenuButton on Mac. BUG=605161 Committed: https://crrev.com/5acf0c2022fc685f2250a54eea3b6e2bfc90dc60 Cr-Commit-Position: refs/heads/master@{#399034} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5acf0c2022fc685f2250a54eea3b6e2bfc90dc60 Cr-Commit-Position: refs/heads/master@{#399034} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
