Making the extension permissions dialog scrollable, when needed (adding expandable sections for thi…

Making the extension permissions dialog scrollable, when needed (adding expandable sections for things needing more details).

BUG=70422
TBR=erg@chromium.org, pkasting@chromium.org, yoz@chromium.org

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

[Finnur, 2013-07-10 20:58:41]

Yoyo, can you do the overall review? (it looks worse than it is -- a lot of file
changes is 1-3 line changes in each)

Scott, can you review this file:
chrome/browser/ui/views/extensions/extension_install_dialog_views.cc

Sailesh, can I get an OWNERS check on:
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm
chrome/browser/ui/cocoa/extensions/extension_install_view_controller_unittest.mm

chrome/app/theme/theme_resources.grd

Likewise, Elliot on:
chrome/browser/ui/gtk/extensions/extension_install_dialog_gtk.cc

Thanks in advance!

[Elliot Glaysher, 2013-07-10 22:34:34]

gtk lgtm stamp

[Yoyo Zhou, 2013-07-10 23:18:57]

https://codereview.chromium.org/18854021/diff/33016/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/18854021/diff/33016/chrome/app/generated_reso...
chrome/app/generated_resources.grd:5057: +      <message
name="IDS_EXTENSION_PROMPT_RETAINED_FILES_WITH_COUNT" desc="A line of
explanatory text that precedes the list of files the app has permanent access
to. This is shown when an app has persistent access to files.">
This is the same desc as the one before it.

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

https://codereview.chromium.org/18854021/diff/33016/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:508: if
(prompt.GetPermission(i, true) != prompt.GetPermission(i, false)) {
These will differ whenever there is any host list. Should this check that the
details is non-empty instead?

https://codereview.chromium.org/18854021/diff/33016/chrome/common/extensions/...
File chrome/common/extensions/permissions/permission_message.h (right):

https://codereview.chromium.org/18854021/diff/33016/chrome/common/extensions/...
chrome/common/extensions/permissions/permission_message.h:98: string16 message_;
An alternative might be to have another string16 here for details. This would
help avoid having to pass the extra "is details" parameter everywhere.

[Finnur, 2013-07-11 14:15:18]

Excellent points, all addressed.

I've also removed sail from the reviewers list since the Mac changes have been
eliminated.

[Finnur, 2013-07-11 17:09:58]

Switching from Scott to Peter for this file, since Scott is OOO:
chrome/browser/ui/views/extensions/extension_install_dialog_views.cc

[Peter Kasting, 2013-07-11 17:28:34]

LGTM

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

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:123: //
views::View:
Nit: You don't directly subclass views::View, so insert these in the proper
location in the DialogDelegateView list.  (Same applies to WidgetDelegate
overrides).

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:164: const
std::vector<string16>& details,
Nit: You may want a typedef for vector<string16> since it's used in so many
places.

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:167: virtual
~ExpandableContainerView() {}
Nit: Virtual functions generally shouldn't be inlined; I suggest defining this
below

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:179: //
Implementation of ui::AnimationDelegate:
Nit: Remove "Implementation of" for consistency

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:506: // If
we have more details to provide, so show it un-expanded.
Nit: Grammar

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:509:
std::vector<string16> details;
Nit: Indenting is wrong

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:514: this,
Nit: I suggest collapsing onto fewer lines, I find these strung-out definitions
harder to scan.  (several places)

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:716: int
horizontal_space, bool show_bullets)
Nit: This is legal but I think one arg per line is better.  (several places)

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:848:
DCHECK_EQ(animation, &slide_animation_);
Nit: (expected, actual)

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:849: 
Nit: I'd remove this blank line

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:855:
ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
Nit: Avoid non-const refs.  (I know this is a common pattern with
ResourceBundle, but I'm trying to get people to change it.)

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:865:
expanded_ ? l10n_util::GetStringUTF16(IDS_EXTENSIONS_HIDE_DETAILS) :
Nit: Indentation of next line is unusual; prefer this:

  more_details_->SetText(expanded_ ?
      l10n_util::GetStringUTF16(IDS_EXTENSIONS_HIDE_DETAILS) :
      l10n_util::GetStringUTF16(IDS_EXTENSIONS_SHOW_DETAILS);

[Yoyo Zhou, 2013-07-11 17:39:48]

LGTM

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

https://codereview.chromium.org/18854021/diff/86001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:506: // If
we have more details to provide, so show it un-expanded.
nit: no 'so'

https://codereview.chromium.org/18854021/diff/86001/chrome/common/extensions/...
File chrome/common/extensions/permissions/permission_message.h (right):

https://codereview.chromium.org/18854021/diff/86001/chrome/common/extensions/...
chrome/common/extensions/permissions/permission_message.h:78:
PermissionMessage(ID id, const string16& message, const string16& details);
nit: you could add a form of the constructor with no details argument.

[Finnur, 2013-07-11 18:17:36]

All done. Thanks to both of you for quick replies.

[commit-bot: I haz the power, 2013-07-11 18:20:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/finnur@chromium.org/18854021/94002

[commit-bot: I haz the power, 2013-07-11 18:58:37]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Finnur, 2013-07-11 20:16:45]

Actually, all *three* of you is what I meant to say!

[Finnur, 2013-07-11 20:39:41]

Re-adding sail as TBR for chrome/app/theme/theme_resources.grd changes.

[Finnur, 2013-07-11 21:15:39]

Committed patchset #12 manually as r211218 (presubmit successful).