Polish for Extension Install Dialog.

Polish for Extension Install Dialog.

- Make sure the bullet is on its own column so that the text that follows doesn't wrap directly under the bullet.
- Add a bit of horizontal padding for the expandable details.
- Make sure the Up/Down arrow is always in the same place so that it doesn't shift horizontally when toggled.

BUG=278328
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222033

[Finnur, 2013-09-06 15:38:41]

This CL does what I want it to do, but triggers a DCHECK in grid_layout.cc@1000
when I try to AddRow the new row using the colset I just added.

  DCHECK(remaining_row_span_ <= 0 ||
         row->column_set() == NULL ||
         row->column_set() == GetLastValidColumnSet());

It seems that GetLastValidColumnSet doesn't return the col set I added but the
one before it... I don't understand why...

It looks like you are the last to change this DCHECK. Do you know what I'm doing
wrong (if anything)? The CL seems to work in other ways.

[sky, 2013-09-06 20:47:28]

GridLayout requires that if you add a view with a row span that you
use the same column set for the rows the view spans. So, you're
hitting the DCHECK because you've added a view with a row span and
switched to a different columnset for one of the rows the view
touches. Don't do this:)

I suspect this constraint could be relaxed, but it would require more
setup and seriously complicate things (both in the API and actual
implementation). Do you really need it?

  -Scott

On Fri, Sep 6, 2013 at 8:38 AM,  <finnur@chromium.org> wrote:
> Reviewers: sky,
>
> Message:
> This CL does what I want it to do, but triggers a DCHECK in
> grid_layout.cc@1000
> when I try to AddRow the new row using the colset I just added.
>
>   DCHECK(remaining_row_span_ <= 0 ||
>          row->column_set() == NULL ||
>          row->column_set() == GetLastValidColumnSet());
>
> It seems that GetLastValidColumnSet doesn't return the col set I added but
> the
> one before it... I don't understand why...
>
> It looks like you are the last to change this DCHECK. Do you know what I'm
> doing
> wrong (if anything)? The CL seems to work in other ways.
>
> Description:
> Polish for Extension Install Dialog.
>
> - Make sure the bullet is on its own.
> - Add a bit of horizontal space for the details of the Permissions.
> - Make sure the Up/Down arrow is always in the same place horizontally.
>
> BUG=278328
>
> Please review this at https://codereview.chromium.org/23890009/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+51, -17 lines):
>   M chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
>
>
> Index: chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
> diff --git
> a/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
> b/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
> index
>
b71fb28bf64bce963f8d4dbd772fee5d1338f715..94eed40b7e5df560a12b46b6be086025e75eefe4
> 100644
> --- a/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
> +++ b/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
> @@ -491,11 +491,39 @@
> ExtensionInstallDialogView::ExtensionInstallDialogView(
>        permissions_header->SizeToFit(left_column_width);
>        layout->AddView(permissions_header);
>
> +      column_set = layout->AddColumnSet(++column_set_id);
> +      int colset_bullet_list = column_set_id;
> +      column_set->AddColumn(views::GridLayout::LEADING,
> +                            views::GridLayout::LEADING,
> +                            0,
> +                            views::GridLayout::USE_PREF,
> +                            0, // no fixed width
> +                            0);
> +      column_set->AddColumn(views::GridLayout::LEADING,
> +                            views::GridLayout::LEADING,
> +                            0,
> +                            views::GridLayout::USE_PREF,
> +                            0,  // no fixed width
> +                            0);
> +
> +      column_set = layout->AddColumnSet(++column_set_id);
> +      int colset_details = column_set_id;
> +      column_set->AddPaddingColumn(
> +          0, 2 * views::kRelatedControlHorizontalSpacing);
> +      column_set->AddColumn(views::GridLayout::LEADING,
> +                            views::GridLayout::LEADING,
> +                            0,
> +                            views::GridLayout::USE_PREF,
> +                            0,  // no fixed width
> +                            0);
> +
> +
>        for (size_t i = 0; i < prompt.GetPermissionCount(); ++i) {
>          layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
> -        layout->StartRow(0, column_set_id);
> -        views::Label* permission_label = new
> views::Label(PrepareForDisplay(
> -            prompt.GetPermission(i), true));
> +        layout->StartRow(0, colset_bullet_list);
> +        layout->AddView(new views::Label(PrepareForDisplay(string16(),
> true)));
> +        views::Label* permission_label = new views::Label(
> +            prompt.GetPermission(i));
>          permission_label->SetMultiLine(true);
>          permission_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
>          permission_label->SizeToFit(left_column_width);
> @@ -503,7 +531,7 @@ ExtensionInstallDialogView::ExtensionInstallDialogView(
>
>          // If we have more details to provide, show them in collapsed form.
>          if (!prompt.GetPermissionsDetails(i).empty()) {
> -          layout->StartRow(0, column_set_id);
> +          layout->StartRow(0, colset_details);
>            PermissionDetails details;
>            details.push_back(
>                PrepareForDisplay(prompt.GetPermissionsDetails(i), false));
> @@ -780,33 +808,39 @@ ExpandableContainerView::ExpandableContainerView(
>    for (size_t i = 0; i < details.size(); ++i)
>      details_view_->AddDetail(details[i]);
>
> -  // Prepare the columns for the More Details row.
> +  views::Link* link = new views::Link(
> +      l10n_util::GetStringUTF16(IDS_EXTENSIONS_SHOW_DETAILS));
> +
> +  // Make sure the link width column is as wide as needed for both Show and
> +  // Hide details, so that the arrow doesn't shift horizontally when we
> toggle.
> +  int link_col_width =
> +      views::kRelatedControlHorizontalSpacing +
> +      std::max(link->font_list().GetStringWidth(
> +                   l10n_util::GetStringUTF16(IDS_EXTENSIONS_HIDE_DETAILS)),
> +               link->font_list().GetStringWidth(
> +
> l10n_util::GetStringUTF16(IDS_EXTENSIONS_SHOW_DETAILS)));
> +
>    column_set = layout->AddColumnSet(++column_set_id);
> +  // Padding to the left of the More Details column.
>    column_set->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
> +  // The More Details column.
>    column_set->AddColumn(views::GridLayout::LEADING,
>                          views::GridLayout::LEADING,
>                          0,
> -                        views::GridLayout::USE_PREF,
> -                        0,
> -                        0);
> -  column_set->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
> +                        views::GridLayout::FIXED,
> +                        link_col_width,
> +                        link_col_width);
> +  // The Up/Down arrow column.
>    column_set->AddColumn(views::GridLayout::LEADING,
>                         views::GridLayout::LEADING,
>                         0,
>                         views::GridLayout::USE_PREF,
>                         0,
>                         0);
> -  column_set->AddColumn(views::GridLayout::LEADING,
> -                        views::GridLayout::LEADING,
> -                        0,
> -                        views::GridLayout::USE_PREF,
> -                        0,
> -                        0);
>
>    // Add the More Details link.
>    layout->StartRow(0, column_set_id);
> -  more_details_ = new views::Link(
> -      l10n_util::GetStringUTF16(IDS_EXTENSIONS_SHOW_DETAILS));
> +  more_details_ = link;
>    more_details_->set_listener(this);
>    more_details_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
>    layout->AddView(more_details_);
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Finnur, 2013-09-09 14:34:03]

https://codereview.chromium.org/23890009/diff/13001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (left):

https://codereview.chromium.org/23890009/diff/13001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:176: bool
show_bullets);
I discovered that this param was always set to false, so I removed it.

https://codereview.chromium.org/23890009/diff/13001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:782: 0);
This column wasn't used, so I eated it.

https://codereview.chromium.org/23890009/diff/13001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/23890009/diff/13001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:194: }
So, to answer your question: No, I don't need to get all complicated with the
grid layout. This serves my purpose nicely (having a sub-layout).

https://codereview.chromium.org/23890009/diff/13001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:206: bool
parent_bulleted);
And then I needed a new param that unfortunately looks related to the one I
removed, but isn't really.

[sky, 2013-09-09 15:00:29]

LGTM

https://codereview.chromium.org/23890009/diff/13001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/23890009/diff/13001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
nit: we don't need to update copyrights anymore.

https://codereview.chromium.org/23890009/diff/13001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:166: // A
simple view that pre-pends a view with a bullet with the help of a grid
prepends

[Finnur, 2013-09-09 15:09:50]

Committed patchset #6 manually as r222033 (presubmit successful).

[please use gerrit instead, 2013-09-09 18:10:45]

On 2013/09/09 15:09:50, Finnur wrote:
> Committed patchset #6 manually as r222033 (presubmit successful).

Untied test ExtensionServiceTest.ExternalInstallMultiple failed on
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%283%29/builds...
after this change. This change looks suspicious, as it did not go through the
commit queue...

[please use gerrit instead, 2013-09-09 18:11:50]

s/untied/unit/

[Finnur, 2013-09-09 23:55:26]

It looks like my CL wasn't to blame, though, and you found the culprit
(flaky test, if I'm reading things correctly). However, if you have
problems with this CL, feel free to revert. I just got home from a visit
with a family member but won't be paying attention to the build bots for a
few hours at least (it is midnight here). :)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.