Merged permissions in the App Info dialog into a single list

Merged permissions in the App Info dialog into a single list

Merged all the permissions in the App Info dialog (including files and
retained devices) into a single list. Also moved the two 'revoke'
buttons to just be X's next to their respective entries in the list.
Screenshots on bug.

BUG=420461
Committed: https://crrev.com/2eae4366fbe574eda8a4c9027716b27ade352d45
Cr-Commit-Position: refs/heads/master@{#301021}

[sashab, 2014-10-19 23:30:34]

sashab@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2014-10-20 00:08:41]

before I delve in... did you consider a views::TableView? 
DevicePermissionsDialogView uses one of these. I don't really know how flexible
they are, but there's a lot of repeated layout code here that might go away..

https://codereview.chromium.org/661353002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/661353002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:2247: +          Access <ph
name="NUMBER_OF_FILES">#<ex>1</ex></ph> files stored on your computer
is this right?

https://codereview.chromium.org/661353002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:2302: +        Communicate with <ph
name="NUMBER_OF_DEVICES">#<ex>1</ex></ph> USB devices
Is this right?

https://codereview.chromium.org/661353002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
(right):

https://codereview.chromium.org/661353002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:32:
static const int kBulletColumnSetId = 1;
nit: `static` is redundant. (and `const` also implies internal linkage so the
anon-namespace is also technically redundant, but we tend to keep that).

[sashab, 2014-10-20 00:45:29]

I think a TableView looks more like the screenshots near the end of this design
doc:
https://docs.google.com/a/chromium.org/document/d/1NtWARiV0JBavA-8bj0Q1pBJSua...

It's for when a grid has sortable columns etc. Here, I just need it for aligning
all the bullets in the same column.

I don't think a lot of this is redundant. Maybe I could make it a little shorter
by taking advantage of the column constructor defaults, but what it's saying is
actually important (align all the bullets in a column, align all the nested
bullets in a column, squish the bullet/revoke button to as small as possible
whilst stretching the permission text, etc).

Alternatively, I could use a flex BoxLayout to just make each bullet a single
row with the same properties. However, GridLayout gives me the peace of mind
that the bullets will always be aligned with eachother (also, I seem to remember
BoxLayout having some trouble with multilined strings, but that could be fixed
now).

https://codereview.chromium.org/661353002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/661353002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:2247: +          Access <ph
name="NUMBER_OF_FILES">#<ex>1</ex></ph> files stored on your computer
On 2014/10/20 00:08:41, tapted wrote:
> is this right?

Done.

https://codereview.chromium.org/661353002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:2302: +        Communicate with <ph
name="NUMBER_OF_DEVICES">#<ex>1</ex></ph> USB devices
On 2014/10/20 00:08:41, tapted wrote:
> Is this right?

Done.

https://codereview.chromium.org/661353002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
(right):

https://codereview.chromium.org/661353002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:32:
static const int kBulletColumnSetId = 1;
On 2014/10/20 00:08:41, tapted wrote:
> nit: `static` is redundant. (and `const` also implies internal linkage so the
> anon-namespace is also technically redundant, but we tend to keep that).

Done.

[tapted, 2014-10-20 06:54:09]

some comments/thoughts below, but someone else will have to take over #flees :)

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
(right):

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:39:
views::LabelButton* btn = views::BubbleFrameView::CreateCloseButton(listener);
This feels a bit weird. (IMO it also looks funny). I reckon a nicer result would
be to use the close_2{,_hover,_pressed}.png resources. And... for everyone using
that, convention seems to be to just load the resources at each place rather
than leaning on an existing dialog.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:72:
const int kSpacingBetweenBulletAndStartOfText = 5;
I'd move these up with the other constants around line 30, and give them a
comment.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:131:
for (const auto& message : GetActivePermissionMessages()) {
nit: no curlies

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:132:
AddPermissionBullet(false, message, true, gfx::NO_ELIDE, NULL);
I kinda feel like there should be a `PermissionsView` that inherits from
views::View to capture all this. To construct it would take a heading, a
possibly empty array of strings for files/devices, and a closure for revoking
the permission.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:179:
vertical_stack->AddChildView(no_permissions_text);
It looks like permissions_list_ and the things it owns
(permissions_list_layout_) are leaked in this code path?

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:197:
// Extract only the bullet from the IDS_EXTENSION_PERMISSION_LINE text.
This doesn't feel right... You should be able to align the bullets with some
kind of padding -- maybe even an empty/fixed-width views::View to take up a
column. Creating a view for each bullet feels really heavy-weight.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:311:
GetRetainedDeviceCount());
It would be nice to share some of this boilerplate with the code that inspired
the version here.  I reviewed that code too (:p), but there wasn't really much
to be gained then. Maybe it just has to look like this.

[sashab, 2014-10-22 04:58:35]

sashab@chromium.org changed reviewers:
+ benwells@chromium.org

[sashab, 2014-10-22 04:58:36]

Ptal Ben :D

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
(right):

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:39:
views::LabelButton* btn = views::BubbleFrameView::CreateCloseButton(listener);
On 2014/10/20 06:54:09, tapted (OOO until Nov 1st) wrote:
> This feels a bit weird. (IMO it also looks funny). I reckon a nicer result
would
> be to use the close_2{,_hover,_pressed}.png resources. And... for everyone
using
> that, convention seems to be to just load the resources at each place rather
> than leaning on an existing dialog.

Agreed with UI on a different set of icons - done.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:72:
const int kSpacingBetweenBulletAndStartOfText = 5;
On 2014/10/20 06:54:09, tapted (OOO until Nov 1st) wrote:
> I'd move these up with the other constants around line 30, and give them a
> comment. 

Done.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:131:
for (const auto& message : GetActivePermissionMessages()) {
On 2014/10/20 06:54:09, tapted (OOO until Nov 1st) wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:132:
AddPermissionBullet(false, message, true, gfx::NO_ELIDE, NULL);
On 2014/10/20 06:54:09, tapted (OOO until Nov 1st) wrote:
> I kinda feel like there should be a `PermissionsView` that inherits from
> views::View to capture all this. To construct it would take a heading, a
> possibly empty array of strings for files/devices, and a closure for revoking
> the permission.

Done.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:179:
vertical_stack->AddChildView(no_permissions_text);
On 2014/10/20 06:54:09, tapted (OOO until Nov 1st) wrote:
> It looks like permissions_list_ and the things it owns
> (permissions_list_layout_) are leaked in this code path?

Fixed.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:197:
// Extract only the bullet from the IDS_EXTENSION_PERMISSION_LINE text.
On 2014/10/20 06:54:09, tapted (OOO until Nov 1st) wrote:
> This doesn't feel right... You should be able to align the bullets with some
> kind of padding -- maybe even an empty/fixed-width views::View to take up a
> column. Creating a view for each bullet feels really heavy-weight.

I really feel that this is the best way to do it. I don't think it's
heavy-weight - having custom code to line up the multiline strings nicely would
be heavy-weight. Really, the bullets should be icons... Maybe one day they will
be checkboxes.

https://codereview.chromium.org/661353002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:311:
GetRetainedDeviceCount());
On 2014/10/20 06:54:09, tapted (OOO until Nov 1st) wrote:
> It would be nice to share some of this boilerplate with the code that inspired
> the version here.  I reviewed that code too (:p), but there wasn't really much
> to be gained then. Maybe it just has to look like this.

I don't mind it looking like this.

[benwells, 2014-10-23 00:38:33]

https://codereview.chromium.org/661353002/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/661353002/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2296: +      <message
name="IDS_APPLICATION_INFO_RETAINED_DEVICES_DEFAULT" desc="A line of explanatory
text that precedes the list of devices the app has permanent access to, showing
the number of devices. This is shown when an app has persistent access to
devices. This is necessary for every language. This is the default for all the
numbers NOT covered by special cases (singular, dual/two, few, many) some
languages need. For CJK, Vietnamese, Turkish and Kannada, this is the only
string necessary. For languages with singular-plural distinction, this is the
generic plural. For Lithuanian, NUMBER_OF_DEVICES is 11 .. 19.">
nit: should the desc mention USB, since the string does?

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
(right):

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:44:
class RevokeButton : public views::LabelButton, public views::ButtonListener {
Would ImageButton work? Seems like a more appropriate parent.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:64:
if (sender == this) {
Just DCHECK(sender == this) instead of the if / else / NOTREACHED

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:101:
column_set->AddPaddingColumn(0, kSpacingBetweenTextAndRevokeButton);
Indenting >> 2

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:169:
layout_->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
Can you add the padding row in AddPermissionnBullets instead? Then you won't
need the if.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:172:
views::Label* bullet_label = new views::Label(l10n_util::GetStringFUTF16(
Can we just hard code a unicode bullet instead of this craziness? Is there any
point in the bullet being in the resource file? Like, is it translated?

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:185:
has_permissions_ = true;
We can also put this line in AddPermissionBullets instead.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:243:
views::View* vertical_stack = CreateVerticalStack();
Why is the vertical stack needed? Aren't we already in a vertical box layout?

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:250:
delete permissions_list;
We should avoid creating the permissions list if there are no permissions.

E.g. at the beginning of the function:
if (GetActivePermissionsCount() == 0 && GetDevicePermissionCount() == 0 ...) {
  .. create no_permissoins_text
  vertical_stack.AddChildView(no_permissoins_text);
  return;
}

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:264:
int AppInfoPermissionsPanel::GetActivePermissionMessagesCount() const {
This isn't currently used, but with the above suggestion it would be.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.h
(right):

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.h:25:
class LabelButton;
Nit: LabelButton forward decl isn't needed any more.,

[sashab, 2014-10-23 01:57:03]

sashab@chromium.org changed reviewers:
+ avi@chromium.org

[sashab, 2014-10-23 01:57:04]

avi@chromium.org: Please review changes in ui/

https://codereview.chromium.org/661353002/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/661353002/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2296: +      <message
name="IDS_APPLICATION_INFO_RETAINED_DEVICES_DEFAULT" desc="A line of explanatory
text that precedes the list of devices the app has permanent access to, showing
the number of devices. This is shown when an app has persistent access to
devices. This is necessary for every language. This is the default for all the
numbers NOT covered by special cases (singular, dual/two, few, many) some
languages need. For CJK, Vietnamese, Turkish and Kannada, this is the only
string necessary. For languages with singular-plural distinction, this is the
generic plural. For Lithuanian, NUMBER_OF_DEVICES is 11 .. 19.">
On 2014/10/23 00:38:33, benwells wrote:
> nit: should the desc mention USB, since the string does?

Done for all versions of this string.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
(right):

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:44:
class RevokeButton : public views::LabelButton, public views::ButtonListener {
On 2014/10/23 00:38:33, benwells wrote:
> Would ImageButton work? Seems like a more appropriate parent.

Yup! Done.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:64:
if (sender == this) {
On 2014/10/23 00:38:33, benwells wrote:
> Just DCHECK(sender == this) instead of the if / else / NOTREACHED

Done.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:101:
column_set->AddPaddingColumn(0, kSpacingBetweenTextAndRevokeButton);
On 2014/10/23 00:38:33, benwells wrote:
> Indenting >> 2

That's weird! :S Fixed.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:169:
layout_->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
On 2014/10/23 00:38:33, benwells wrote:
> Can you add the padding row in AddPermissionnBullets instead? Then you won't
> need the if.

You'll still need the if, since the first call to AddSinglePermissionBullet()
might need a padding row before it. I think it actually makes more sense here...
Moving it to AddPermissionBullets() would just duplicate it twice (once before
each call to AddSinglePermissionBullet()).

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:172:
views::Label* bullet_label = new views::Label(l10n_util::GetStringFUTF16(
On 2014/10/23 00:38:33, benwells wrote:
> Can we just hard code a unicode bullet instead of this craziness? Is there any
> point in the bullet being in the resource file? Like, is it translated?

I copied this from BulletedView::BulletedView() in
extension_uninstall_dialog_view.cc. Doing a search for '<ph name="PERMISSION"/>'
shows that it seems to be translated to just a single bullet in every language.

Searching for 'x2022' in the code shows that quite few places actually do this
(just hardcode the bullet), e.g. extension_message_bubble_view.cc. So just
copied this from there.

EDIT: Can't figure out how to cast from a char16 to a string16...

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:185:
has_permissions_ = true;
On 2014/10/23 00:38:33, benwells wrote:
> We can also put this line in AddPermissionBullets instead.

The padding needs to go between every single bullet, not just between sets of
bullets. So I think this needs to stay here.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:243:
views::View* vertical_stack = CreateVerticalStack();
On 2014/10/23 00:38:33, benwells wrote:
> Why is the vertical stack needed? Aren't we already in a vertical box layout?

Good point. Done.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:250:
delete permissions_list;
On 2014/10/23 00:38:33, benwells wrote:
> We should avoid creating the permissions list if there are no permissions.
> 
> E.g. at the beginning of the function:
> if (GetActivePermissionsCount() == 0 && GetDevicePermissionCount() == 0 ...) {
>   .. create no_permissoins_text
>   vertical_stack.AddChildView(no_permissoins_text);
>   return;
> }

Ugh, I was hoping to avoid this, but I guess you're right. Done.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:264:
int AppInfoPermissionsPanel::GetActivePermissionMessagesCount() const {
On 2014/10/23 00:38:33, benwells wrote:
> This isn't currently used, but with the above suggestion it would be.

Acknowledged. Changed to 'HasActivePermissionMessages' since the number is not
needed.

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.h
(right):

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.h:25:
class LabelButton;
On 2014/10/23 00:38:33, benwells wrote:
> Nit: LabelButton forward decl isn't needed any more.,

Done.

[benwells, 2014-10-23 03:34:48]

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
(right):

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:169:
layout_->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
On 2014/10/23 01:57:03, sasha_b wrote:
> On 2014/10/23 00:38:33, benwells wrote:
> > Can you add the padding row in AddPermissionnBullets instead? Then you won't
> > need the if.
> 
> You'll still need the if, since the first call to AddSinglePermissionBullet()
> might need a padding row before it. I think it actually makes more sense
here...
> Moving it to AddPermissionBullets() would just duplicate it twice (once before
> each call to AddSinglePermissionBullet()).

Ack

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:185:
has_permissions_ = true;
On 2014/10/23 01:57:04, sasha_b wrote:
> On 2014/10/23 00:38:33, benwells wrote:
> > We can also put this line in AddPermissionBullets instead.
> 
> The padding needs to go between every single bullet, not just between sets of
> bullets. So I think this needs to stay here.

has_permissions_ has nothing to do with padding, but I think you can delete it
now anyway.

https://codereview.chromium.org/661353002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
(right):

https://codereview.chromium.org/661353002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:170:
new views::Label(bullet_point + base::string16());
How about using this:
    const base::char16 bullet_point[] = {0x2022, 0};
    views::Label* bullet_label = new views::Label(base::string16(bullet_point));

[Avi (use Gerrit), 2014-10-23 04:26:25]

ui/ lgtm

[sashab, 2014-10-23 06:25:41]

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
(right):

https://codereview.chromium.org/661353002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc:185:
has_permissions_ = true;
On 2014/10/23 03:34:47, benwells wrote:
> On 2014/10/23 01:57:04, sasha_b wrote:
> > On 2014/10/23 00:38:33, benwells wrote:
> > > We can also put this line in AddPermissionBullets instead.
> > 
> > The padding needs to go between every single bullet, not just between sets
of
> > bullets. So I think this needs to stay here.
> 
> has_permissions_ has nothing to do with padding, but I think you can delete it
> now anyway.

Oops sorry. Done.

[benwells, 2014-10-23 23:18:35]

lgtm

[sashab, 2014-10-23 23:21:05]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-23 23:26:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661353002/120001

[commit-bot: I haz the power, 2014-10-24 01:42:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-24 01:43:34]

Patchset 7 (id:??) landed as
https://crrev.com/2eae4366fbe574eda8a4c9027716b27ade352d45
Cr-Commit-Position: refs/heads/master@{#301021}