Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2306673003:
Material Page Info (Views, 3/3): Update site settings section. (Closed)
|
Created:
4 years, 3 months ago by lgarron Modified:
4 years, 2 months ago Reviewers:
msw CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaterial Page Info (Views, 3/3): Update site settings section.
This includes:
- Changing PermissionSelectorRow() to add to a grid layout instead of being its own view.
- Renaming some of the views to reflect their current role (cookies_view_, permission_view_, permissions_view_)
- Updating a lot of padding values for consistency, and renaming some of them to match the Mac values.
BUG=512442
Committed: https://crrev.com/c6607f9c83b3c81acb07eb9fc1496dfc814f6a0b
Cr-Commit-Position: refs/heads/master@{#424247}
Patch Set 1 #Patch Set 2 : Massively manual rebasing. #Patch Set 3 : Rebase. #Patch Set 4 : Alignment fixes! #Patch Set 5 : Remove CreateSection() and rename cookies view. #Patch Set 6 : Refactor padding details. #Patch Set 7 : Pointers. #
Total comments: 68
Patch Set 8 : Address comments. #
Total comments: 8
Patch Set 9 : Material Page Info (Views, 3/3): Update site settings section. #Patch Set 10 #Patch Set 11 #Patch Set 12 #Patch Set 13 : Update layout. #
Total comments: 2
Patch Set 14 : Reintroduce kMinSeparationBetweenLabelAndMenu. #Patch Set 15 : Remove redundant this->? #
Total comments: 19
Patch Set 16 : Material Page Info (Views, 3/3): Update site settings section. #
Total comments: 3
Patch Set 17 : Fix up unused includes. #
Depends on Patchset: Messages
Total messages: 35 (15 generated)
Description was changed from ========== [IGNORE FOR NOW] Material Page Info (Views, 3/3): Update site settings section. BUG=512442 ========== to ========== Material Page Info (Views, 3/3): Update site settings section. BUG=512442 ==========
Description was changed from ========== Material Page Info (Views, 3/3): Update site settings section. BUG=512442 ========== to ========== Material Page Info (Views, 3/3): Update site settings section. This includes: - Changing PermissionSelectorRow() to add to a grid layout instead of being its on view. - Renaming some of the views to reflect their current role (cookies_view_, permission_view_, permissions_view_) - Updating a lot of padding values for consistency, and renaming some of them to match the Mac values. BUG=512442 ==========
lgarron@chromium.org changed reviewers: + msw@chromium.org
msw@, could you review? I *think* I fully understand all the code I've changed and made sensible decisions, but I'd be happy to learn if I'm wrong anywhere. :-) https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:75: /**************** General constants ****************/ I've updated the structure and variable names in the constant declaration namespace to match the new Mac implementation more closely. [1] https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/website_settings...
https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chosen_object_row.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.cc:30: label_with_delete_ = new views::View(); Why create a view to contain the label and the delete button? Can't they be added directly to the grid layout? Can you post a screenshot of the delete button or point to any on the bug? (I can't find one) https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.cc:32: new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 5); optional nit: use a views layout constant (kRelatedButtonHSpacing is 6), or a function/file-local constant, instead of the literal '5'. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.cc:39: ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta(1); I find it odd to just toss around 1px taller fonts in random places. It's unclear how this relates to the title and other elements in the bubble, and having these size delta literals scattered throughout the directory makes it difficult to reason about the overall layout and font size consistency/relations. Since we are phasing out ResourceBundle::FontStyle and related constants, I wonder if we should have a centralized place to put font size deltas for Views UI or website settings. WDYT? Maybe ping tapted? https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chosen_object_row.h (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.h:29: explicit ChosenObjectRow( nit: remove explicit https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (left): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:269: IDS_WEBSITE_SETTINGS_PERMISSION_TYPE, Remove string resources if they are no longer used. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:27: #include "ui/views/layout/box_layout.h" Remove; not used. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:33: // Minimum distance between the label and its corresponding menu. nit: '... in DIPs.' or similar. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:241: : icon_(NULL), nit: nullptr here and elsewhere https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:250: views::View* wrapper = new views::View(); Ditto: it'd be nice to avoid this view, but not necessary, I suppose. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:268: ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta(1); Ditto: consider adding constants for text sizes. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (left): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:588: IDS_WEBSITE_SETTINGS_FIRST_PARTY_SITE_DATA, i.allowed); Remove string resources that are no longer used. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:591: IDS_WEBSITE_SETTINGS_THIRD_PARTY_SITE_DATA, i.allowed); Remove string resources that are no longer used. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:75: /**************** General constants ****************/ nit: consider a more common chrome/* comment heading pattern, like: // General constants ----------------------------------------------------------- https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:82: const int kSectionPaddingHorizontal = 16; nit: try to use a views constant, like kUnrelatedControlHorizontalSpacing (12), kUnrelatedControlLargeHorizontalSpacing (20) or similar. Ditto for other values, where it makes sense. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:104: const int kPermissionImageSize = 16; Does this only apply to vector icons or are image resources being scaled down? If we are shrinking icon sizes, should we shrink any image resources? https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:550: // that Fix line wrapping. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:591: width = std::max(width, header_->GetPreferredNameWidth()); Did this rely on the addition of padding values? width += kSiteSettingsViewPaddingLeft + kSiteSettingsViewPaddingRight; If so, a long string here might not fit when the padding is applied. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:604: int total_allowed = 0; nit: can you just do something like: int allowed = cookie_info_list[0].allowed + cookie_info_list[1].allowed;? https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:605: for (const auto& i : cookie_info_list) { nit: curlies not needed; s/i/cookie/ https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:608: label_text = l10n_util::GetPluralStringFUTF16( nit: combine the declaration and assignment of |label_text| in one statement. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:651: const gfx::FontList& font_list = rb.GetFontListWithDelta(1); Ditto https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:704: if (chosen_object_info_list.size() > 0) { nit: This seems unnecessary, given the loop below; if not, use !empty. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:707: // layout->SkipColumns(1); Remove commented-out code; please do a close self-review first. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:711: // layout->AddView(object_view, 1, 2, views::GridLayout::LEADING, Remove commented-out code; please do a close self-review first. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:720: views::LabelButton* site_settings_button_ = Does this conflict with a member identifier? Don't use underscores post-fix for local variables. Remove the |site_settings_button_| class member if it's unused. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:727: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { nit: box_layout->set_main_axis_alignment(ui::MaterialDesignController::IsSecondaryUiMaterial() ? views::BoxLayout::MAIN_AXIS_ALIGNMENT_END : views::BoxLayout::MAIN_AXIS_ALIGNMENT_START); https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:777: box_layout->set_inside_border_insets( nit: move above with the other |box_layout| init (set_cross_axis_alignment) https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:165: views::Link* site_settings_button_; nit: avoid 'button' naming for views::Link instances.
https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chosen_object_row.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.cc:30: label_with_delete_ = new views::View(); On 2016/09/26 at 20:47:55, msw wrote: > Why create a view to contain the label and the delete button? Can't they be added directly to the grid layout? Can you post a screenshot of the delete button or point to any on the bug? (I can't find one) They could be added directly, but then I'd need to use a different column set, and redundantly implement the icon layout for it. I just realized I forgot to upload a recent screenshot with a chosen object. See https://bugs.chromium.org/p/chromium/issues/detail?id=512442#c92 https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.cc:32: new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 5); On 2016/09/26 at 20:47:55, msw wrote: > optional nit: use a views layout constant (kRelatedButtonHSpacing is 6), or a function/file-local constant, instead of the literal '5'. Thanks for the tip; I've changed to views::kRelatedButtonHSpacing. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.cc:39: ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta(1); On 2016/09/26 at 20:47:55, msw wrote: > I find it odd to just toss around 1px taller fonts in random places. It's unclear how this relates to the title and other elements in the bubble, and having these size delta literals scattered throughout the directory makes it difficult to reason about the overall layout and font size consistency/relations. Since we are phasing out ResourceBundle::FontStyle and related constants, I wonder if we should have a centralized place to put font size deltas for Views UI or website settings. WDYT? Maybe ping tapted? Brought it up with Max (designer) and Emily (PM) and you CCed on the design doc. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chosen_object_row.h (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.h:29: explicit ChosenObjectRow( On 2016/09/26 at 20:47:55, msw wrote: > nit: remove explicit Done. At the risk of asking in the wrong place, why was it appropriate for the views::View subclass but not now? https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (left): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:269: IDS_WEBSITE_SETTINGS_PERMISSION_TYPE, On 2016/09/26 at 20:47:55, msw wrote: > Remove string resources if they are no longer used. *woo, another one finally bites the dust* Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:27: #include "ui/views/layout/box_layout.h" On 2016/09/26 at 20:47:55, msw wrote: > Remove; not used. Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:33: // Minimum distance between the label and its corresponding menu. On 2016/09/26 at 20:47:55, msw wrote: > nit: '... in DIPs.' or similar. Aren't all layout constants using the same scale? https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:241: : icon_(NULL), On 2016/09/26 at 20:47:55, msw wrote: > nit: nullptr here and elsewhere Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:250: views::View* wrapper = new views::View(); On 2016/09/26 at 20:47:55, msw wrote: > Ditto: it'd be nice to avoid this view, but not necessary, I suppose. I tried hard to avoid this, but it seems to be necessary. Each row needs to contain: 1. An icon. 2. A left-aligned label. 3. A right-aligned menu dropdown. 1. should be shared for all rows, but the location of the horizontal padding between 2. and 3. needs to be unique to each row (see the Flash row at [1]). I only know of two practical solutions: - Use a separate column set for each row. - This. I *think* this implementation is the most reasonable. [1] https://crbug.com/512442#c87 https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (left): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:588: IDS_WEBSITE_SETTINGS_FIRST_PARTY_SITE_DATA, i.allowed); On 2016/09/26 at 20:47:55, msw wrote: > Remove string resources that are no longer used. Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:591: IDS_WEBSITE_SETTINGS_THIRD_PARTY_SITE_DATA, i.allowed); On 2016/09/26 at 20:47:56, msw wrote: > Remove string resources that are no longer used. Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:75: /**************** General constants ****************/ On 2016/09/26 at 20:47:56, msw wrote: > nit: consider a more common chrome/* comment heading pattern, like: > // General constants ----------------------------------------------------------- Done. Thanks for letting me know the convention; I couldn't find an example! https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:82: const int kSectionPaddingHorizontal = 16; On 2016/09/26 at 20:47:56, msw wrote: > nit: try to use a views constant, like kUnrelatedControlHorizontalSpacing (12), kUnrelatedControlLargeHorizontalSpacing (20) or similar. Ditto for other values, where it makes sense. This is the outside padding for each section, which I guess matches kPanelHorizMargin (13). Should I prefer matching the mocks as closely as possible, or using those values? https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:104: const int kPermissionImageSize = 16; On 2016/09/26 at 20:47:55, msw wrote: > Does this only apply to vector icons or are image resources being scaled down? If we are shrinking icon sizes, should we shrink any image resources? All image resources are 16px (default_100_percent) or 32px (default_200_percent) PNGs [1], although hopefully we will switch to vector [2]. Since the source images are the right size, I believe we do not need to scale. The icons have already landed and look correct. [1] https://codereview.chromium.org/2269513003 [2] https://crbug.com/647551 https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:550: // that On 2016/09/26 at 20:47:56, msw wrote: > Fix line wrapping. Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:591: width = std::max(width, header_->GetPreferredNameWidth()); On 2016/09/26 at 20:47:56, msw wrote: > Did this rely on the addition of padding values? > width += kSiteSettingsViewPaddingLeft + kSiteSettingsViewPaddingRight; > If so, a long string here might not fit when the padding is applied. Indeed, yes. But now that you've prompted me to look into it, there are two issues: This line "should" be width = std::max(width, header_->GetPreferredNameWidth()) + 2*kSectionPaddingHorizontal; However, the grid cell for the label actually needs to keep a distance of kHeaderPaddingForCloseButton + (width of close button) from the right. In addition, the text seems to use ellipses instead of wrapping when it's too wide. I believe the right choice is actually to: - Set `summary_label_->SetMultiLine(true);` - Not take into account the label for width sizing. I'm double-checking with the designer that this is okay, but I think it's a safe choice. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:604: int total_allowed = 0; On 2016/09/26 at 20:47:56, msw wrote: > nit: can you just do something like: > int allowed = cookie_info_list[0].allowed + cookie_info_list[1].allowed;? I could, but that would be inconsistent with the (identical) Mac code. Which of this and the Mac code would you prefer to update? https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:605: for (const auto& i : cookie_info_list) { On 2016/09/26 at 20:47:56, msw wrote: > nit: curlies not needed; s/i/cookie/ Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:608: label_text = l10n_util::GetPluralStringFUTF16( On 2016/09/26 at 20:47:56, msw wrote: > nit: combine the declaration and assignment of |label_text| in one statement. Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:704: if (chosen_object_info_list.size() > 0) { On 2016/09/26 at 20:47:55, msw wrote: > nit: This seems unnecessary, given the loop below; if not, use !empty. Hmm, this seems to be from an old version of the code before refactoring, but I can't find why. Removed. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:707: // layout->SkipColumns(1); On 2016/09/26 at 20:47:56, msw wrote: > Remove commented-out code; please do a close self-review first. Done, sorry about that (I actually did several passes, I promise :-( ). https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:711: // layout->AddView(object_view, 1, 2, views::GridLayout::LEADING, On 2016/09/26 at 20:47:56, msw wrote: > Remove commented-out code; please do a close self-review first. Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:720: views::LabelButton* site_settings_button_ = On 2016/09/26 at 20:47:56, msw wrote: > Does this conflict with a member identifier? Don't use underscores post-fix for local variables. Remove the |site_settings_button_| class member if it's unused. It used to be a member, but should not be anymore. I've removed the member variable and removed the underscore. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:727: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { On 2016/09/26 at 20:47:55, msw wrote: > nit: box_layout->set_main_axis_alignment(ui::MaterialDesignController::IsSecondaryUiMaterial() ? views::BoxLayout::MAIN_AXIS_ALIGNMENT_END : views::BoxLayout::MAIN_AXIS_ALIGNMENT_START); Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:777: box_layout->set_inside_border_insets( On 2016/09/26 at 20:47:56, msw wrote: > nit: move above with the other |box_layout| init (set_cross_axis_alignment) Done. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:165: views::Link* site_settings_button_; On 2016/09/26 at 20:47:56, msw wrote: > nit: avoid 'button' naming for views::Link instances. Even when the link is styled like a button?
https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chosen_object_row.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.cc:30: label_with_delete_ = new views::View(); On 2016/09/28 21:11:45, lgarron wrote: > On 2016/09/26 at 20:47:55, msw wrote: > > Why create a view to contain the label and the delete button? Can't they be > > added directly to the grid layout? > They could be added directly, but then I'd need to use a different column set, > and redundantly implement the icon layout for it. It'd be nice to avoid unnecessary containers, but I suppose it's okay as-is. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chosen_object_row.h (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.h:29: explicit ChosenObjectRow( On 2016/09/28 21:11:46, lgarron wrote: > On 2016/09/26 at 20:47:55, msw wrote: > > nit: remove explicit > > Done. > > At the risk of asking in the wrong place, why was it appropriate for the > views::View subclass but not now? The explicit keyword is a guard against implicit construction invocation when the ctor only has one argument. Now that the ctor has two arguments, the explicit keyword guard is no longer needed. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:33: // Minimum distance between the label and its corresponding menu. On 2016/09/28 21:11:46, lgarron wrote: > On 2016/09/26 at 20:47:55, msw wrote: > > nit: '... in DIPs.' or similar. > > Aren't all layout constants using the same scale? Yes, most Views code uses DIPs, so specifying isn't needed. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:250: views::View* wrapper = new views::View(); On 2016/09/28 21:11:46, lgarron wrote: > On 2016/09/26 at 20:47:55, msw wrote: > > Ditto: it'd be nice to avoid this view, but not necessary, I suppose. > > I tried hard to avoid this, but it seems to be necessary. > > Each row needs to contain: > 1. An icon. > 2. A left-aligned label. > 3. A right-aligned menu dropdown. > > 1. should be shared for all rows, but the location of the horizontal padding > between 2. and 3. needs to be unique to each row (see the Flash row at [1]). > > I only know of two practical solutions: > - Use a separate column set for each row. > - This. > > I *think* this implementation is the most reasonable. > > [1] https://crbug.com/512442#c87 Ah, so all rows would share the same width for a given column if they use the same column set... I guess this is fine. Could you instead make this wrapper just use BoxLayout? https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:82: const int kSectionPaddingHorizontal = 16; On 2016/09/28 21:11:46, lgarron wrote: > On 2016/09/26 at 20:47:56, msw wrote: > > nit: try to use a views constant, like kUnrelatedControlHorizontalSpacing > (12), kUnrelatedControlLargeHorizontalSpacing (20) or similar. Ditto for other > values, where it makes sense. > > This is the outside padding for each section, which I guess matches > kPanelHorizMargin (13). Should I prefer matching the mocks as closely as > possible, or using those values? Sadly, there's no one-size-fits-all answer to that question; it's up to you when to follow the mocks closely and when to follow standards/conventions. I'd err on the side of using consts and very rarely tweaking those values (to fit MD/Harmony/hotness++) and *very* rarely use one-off magic values. If it were up to me, we'd have a comprehensive set of constants and the designers would define mocks in terms of those values... ie. instead of a mock saying 'put 5px between these controls', it should say 'these controls are related, use the standard horizontal padding for related controls between them'. /rant https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:604: int total_allowed = 0; On 2016/09/28 21:11:47, lgarron wrote: > On 2016/09/26 at 20:47:56, msw wrote: > > nit: can you just do something like: > > int allowed = cookie_info_list[0].allowed + cookie_info_list[1].allowed;? > > I could, but that would be inconsistent with the (identical) Mac code. > Which of this and the Mac code would you prefer to update? This is fine as-is https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:165: views::Link* site_settings_button_; On 2016/09/28 21:11:47, lgarron wrote: > On 2016/09/26 at 20:47:56, msw wrote: > > nit: avoid 'button' naming for views::Link instances. > > Even when the link is styled like a button? If it were actually a views::Link*, then yes. https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:517: bool canDropAtLocation = Is this from a bad rebase? https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h (right): https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h:67: // Returns true if it's okay to drop dragged data into the view at the Ditto: Is this from a bad rebase? https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm (right): https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:427: - (BOOL)canDropAtLocationInWindow:(NSPoint)location Ditto: Is this from a bad rebase? https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:242: combobox_(nullptr) { // Create the permission icon. nit: comment belongs on next line, right? https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:758: box_layout->set_inside_border_insets( nit: move above with the other |box_layout| init (set_cross_axis_alignment) (you said 'Done.', but didn't change it; please be careful about that...)
Description was changed from ========== Material Page Info (Views, 3/3): Update site settings section. This includes: - Changing PermissionSelectorRow() to add to a grid layout instead of being its on view. - Renaming some of the views to reflect their current role (cookies_view_, permission_view_, permissions_view_) - Updating a lot of padding values for consistency, and renaming some of them to match the Mac values. BUG=512442 ========== to ========== Material Page Info (Views, 3/3): Update site settings section. This includes: - Changing PermissionSelectorRow() to add to a grid layout instead of being its own view. - Renaming some of the views to reflect their current role (cookies_view_, permission_view_, permissions_view_) - Updating a lot of padding values for consistency, and renaming some of them to match the Mac values. BUG=512442 ==========
https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chosen_object_row.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.cc:30: label_with_delete_ = new views::View(); On 2016/09/30 at 00:37:04, msw wrote: > On 2016/09/28 21:11:45, lgarron wrote: > > On 2016/09/26 at 20:47:55, msw wrote: > > > Why create a view to contain the label and the delete button? Can't they be > > > added directly to the grid layout? > > They could be added directly, but then I'd need to use a different column set, > > and redundantly implement the icon layout for it. > > It'd be nice to avoid unnecessary containers, but I suppose it's okay as-is. Acknowledged. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chosen_object_row.h (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chosen_object_row.h:29: explicit ChosenObjectRow( On 2016/09/30 at 00:37:04, msw wrote: > On 2016/09/28 21:11:46, lgarron wrote: > > On 2016/09/26 at 20:47:55, msw wrote: > > > nit: remove explicit > > > > Done. > > > > At the risk of asking in the wrong place, why was it appropriate for the > > views::View subclass but not now? > > The explicit keyword is a guard against implicit construction invocation when the ctor only has one argument. Now that the ctor has two arguments, the explicit keyword guard is no longer needed. Ah, that makes sense, thanks. https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:250: views::View* wrapper = new views::View(); On 2016/09/30 at 00:37:04, msw wrote: > On 2016/09/28 21:11:46, lgarron wrote: > > On 2016/09/26 at 20:47:55, msw wrote: > > > Ditto: it'd be nice to avoid this view, but not necessary, I suppose. > > > > I tried hard to avoid this, but it seems to be necessary. > > > > Each row needs to contain: > > 1. An icon. > > 2. A left-aligned label. > > 3. A right-aligned menu dropdown. > > > > 1. should be shared for all rows, but the location of the horizontal padding > > between 2. and 3. needs to be unique to each row (see the Flash row at [1]). > > > > I only know of two practical solutions: > > - Use a separate column set for each row. > > - This. > > > > I *think* this implementation is the most reasonable. > > > > [1] https://crbug.com/512442#c87 > > Ah, so all rows would share the same width for a given column if they use the same column set... I guess this is fine. Could you instead make this wrapper just use BoxLayout? BoxLayout doesn't support MAIN_AXIS_ALIGNMENT_JUSTIFY [1]. :-( I spent some time trying to see if I could implement it, but I decided it wasn't worth my time if GridLayout already works. https://cs.chromium.org/chromium/src/ui/views/layout/box_layout.h?q=boxlayout... https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:82: const int kSectionPaddingHorizontal = 16; On 2016/09/30 at 00:37:04, msw wrote: > On 2016/09/28 21:11:46, lgarron wrote: > > On 2016/09/26 at 20:47:56, msw wrote: > > > nit: try to use a views constant, like kUnrelatedControlHorizontalSpacing > > (12), kUnrelatedControlLargeHorizontalSpacing (20) or similar. Ditto for other > > values, where it makes sense. > > > > This is the outside padding for each section, which I guess matches > > kPanelHorizMargin (13). Should I prefer matching the mocks as closely as > > possible, or using those values? > > Sadly, there's no one-size-fits-all answer to that question; it's up to you when to follow the mocks closely and when to follow standards/conventions. I'd err on the side of using consts and very rarely tweaking those values (to fit MD/Harmony/hotness++) and *very* rarely use one-off magic values. If it were up to me, we'd have a comprehensive set of constants and the designers would define mocks in terms of those values... ie. instead of a mock saying 'put 5px between these controls', it should say 'these controls are related, use the standard horizontal padding for related controls between them'. /rant Yeah, I wish translating specs to code didn't require so much interpretation. :-/ I'd prefer to match the values from the specs as closely as possible for now, though, since consensus on them seems fragile. (I did try to consolidate some constants, though.) Do the designers know about `ui/views/layout/layout_constants.h`? It mentions "spec 21/4", which is still a mysterious name to me. I asked about it once and got no answer. Do you know? https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:165: views::Link* site_settings_button_; On 2016/09/30 00:37:04, msw wrote: > On 2016/09/28 21:11:47, lgarron wrote: > > On 2016/09/26 at 20:47:56, msw wrote: > > > nit: avoid 'button' naming for views::Link instances. > > > > Even when the link is styled like a button? > > If it were actually a views::Link*, then yes. Acknowledged (although the var has now been removed.) https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:517: bool canDropAtLocation = On 2016/09/30 00:37:04, msw wrote: > Is this from a bad rebase? Yes, sorry. (I recently switched to using dependent branches, and this CL accidentally ended up with an extra commit on its branch in the one-time transition.) https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:242: combobox_(nullptr) { // Create the permission icon. On 2016/09/30 at 00:37:04, msw wrote: > nit: comment belongs on next line, right? Moved. https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:758: box_layout->set_inside_border_insets( On 2016/09/30 00:37:04, msw wrote: > nit: move above with the other |box_layout| init (set_cross_axis_alignment) > (you said 'Done.', but didn't change it; please be careful about that...) I definitely remember moving this. 😯 I've made sure it moved in patch 9, and double-checked the upload.
lgtm https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:82: const int kSectionPaddingHorizontal = 16; On 2016/09/30 05:05:55, lgarron wrote: > On 2016/09/30 at 00:37:04, msw wrote: > > On 2016/09/28 21:11:46, lgarron wrote: > > > On 2016/09/26 at 20:47:56, msw wrote: > > > > nit: try to use a views constant, like kUnrelatedControlHorizontalSpacing > > > (12), kUnrelatedControlLargeHorizontalSpacing (20) or similar. Ditto for > other > > > values, where it makes sense. > > > > > > This is the outside padding for each section, which I guess matches > > > kPanelHorizMargin (13). Should I prefer matching the mocks as closely as > > > possible, or using those values? > > > > Sadly, there's no one-size-fits-all answer to that question; it's up to you > when to follow the mocks closely and when to follow standards/conventions. I'd > err on the side of using consts and very rarely tweaking those values (to fit > MD/Harmony/hotness++) and *very* rarely use one-off magic values. If it were up > to me, we'd have a comprehensive set of constants and the designers would define > mocks in terms of those values... ie. instead of a mock saying 'put 5px between > these controls', it should say 'these controls are related, use the standard > horizontal padding for related controls between them'. /rant > > Yeah, I wish translating specs to code didn't require so much interpretation. > :-/ > I'd prefer to match the values from the specs as closely as possible for now, > though, since consensus on them seems fragile. (I did try to consolidate some > constants, though.) > > Do the designers know about `ui/views/layout/layout_constants.h`? > It mentions "spec 21/4", which is still a mysterious name to me. I asked about > it once and got no answer. Do you know? That line dates back to initial commit, or perhaps some history was trashed, perhaps it doesn't mean anything to anyone anymore. I doubt most designers know/care about existing layout constants, native theme colors, or other such stuff, maybe they have their own list and just inline literal values into the spec? https://chromium.googlesource.com/chromium/src/+blame/cee34b707ac41e83ff9d904...
The CQ bit was checked by lgarron@chromium.org
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 lgarron@chromium.org
msw@: I switch back to leaving PermissionSelectorRow/ChosenObjectRow into views. This is a more minimal change, has the correct behaviour after changing a dropdown, and doesn't require a significant change to adapt to the current tests. It still requires some layout duplication, but I can live with that. Assuming this still looks good to you, I plan to revert https://codereview.chromium.org/2272793007 (Material Page Info (Views, 1/3): Rename {ChosenObject, PermissionSelector}View classes to {...}Row.) as a followup.
https://codereview.chromium.org/2306673003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:243: column_set->AddPaddingColumn(1, 16); Woops, this should be a constant. Fixing.
Patchset #14 (id:260001) has been deleted
https://codereview.chromium.org/2306673003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:243: column_set->AddPaddingColumn(1, 16); On 2016/10/07 at 00:40:08, lgarron wrote: > Woops, this should be a constant. Fixing. Done. https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:513: SizeToContents(); Is it okay to call this in the observer callback, or should I rely on the Views hierarchy (e.g. implement WebsiteSettingsPopupView::ChildPreferredSizeChanged() and call SizeToContents() in it)?
I'll do a quick pass now, but wanted to send out this comment first. Please excuse my delay in reviewing your latest changes here. https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:513: SizeToContents(); On 2016/10/07 00:54:36, lgarron wrote: > Is it okay to call this in the observer callback, or should I rely on the Views > hierarchy (e.g. implement WebsiteSettingsPopupView::ChildPreferredSizeChanged() > and call SizeToContents() in it)? Making a ChildPreferredSizeChanged override that calls SizeToContents seems preferable to me, it's already done for a couple other bubbles and seems more direct and complete. (for example, that should handle chosen object changes too, I'm not sure exactly how that's working otherwise at the moment)
mostly minor comments and nits https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:15: #include "ui/base/l10n/l10n_util.h" nit: remove https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:365: ChildPreferredSizeChanged(menu_button_); Can this just continue calling |menu_button_->SizeToPreferredSize();| or could it instead call menu_button_->PreferredSizeChanged? Nothing else calls ChildPreferredSizeChanged for a particular child view like this. https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:84: // Margin and padding values shared by all sections. nit: omit comment and line above (line 81 comment can be shared) https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:100: // Spacing before and after the cookies view. nit: s/before and after/above and below/ https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:104: const int kPermissionImageSize = 16; nit: any chance we could replace/merge this with kPermissionIconColumnWidth? https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:578: int width = kMinPopupWidth; q: just checking; should this consider the |cookies_view_|, |permissions_view_|, or |header_| preferred widths? https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:745: views::View* site_settings_view = new views::View(); aside: it'd be nice to avoid this container (but maybe that makes layout tough), and it'd be nice to refactor some of the |cookies_view_| nit, but don't go out of your way any further in this CL, I've already caused you too much trouble. Anyway, just something to consider for followup. https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:13: #include "base/strings/string16.h" nit: remove
https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:365: ChildPreferredSizeChanged(menu_button_); On 2016/10/08 at 00:21:57, msw wrote: > Can this just continue calling |menu_button_->SizeToPreferredSize();| or could it instead call menu_button_->PreferredSizeChanged? Nothing else calls ChildPreferredSizeChanged for a particular child view like this. Yes, it can! That does seem to work. https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:84: // Margin and padding values shared by all sections. On 2016/10/08 at 00:21:57, msw wrote: > nit: omit comment and line above (line 81 comment can be shared) Comment updated instead. https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:100: // Spacing before and after the cookies view. On 2016/10/08 at 00:21:57, msw wrote: > nit: s/before and after/above and below/ Done. https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:104: const int kPermissionImageSize = 16; On 2016/10/08 at 00:21:57, msw wrote: > nit: any chance we could replace/merge this with kPermissionIconColumnWidth? Replaced. https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:513: SizeToContents(); On 2016/10/07 at 23:56:23, msw wrote: > On 2016/10/07 00:54:36, lgarron wrote: > > Is it okay to call this in the observer callback, or should I rely on the Views > > hierarchy (e.g. implement WebsiteSettingsPopupView::ChildPreferredSizeChanged() > > and call SizeToContents() in it)? > > Making a ChildPreferredSizeChanged override that calls SizeToContents seems preferable to me, it's already done for a couple other bubbles and seems more direct and complete. (for example, that should handle chosen object changes too, I'm not sure exactly how that's working otherwise at the moment) Hmm, I can't figure out the right combination of ChildPreferredSizeChanged/SizeToPreferredSize/SizeToContents to implement/call. Will have to pick it up another day. :-( https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:578: int width = kMinPopupWidth; On 2016/10/08 at 00:21:57, msw wrote: > q: just checking; should this consider the |cookies_view_|, |permissions_view_|, or |header_| preferred widths? cookies_view and permissions_view_: handled through site_settings_view_ header_: Not taken into account for preferred width (see https://codereview.chromium.org/2400883002/ to make sure it reflows when the bubble changes size). https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:745: views::View* site_settings_view = new views::View(); On 2016/10/08 at 00:21:57, msw wrote: > aside: it'd be nice to avoid this container (but maybe that makes layout tough), and it'd be nice to refactor some of the |cookies_view_| nit, but don't go out of your way any further in this CL, I've already caused you too much trouble. Anyway, just something to consider for followup. Ack.
lgtm with nits (some repeated). Please try to respond to all comments, including nits you do not to intend to address. (ie. I'm not sure why you might intentionally leave unused includes) https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:513: SizeToContents(); On 2016/10/08 03:50:04, lgarron wrote: > On 2016/10/07 at 23:56:23, msw wrote: > > On 2016/10/07 00:54:36, lgarron wrote: > > > Is it okay to call this in the observer callback, or should I rely on the > Views > > > hierarchy (e.g. implement > WebsiteSettingsPopupView::ChildPreferredSizeChanged() > > > and call SizeToContents() in it)? > > > > Making a ChildPreferredSizeChanged override that calls SizeToContents seems > preferable to me, it's already done for a couple other bubbles and seems more > direct and complete. (for example, that should handle chosen object changes too, > I'm not sure exactly how that's working otherwise at the moment) > > > Hmm, I can't figure out the right combination of > ChildPreferredSizeChanged/SizeToPreferredSize/SizeToContents to implement/call. > Will have to pick it up another day. :-( Acknowledged. https://codereview.chromium.org/2306673003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:578: int width = kMinPopupWidth; On 2016/10/08 03:50:04, lgarron wrote: > On 2016/10/08 at 00:21:57, msw wrote: > > q: just checking; should this consider the |cookies_view_|, > |permissions_view_|, or |header_| preferred widths? > > cookies_view and permissions_view_: handled through site_settings_view_ > header_: Not taken into account for preferred width (see > https://codereview.chromium.org/2400883002/ to make sure it reflows when the > bubble changes size). Acknowledged. https://codereview.chromium.org/2306673003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2306673003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permission_selector_row.cc:15: #include "ui/base/l10n/l10n_util.h" repeated nit: remove https://codereview.chromium.org/2306673003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2306673003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:13: #include "base/strings/string16.h" repeated nit: remove https://codereview.chromium.org/2306673003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc (right): https://codereview.chromium.org/2306673003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:73: return base::ASCIIToUTF16(""); optional nit: return base::string16();
Apologies for not removing those includes. I was not able to see them because of a Rietveld bug: https://crbug.com/516802 :-( Have removed in the latest patch.
On 2016/10/10 19:09:53, lgarron wrote: > Apologies for not removing those includes. I was not able to see them because of > a Rietveld bug: https://crbug.com/516802 :-( > > Have removed in the latest patch. Ah, kudos for dogfooding the new code review tools/ui. lgtm
The CQ bit was checked by lgarron@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2400883002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by lgarron@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.
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #17 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Material Page Info (Views, 3/3): Update site settings section. This includes: - Changing PermissionSelectorRow() to add to a grid layout instead of being its own view. - Renaming some of the views to reflect their current role (cookies_view_, permission_view_, permissions_view_) - Updating a lot of padding values for consistency, and renaming some of them to match the Mac values. BUG=512442 ========== to ========== Material Page Info (Views, 3/3): Update site settings section. This includes: - Changing PermissionSelectorRow() to add to a grid layout instead of being its own view. - Renaming some of the views to reflect their current role (cookies_view_, permission_view_, permissions_view_) - Updating a lot of padding values for consistency, and renaming some of them to match the Mac values. BUG=512442 Committed: https://crrev.com/c6607f9c83b3c81acb07eb9fc1496dfc814f6a0b Cr-Commit-Position: refs/heads/master@{#424247} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/c6607f9c83b3c81acb07eb9fc1496dfc814f6a0b Cr-Commit-Position: refs/heads/master@{#424247} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
