Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2725783004:
views: align columns in site settings dialog (Closed)
|
Created:
3 years, 9 months ago by Elly Fong-Jones Modified:
3 years, 9 months ago CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[WIP] views: align columns in site settings dialog
Instead of each row being a View with its own internal GridLayout,
impose one GridLayout on the entire dialog, which forces the columns
into alignment.
BUG=535074
Review-Url: https://codereview.chromium.org/2725783004
Cr-Commit-Position: refs/heads/master@{#456089}
Committed: https://chromium.googlesource.com/chromium/src/+/b242d570b855dceae68d4da75c76186d5d7f8011
Patch Set 1 #Patch Set 2 : fix unit tests, document stuff #
Total comments: 8
Patch Set 3 : document more #
Total comments: 8
Patch Set 4 : fix nits #
Total comments: 17
Patch Set 5 : fixes #
Messages
Total messages: 24 (8 generated)
Description was changed from ========== [WIP] views: align columns in site settings dialog Instead of each row being a View with its own internal GridLayout, impose one GridLayout on the entire dialog, which forces the columns into alignment. BUG=535074 ========== to ========== [WIP] views: align columns in site settings dialog Instead of each row being a View with its own internal GridLayout, impose one GridLayout on the entire dialog, which forces the columns into alignment. BUG=535074 ==========
ellyjones@chromium.org changed reviewers: + sky@chromium.org
sky: ptal? :)
I started reviewing this, but the local owners are more familiar with the code than I. Could you get one of them to review it? Perhaps benwells or felt? https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:332: menu_button_->InvalidateLayout(); Who calls Layout? https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:343: return menu_button_ ? static_cast<views::View*>(menu_button_) Why do you need the cast here? https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.h (right): https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.h:50: views::GridLayout* container); The name container implies a view, where as |container| is really a GridLayout. Maybe grid_layout, or layout which is consistent with other names in this calss. https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.h:67: views::View* button(); Add description of this, it isn't at all obvious that this is one of menu_button_ or combobox_.
ellyjones@chromium.org changed reviewers: + palmer@chromium.org
palmer: ptal? you're the domain expert here :) sky: done, ptal for Views stuff? https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:332: menu_button_->InvalidateLayout(); On 2017/03/02 20:05:41, sky wrote: > Who calls Layout? Done. https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:343: return menu_button_ ? static_cast<views::View*>(menu_button_) On 2017/03/02 20:05:41, sky wrote: > Why do you need the cast here? The types of the arms of a ?: cannot be different types T1 and T2 even if both T1 and T2 convert to T and the result of the eventual expression is a T. :( https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.h (right): https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.h:50: views::GridLayout* container); On 2017/03/02 20:05:41, sky wrote: > The name container implies a view, where as |container| is really a GridLayout. > Maybe grid_layout, or layout which is consistent with other names in this calss. Done. https://codereview.chromium.org/2725783004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.h:67: views::View* button(); On 2017/03/02 20:05:41, sky wrote: > Add description of this, it isn't at all obvious that this is one of > menu_button_ or combobox_. Done.
palmer owns all this code, so I don't think you need me. I'm happy to review views specific bits if palmer isn't comfortable.
Some nits idiosyncratic to me, but the code and general approach LGTM. https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:333: // WebsiteSettingsPopupView may need to resize itself to accomodate the new Nit: "...the |WebsiteSettingsPopupView| level, since that view may..." https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.h (right): https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.h:69: // Combobox (combobox_). Nit (This is nit-tacular): Be consistent with demarcating C++ identifiers with |...| throughout: |Combobox|, |menu_button_|, |PermissionSelectorRow|, et c. It is possible to go insane achieving perfect consistency, but I enjoy that particular madness. Not everyone does, and I'm OK with that... ;) Potential compromises: "A |PermissionSelectorRow| is... A PSR is not itself a |View|, but..." I.e. acronyms for verbose class names after being introduced. "Returns the control for this row, which is used to change the permission's value. The control is either |menu_button_| or |combobox_|." I.e. let the reader look up the precise class of the member fields if they want. (Or you could leave out the member field names and mention the class names instead.) https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:632: const int perms_column = 1; Nit: I'd rather use full words: |permissions_column|. https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:179: std::vector<std::unique_ptr<PermissionSelectorRow>> selector_rows_; Nit: "These rows bundle together all the |View|s involved in a single row of the permissions section, and keep those views updated when the underlying |Permission| changes."
sky: would you mind reviewing also? thanks :) https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:333: // WebsiteSettingsPopupView may need to resize itself to accomodate the new On 2017/03/06 19:16:10, palmer wrote: > Nit: "...the |WebsiteSettingsPopupView| level, since that view may..." Done. https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.h (right): https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.h:69: // Combobox (combobox_). On 2017/03/06 19:16:10, palmer wrote: > Nit (This is nit-tacular): Be consistent with demarcating C++ identifiers with > |...| throughout: |Combobox|, |menu_button_|, |PermissionSelectorRow|, et c. > > It is possible to go insane achieving perfect consistency, but I enjoy that > particular madness. Not everyone does, and I'm OK with that... ;) > > Potential compromises: > > "A |PermissionSelectorRow| is... A PSR is not itself a |View|, but..." > > I.e. acronyms for verbose class names after being introduced. > > "Returns the control for this row, which is used to change the permission's > value. The control is either |menu_button_| or |combobox_|." > > I.e. let the reader look up the precise class of the member fields if they want. > (Or you could leave out the member field names and mention the class names > instead.) Done. https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:632: const int perms_column = 1; On 2017/03/06 19:16:10, palmer wrote: > Nit: I'd rather use full words: |permissions_column|. Done. https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2725783004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:179: std::vector<std::unique_ptr<PermissionSelectorRow>> selector_rows_; On 2017/03/06 19:16:10, palmer wrote: > Nit: "These rows bundle together all the |View|s involved in a single row of the > permissions section, and keep those views updated when the underlying > |Permission| changes." Done.
https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:346: return menu_button_ ? static_cast<views::View*>(menu_button_) I'm surprised you need a cast here. https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.h (right): https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.h:20: #include "ui/views/view.h" Forward declare this? https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:469: Layout(); How come you Layout() here? https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc (right): https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:59: return static_cast<PermissionSelectorRow*>( Do you need this cast? https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:60: view_->selector_rows_.at(index).get()); .at(index) -> [index] ? https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:65: if (view->GetClassName() == views::Label::kViewClassName) This is subtle and worth a comment. https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:184: 3 * (ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled() ? 11 : 13); The 3 is subtle and worth a comment. I suggest a constant that is shared with the 3 below that has the comment. https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:192: EXPECT_NE(nullptr, selector); EXPECT(selector)?
sky: ptal? :) https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:346: return menu_button_ ? static_cast<views::View*>(menu_button_) On 2017/03/07 22:03:40, sky wrote: > I'm surprised you need a cast here. You can't have different types on the arms of a ?:, even if the two types are both about to be upcast to the same type. :( If you try, you get this: error: incompatible operand types ('internal::PermissionMenuButton *' and 'internal::PermissionCombobox *') I'll put a comment to explain this, because it does look pretty weird. https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.h (right): https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.h:20: #include "ui/views/view.h" On 2017/03/07 22:03:40, sky wrote: > Forward declare this? Done. https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:469: Layout(); On 2017/03/07 22:03:40, sky wrote: > How come you Layout() here? If we don't, the sizes of the individual permission selectors might not change after their values have changed. If we did the layout in the individual selectors instead (where the InvalidateLayout is), the WebsiteSettingsPopupView won't resize to be wide enough to contain the selectors, so we do the Layout here instead. I tried to explain this in the comment above but it seems I failed :( https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc (right): https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:59: return static_cast<PermissionSelectorRow*>( On 2017/03/07 22:03:40, sky wrote: > Do you need this cast? Nope, deleted. https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:60: view_->selector_rows_.at(index).get()); On 2017/03/07 22:03:40, sky wrote: > .at(index) -> [index] ? Done. https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:65: if (view->GetClassName() == views::Label::kViewClassName) On 2017/03/07 22:03:40, sky wrote: > This is subtle and worth a comment. Done. https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:184: 3 * (ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled() ? 11 : 13); On 2017/03/07 22:03:40, sky wrote: > The 3 is subtle and worth a comment. I suggest a constant that is shared with > the 3 below that has the comment. Done. https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:192: EXPECT_NE(nullptr, selector); On 2017/03/07 22:03:40, sky wrote: > EXPECT(selector)? Done.
LGTM https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:469: Layout(); On 2017/03/08 18:41:09, Elly Fong-Jones wrote: > On 2017/03/07 22:03:40, sky wrote: > > How come you Layout() here? > > If we don't, the sizes of the individual permission selectors might not change > after their values have changed. If we did the layout in the individual > selectors instead (where the InvalidateLayout is), the WebsiteSettingsPopupView > won't resize to be wide enough to contain the selectors, so we do the Layout > here instead. I tried to explain this in the comment above but it seems I failed > :( Are you sure you don't want the Layout() after the SizeToContents()?
On 2017/03/08 18:50:35, sky wrote: > LGTM > > https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... > File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc > (right): > > https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... > chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:469: > Layout(); > On 2017/03/08 18:41:09, Elly Fong-Jones wrote: > > On 2017/03/07 22:03:40, sky wrote: > > > How come you Layout() here? > > > > If we don't, the sizes of the individual permission selectors might not change > > after their values have changed. If we did the layout in the individual > > selectors instead (where the InvalidateLayout is), the > WebsiteSettingsPopupView > > won't resize to be wide enough to contain the selectors, so we do the Layout > > here instead. I tried to explain this in the comment above but it seems I > failed > > :( > > Are you sure you don't want the Layout() after the SizeToContents()? I am not - I was imitating the other code in this file, which seems to Layout() before SizeToContents(). I'm not sure how to decide which order is right.
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/2725783004/#ps80001 (title: "fixes")
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": 80001, "attempt_start_ts": 1489164578730110,
"parent_rev": "e0e3c52cbc129defc3e8b8e2ff20d16b63d15563", "commit_rev":
"b242d570b855dceae68d4da75c76186d5d7f8011"}
Message was sent while issue was closed.
Description was changed from ========== [WIP] views: align columns in site settings dialog Instead of each row being a View with its own internal GridLayout, impose one GridLayout on the entire dialog, which forces the columns into alignment. BUG=535074 ========== to ========== [WIP] views: align columns in site settings dialog Instead of each row being a View with its own internal GridLayout, impose one GridLayout on the entire dialog, which forces the columns into alignment. BUG=535074 Review-Url: https://codereview.chromium.org/2725783004 Cr-Commit-Position: refs/heads/master@{#456089} Committed: https://chromium.googlesource.com/chromium/src/+/b242d570b855dceae68d4da75c76... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/b242d570b855dceae68d4da75c76...
Message was sent while issue was closed.
On 2017/03/10 16:49:33, Elly Fong-Jones wrote: > On 2017/03/08 18:50:35, sky wrote: > > LGTM > > > > > https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... > > File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc > > (right): > > > > > https://codereview.chromium.org/2725783004/diff/60001/chrome/browser/ui/views... > > chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:469: > > Layout(); > > On 2017/03/08 18:41:09, Elly Fong-Jones wrote: > > > On 2017/03/07 22:03:40, sky wrote: > > > > How come you Layout() here? > > > > > > If we don't, the sizes of the individual permission selectors might not > change > > > after their values have changed. If we did the layout in the individual > > > selectors instead (where the InvalidateLayout is), the > > WebsiteSettingsPopupView > > > won't resize to be wide enough to contain the selectors, so we do the Layout > > > here instead. I tried to explain this in the comment above but it seems I > > failed > > > :( > > > > Are you sure you don't want the Layout() after the SizeToContents()? > > I am not - I was imitating the other code in this file, which seems to Layout() > before SizeToContents(). I'm not sure how to decide which order is right. It's a little weird to have a Layout() followed by a size, as presumably the SizeToContents() is going to change the size, which triggers a layout. But I think if the size hasn't changed nothing happens.
Message was sent while issue was closed.
lgarron@chromium.org changed reviewers: + lgarron@chromium.org
Message was sent while issue was closed.
aargh, I wish I already had a watchlist for this. This CL causes the bubble to be very wide when there is a long permission name e.g. "MIDI devices full control" and a long permission value. The latter can happen in some locales by accident, and could also by triggered by certain bugs, two of which I've filed in the last half year. I commented about this on https://bugs.chromium.org/p/chromium/issues/detail?id=535074#c38 but didn't get a response; did UI approve the change out of band?
Message was sent while issue was closed.
On 2017/03/13 21:01:08, lgarron wrote: > aargh, I wish I already had a watchlist for this. > > This CL causes the bubble to be very wide when there is a long permission name > e.g. "MIDI devices full control" and a long permission value. The latter can > happen in some locales by accident, and could also by triggered by certain bugs, > two of which I've filed in the last half year. > > I commented about this on > https://bugs.chromium.org/p/chromium/issues/detail?id=535074#c38 but didn't get > a response; did UI approve the change out of band? I didn't ask them specifically about this but it seems like the only reasonable path forward. If the permission name is 400 pts long in text, the bubble must also be at least 400 pts wide to accomodate it without clipping the text. patricialor@ landed a related change to use short strings throughout, though - does that help?
Message was sent while issue was closed.
On 2017/03/13 at 21:05:24, ellyjones wrote: > On 2017/03/13 21:01:08, lgarron wrote: > > aargh, I wish I already had a watchlist for this. > > > > This CL causes the bubble to be very wide when there is a long permission name > > e.g. "MIDI devices full control" and a long permission value. The latter can > > happen in some locales by accident, and could also by triggered by certain bugs, > > two of which I've filed in the last half year. > > > > I commented about this on > > https://bugs.chromium.org/p/chromium/issues/detail?id=535074#c38 but didn't get > > a response; did UI approve the change out of band? > > I didn't ask them specifically about this but it seems like the only reasonable path forward. If the permission name is 400 pts long in text, the bubble must also be at least 400 pts wide to accomodate it without clipping the text. > > patricialor@ landed a related change to use short strings throughout, though - does that help? The short string change helps, but I don't know about edge cases in other languages. See https://bugs.chromium.org/p/chromium/issues/detail?id=512442#c92 for a case in September where this CL would have added 50% to the Page Info width. That particular case can't happen right now, but I want to make sure UI is agreed that we'd rather widen the bubble in case there is ever a similar combination of long strings on different lines. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
