Extend App Info dialog to include app shortcut and list of permissions

Extend App Info dialog to include app shortcut and list of permissions

Added a list of permissions to the app info dialog, as well as the app's
shortcut icon and some basic app information (name, version and description).

Context menu option to open the dialog is still behind the flag
--enable-app-list-app-info.

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

[sashab, 2014-02-12 03:18:58]

Screenshot:
http://pasteboard.co/CickSCW.png

[sashab, 2014-02-12 05:17:50]

+mgiuca

[Matt Giuca, 2014-02-12 05:18:55]

You got "error: old chunk mismatch". This is Reitveld being crap. Please try
over and over again until you can see the patch in Reitveld, and then delete all
the bad patch sets. Thanks!

[sashab, 2014-02-12 05:55:21]

Try again now? :S


On Wed, Feb 12, 2014 at 4:18 PM, <mgiuca@chromium.org> wrote:

> You got "error: old chunk mismatch". This is Reitveld being crap. Please
> try
> over and over again until you can see the patch in Reitveld, and then
> delete all
> the bad patch sets. Thanks!
>
> https://codereview.chromium.org/146583010/
>

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

[Matt Giuca, 2014-02-12 06:22:33]

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog_views.cc (right):

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:26: using
extensions::Manifest;
Is there a good reason to have this using here? You only mention it twice so
it's probably not necessary. (Although the style guide does not forbid this.)

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:29: const int kIconSize =
64;
So does it look good (crisp) now using 64 instead of 69?

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:47:
weak_ptr_factory_(this) {
I think we discussed this using weak pointers but in relation to something else
-- now you seem to need it for the icon loading. The problem is that your weak
pointer factory is 'this' and (if my understanding of weak pointers is correct)
this will still be unsafe.

If the AppInfoView is destroyed before the callback returns, then the weak
pointer factory will also be destroyed, and so you will be dereferencing a
dangling pointer when you try to determine whether the weak pointer is alive. I
think you'll have to find another way around. (I'm not really sure what the
standard idioms are for weak pointer use; we can discuss.)

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:78: // TODO(sashab,
mguica): Update this to use ImageFamily::GetBest() instead.
TODOs should preferably have only one person in them (but there are exceptions).
Also, -->mgiuca<--spelling.

I thought we decided that use of ImageFamily is not necessary for getting a
single icon?

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:115: static const int
kPermissionBulletsColumnSetId = 1;
I remember when sammc did his permissions dialog, he went to an inordinate
amount of trouble to make it scrollable. You might want to do the same here --
especially since this is going to be one small part of a bigger dialog with
tabs. You probably want the dialog to have a fixed size, with scrollable
sub-regions for anything that is variable in length.

(Unless I'm missing a part where scroll bars are added, or they are automatic,
or something.)

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:214: // Let's set default
icon bitmap whose size is equal to the default icon's
This is somewhat informal.

I think you just want to say that you're getting the image at the highest scale
factor. You don't need to think about whether it will be scaled down; just
consider ImageSkia to display an image at its logical size, regardless of the
scale factor. The DPI thing is an implementation detail.

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog_views.h (right):

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.h:49: void
OnAppImageLoaded(const gfx::Image& image);
The comment is out of date.

[sashab, 2014-02-17 07:20:56]

Scrollbars added! :D

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog_views.cc (right):

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:26: using
extensions::Manifest;
On 2014/02/12 06:22:34, Matt Giuca wrote:
> Is there a good reason to have this using here? You only mention it twice so
> it's probably not necessary. (Although the style guide does not forbid this.)

Was using it more before an edit :) Removed, thanks.

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:29: const int kIconSize =
64;
On 2014/02/12 06:22:34, Matt Giuca wrote:
> So does it look good (crisp) now using 64 instead of 69?

Yes, comparing 69 to 64 there is a noticable difference. Not as blurry now, not
sure if its still blurry though (but I don't think it is).

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:47:
weak_ptr_factory_(this) {
On 2014/02/12 06:22:34, Matt Giuca wrote:
> I think we discussed this using weak pointers but in relation to something
else
> -- now you seem to need it for the icon loading. The problem is that your weak
> pointer factory is 'this' and (if my understanding of weak pointers is
correct)
> this will still be unsafe.
> 
> If the AppInfoView is destroyed before the callback returns, then the weak
> pointer factory will also be destroyed, and so you will be dereferencing a
> dangling pointer when you try to determine whether the weak pointer is alive.
I
> think you'll have to find another way around. (I'm not really sure what the
> standard idioms are for weak pointer use; we can discuss.)

From my understanding, this is actually correct - the weak pointer factory is
destroyed safely with the object, and the factory's destructor invalidates all
its pointers.

If you look at the example at the top of weak_ptr.h, it uses it like this:

  class Controller {
   public:
    void SpawnWorker() { Worker::StartNew(weak_factory_.GetWeakPtr()); }
    void WorkComplete(const Result& result) { ... }
   private:
    // Member variables should appear before the WeakPtrFactory, to ensure
    // that any WeakPtrs to Controller are invalidated before its members
    // variable's destructors are executed, rendering them invalid.
    WeakPtrFactory<Controller> weak_factory_;
  };

And then Worker uses the WeakPtr as normal. So this should be OK.

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:78: // TODO(sashab,
mguica): Update this to use ImageFamily::GetBest() instead.
On 2014/02/12 06:22:34, Matt Giuca wrote:
> TODOs should preferably have only one person in them (but there are
exceptions).
> Also, -->mgiuca<--spelling.
> 
> I thought we decided that use of ImageFamily is not necessary for getting a
> single icon?

Ahh yes, you're right, this is old :) Removed, thanks.

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:115: static const int
kPermissionBulletsColumnSetId = 1;
On 2014/02/12 06:22:34, Matt Giuca wrote:
> I remember when sammc did his permissions dialog, he went to an inordinate
> amount of trouble to make it scrollable. You might want to do the same here --
> especially since this is going to be one small part of a bigger dialog with
> tabs. You probably want the dialog to have a fixed size, with scrollable
> sub-regions for anything that is variable in length.
> 
> (Unless I'm missing a part where scroll bars are added, or they are automatic,
> or something.)

Okay, the permissions part is now scrollable. There are a few bugs:
  1. If there are no permissions, the area will appear empty (instead of
displaying a nice message) - I think this is OK for now
  2. If there are a large number of permissions, they will display in a scroll
box, instead of prompting the user to view 'More permissions' to enable the
scrollable view (like in the current permissions dialog) - I think this is OK
for now
  3. The 'padding' between bulleted items appears at the end of each item, so it
appears at the end of the list as well (what's a nice way to fix this when using
iterators?)

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:214: // Let's set default
icon bitmap whose size is equal to the default icon's
On 2014/02/12 06:22:34, Matt Giuca wrote:
> This is somewhat informal.
> 
> I think you just want to say that you're getting the image at the highest
scale
> factor. You don't need to think about whether it will be scaled down; just
> consider ImageSkia to display an image at its logical size, regardless of the
> scale factor. The DPI thing is an implementation detail.

Oh, I didn't write this, whoops. I think it's leftovers from where I copied this
from 8-) I'll just remove it.

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog_views.h (right):

https://codereview.chromium.org/146583010/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.h:49: void
OnAppImageLoaded(const gfx::Image& image);
On 2014/02/12 06:22:34, Matt Giuca wrote:
> The comment is out of date.

Done.

[Matt Giuca, 2014-02-17 23:07:02]

https://codereview.chromium.org/146583010/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog_views.cc (right):

https://codereview.chromium.org/146583010/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:40: :
message_center::BoundedScrollView(min_height, max_height) {
I'm not familiar with the pattern of building ScrollViews, but this seems like
too much work to be doing in a constructor.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...

If this is how all the scroll views are done (or there is no other way) then I
guess it's OK, but maybe you could find a way to have a separate Init method?

https://codereview.chromium.org/146583010/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:58: // Create 2 columns: 1
for the bullet, one for the bullet text
s/1/one/

[sashab, 2014-02-18 01:33:47]

https://codereview.chromium.org/146583010/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog_views.cc (right):

https://codereview.chromium.org/146583010/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:40: :
message_center::BoundedScrollView(min_height, max_height) {
On 2014/02/17 23:07:02, Matt Giuca wrote:
> I'm not familiar with the pattern of building ScrollViews, but this seems like
> too much work to be doing in a constructor.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...
> 
> If this is how all the scroll views are done (or there is no other way) then I
> guess it's OK, but maybe you could find a way to have a separate Init method?

Looking at extension_install_dialog_view.cc, and other views, this seems to be a
pretty common practice (putting all the init code in the constructor). I could
make a separate Init method, but I would just be calling it from the
constructor, so there's not much point?

Also, we don't throw Exceptions in chrome, so the part in the style guide about
exceptions doesn't apply. So I think this design practice might be OK here.

https://codereview.chromium.org/146583010/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:58: // Create 2 columns: 1
for the bullet, one for the bullet text
On 2014/02/17 23:07:02, Matt Giuca wrote:
> s/1/one/

Done.

[Matt Giuca, 2014-02-18 03:07:16]

lgtm

https://codereview.chromium.org/146583010/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog_views.cc (right):

https://codereview.chromium.org/146583010/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:40: :
message_center::BoundedScrollView(min_height, max_height) {
On 2014/02/18 01:33:47, sasha_b wrote:
> 
> Looking at extension_install_dialog_view.cc, and other views, this seems to be
a
> pretty common practice (putting all the init code in the constructor). I could
> make a separate Init method, but I would just be calling it from the
> constructor, so there's not much point?

The point of having a separate Init method is not that you call it from the
constructor, but that the constructor does basically nothing (other than getting
the object into a safe "null" state), and whoever constructs the object is then
expected to call Init. It's a reasonably common pattern for avoiding doing too
much work in the constructor and has a few other advantages (such as being able
to call an overload of Init from another overload).

> Also, we don't throw Exceptions in chrome, so the part in the style guide
about
> exceptions doesn't apply.

We don't throw exceptions at Google at all. The part in the style guide about
exceptions is: "There is no easy way for constructors to signal errors, short of
using exceptions (which are forbidden)". Basically this is saying that *if* we
did have exceptions, constructors could signal errors nicely, but *since* we
don't have exceptions, there is no way to communicate an error at all.

Not necessarily saying that this stuff applies to you. As I said, if everybody
else is initialising UI code this way, then I don't have a problem with it. Just
explaining some of that style guide rationale.

[sashab, 2014-02-18 03:43:04]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-02-18 03:43:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sashab@chromium.org/146583010/320001

[commit-bot: I haz the power, 2014-02-18 03:49:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-18 03:49:02]

Try jobs failed on following builders: linux_chromium_rel

[sashab, 2014-02-18 22:11:17]

benwells please review :-)

[benwells, 2014-02-19 00:23:29]

lgtm with a nit.

https://codereview.chromium.org/146583010/diff/320001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog_views.cc (right):

https://codereview.chromium.org/146583010/diff/320001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:48: app ? app->GetType() :
extensions::Manifest::TYPE_UNKNOWN;
I don't think app can be NULL here (you deref it unconditionally just above), so
there is no need for the app ? check.

[sashab, 2014-02-19 03:02:59]

https://codereview.chromium.org/146583010/diff/320001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog_views.cc (right):

https://codereview.chromium.org/146583010/diff/320001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog_views.cc:48: app ? app->GetType() :
extensions::Manifest::TYPE_UNKNOWN;
On 2014/02/19 00:23:29, benwells wrote:
> I don't think app can be NULL here (you deref it unconditionally just above),
so
> there is no need for the app ? check.

Yup, you're right, thanks for pointing this out. I actually cleaned up the code
a little after this change - I noticed a lot of variables were no longer needed.

[sashab, 2014-02-19 04:01:59]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-02-19 04:02:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sashab@chromium.org/146583010/440001

[commit-bot: I haz the power, 2014-02-19 06:00:53]

Change committed as 251954