Replace Webstore link with app info dialog link in chrome://apps

Replace Webstore link with app info dialog link in chrome://apps

This brings the context menu options closer to what is in the App
Launcher, and hides the webstore link in cases where it shouldn't be
shown to the user. This change is ignored on Mac.

BUG=449095
Committed: https://crrev.com/0094cacbe5113923a2442b7a4aa34779850d14ef
Cr-Commit-Position: refs/heads/master@{#318163}

[tsergeant, 2015-02-05 05:33:26]

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

[tsergeant, 2015-02-05 05:33:27]

Sashab, can you take a look at this when you get a chance? (no rush) There are a
couple of things I'm not really sure about.

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

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:32:
gfx::Size GetApproximateAppListSize() {
Is there a better place to put this, or a more appropriate way to do it?

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:162: bool
ShouldDisplayAppInfoDialog() {
Is there a good way to avoid adding new logic to figure whether the app info
dialog is available? This is mostly a duplicate of
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[sashab, 2015-02-06 16:23:00]

Nice first patch. Some general feedback, although overall its looking awesome.

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

https://codereview.chromium.org/899173002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:11648: +          App Info
Will this ever be different than IDS_APP_CONTEXT_MENU_SHOW_INFO? I don't see any
reason not to share the same string (less work for translators).
If you're worried about the ID, maybe make a IDS_APPLICATION_INFO_CONTEXT_MENU
which is used wherever the dialog is launched from a context menu :)

https://codereview.chromium.org/899173002/diff/1/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:72: if
(loadTimeData.getBoolean('enableAppInfoDialog')) {
I'd rename this to 'canShowAppInfoDialog' ('enable' suggests its a flag).

https://codereview.chromium.org/899173002/diff/1/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:77: this.details_ =
this.appendMenuItem_('appdetails');
If they're actually replacing each other, why not just re-use the details_
field? You could then avoid bugs caused from things like 'member variables in
javascript are not null by default' (although I think they are, so you're OK
there).

https://codereview.chromium.org/899173002/diff/1/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:157: this.details_.disabled =
!app.appData.detailsUrl;
Yeah... I don't like this. I think maybe create the link anyway but just disable
it.

So the change above would be like:
this.details = ...
if (enable dialog) {
  this.dialog = ...
  this.details.disabled = true
}

Then here you could say:
this.details.disabled = this.details.disabled || !app has a url;

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

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:32:
gfx::Size GetApproximateAppListSize() {
On 2015/02/05 05:33:27, tsergeant wrote:
> Is there a better place to put this, or a more appropriate way to do it?

We agreed with UI on a hardcoded size, so I think this is the best way to do it
:) It actually doesn't have a whole lot to do with the app list (it's just a
nice size for a dialog).
I like putting it in a function though, in case we ever want to make it dynamic.
Maybe you could move it to the same place you move CanShowAppInfoDialog to, and
call it GetAppInfoNativeDialogSize() or similar :)

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:8: #include
<string>
Not needed :)

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:626:
extension_service_->GetExtensionById(extension_id, true);
DCHECK(extension) here

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:628: // Display the dialog
at a size similar to the app list.
Don't need this comment - code explains it, and with the renamed Get...Size() it
won't be true anyway :)

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:162: bool
ShouldDisplayAppInfoDialog() {
On 2015/02/05 05:33:27, tsergeant wrote:
> Is there a good way to avoid adding new logic to figure whether the app info
> dialog is available? This is mostly a duplicate of
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Hmm... This would be perfect as a static method to AppInfoDialog, but it has a
views dependency so can't be imported on Mac :)
Actually, maybe it can... Check with Trent and maybe put it there,
AppInfoDialog::CanDisplayInNativeDialog() and
AppInfoDialog::CanDisplayInAppList() (since we will probably enable the two
separately) and update the dependency you linked to as well.

Otherwise, maybe add them as just plain methods to app_info_dialog.h.

[tsergeant, 2015-02-09 03:10:49]

Addressed all comments apart from the ShouldShowAppInfoDialog one, since I'm not
sure where is best to move it.

https://codereview.chromium.org/899173002/diff/1/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:72: if
(loadTimeData.getBoolean('enableAppInfoDialog')) {
On 2015/02/06 16:23:00, sasha_b wrote:
> I'd rename this to 'canShowAppInfoDialog' ('enable' suggests its a flag).

Done.

https://codereview.chromium.org/899173002/diff/1/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:157: this.details_.disabled =
!app.appData.detailsUrl;
On 2015/02/06 16:23:00, sasha_b wrote:
> Yeah... I don't like this. I think maybe create the link anyway but just
disable
> it.
> 
> So the change above would be like:
> this.details = ...
> if (enable dialog) {
>   this.dialog = ...
>   this.details.disabled = true
> }
> 
> Then here you could say:
> this.details.disabled = this.details.disabled || !app has a url;

Creating the link and then disabling it means that non-Mac platforms will
*always* have a disabled link, which seems like bad UX to me. I'm not sure that
overlapping the two in the same variable will make things any cleaner either.

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

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:32:
gfx::Size GetApproximateAppListSize() {
On 2015/02/06 16:23:00, sasha_b wrote:
> On 2015/02/05 05:33:27, tsergeant wrote:
> > Is there a better place to put this, or a more appropriate way to do it?
> 
> We agreed with UI on a hardcoded size, so I think this is the best way to do
it
> :) It actually doesn't have a whole lot to do with the app list (it's just a
> nice size for a dialog).
> I like putting it in a function though, in case we ever want to make it
dynamic.
> Maybe you could move it to the same place you move CanShowAppInfoDialog to,
and
> call it GetAppInfoNativeDialogSize() or similar :)

Done.

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:8: #include
<string>
On 2015/02/06 16:23:00, sasha_b wrote:
> Not needed :)

Done.

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:626:
extension_service_->GetExtensionById(extension_id, true);
On 2015/02/06 16:23:00, sasha_b wrote:
> DCHECK(extension) here

Done.

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:628: // Display the dialog
at a size similar to the app list.
On 2015/02/06 16:23:00, sasha_b wrote:
> Don't need this comment - code explains it, and with the renamed Get...Size()
it
> won't be true anyway :)

Done.

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:162: bool
ShouldDisplayAppInfoDialog() {
On 2015/02/06 16:23:00, sasha_b wrote:
> On 2015/02/05 05:33:27, tsergeant wrote:
> > Is there a good way to avoid adding new logic to figure whether the app info
> > dialog is available? This is mostly a duplicate of
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> Hmm... This would be perfect as a static method to AppInfoDialog, but it has a
> views dependency so can't be imported on Mac :)
> Actually, maybe it can... Check with Trent and maybe put it there,
> AppInfoDialog::CanDisplayInNativeDialog() and
> AppInfoDialog::CanDisplayInAppList() (since we will probably enable the two
> separately) and update the dependency you linked to as well.
> 
> Otherwise, maybe add them as just plain methods to app_info_dialog.h.

I already tried something similar. Adding a method to AppInfoDialog in
`chrome/browser/ui/views/app_info_dialog_views.h` doesn't work, because
`chrome/browser/ui/DEPS` prevents importing from `chrome/browser/ui/views` in
this file.

Adding it to `app_info_dialog` should work. There's also
`ui/app_list/app_list_switches`, which contains `IsAppInfoEnabled`, which is
used in the applist itself.

There's also a slight difficulty since the `extension_settings_handler` version
has a flag which doesn't necessarily apply elsewhere.

[sashab, 2015-02-09 11:47:08]

Hmm.. Tricky. Nice work though. See comments. :)

https://codereview.chromium.org/899173002/diff/1/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:157: this.details_.disabled =
!app.appData.detailsUrl;
On 2015/02/09 03:10:49, tsergeant wrote:
> On 2015/02/06 16:23:00, sasha_b wrote:
> > Yeah... I don't like this. I think maybe create the link anyway but just
> disable
> > it.
> > 
> > So the change above would be like:
> > this.details = ...
> > if (enable dialog) {
> >   this.dialog = ...
> >   this.details.disabled = true
> > }
> > 
> > Then here you could say:
> > this.details.disabled = this.details.disabled || !app has a url;
> 
> Creating the link and then disabling it means that non-Mac platforms will
> *always* have a disabled link, which seems like bad UX to me. I'm not sure
that
> overlapping the two in the same variable will make things any cleaner either.

Ohh I didn't realise - yes having it disabled on mac is awful. Ok, sure, I'm
happy with this.

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/899173002/diff/1/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:162: bool
ShouldDisplayAppInfoDialog() {
On 2015/02/09 03:10:49, tsergeant wrote:
> On 2015/02/06 16:23:00, sasha_b wrote:
> > On 2015/02/05 05:33:27, tsergeant wrote:
> > > Is there a good way to avoid adding new logic to figure whether the app
info
> > > dialog is available? This is mostly a duplicate of
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > 
> > Hmm... This would be perfect as a static method to AppInfoDialog, but it has
a
> > views dependency so can't be imported on Mac :)
> > Actually, maybe it can... Check with Trent and maybe put it there,
> > AppInfoDialog::CanDisplayInNativeDialog() and
> > AppInfoDialog::CanDisplayInAppList() (since we will probably enable the two
> > separately) and update the dependency you linked to as well.
> > 
> > Otherwise, maybe add them as just plain methods to app_info_dialog.h.
> 
> I already tried something similar. Adding a method to AppInfoDialog in
> `chrome/browser/ui/views/app_info_dialog_views.h` doesn't work, because
> `chrome/browser/ui/DEPS` prevents importing from `chrome/browser/ui/views` in
> this file.
> 
> Adding it to `app_info_dialog` should work. There's also
> `ui/app_list/app_list_switches`, which contains `IsAppInfoEnabled`, which is
> used in the applist itself.
> 
> There's also a slight difficulty since the `extension_settings_handler`
version
> has a flag which doesn't necessarily apply elsewhere.

Don't really want to put it in switches, since that's more for command-line
flags. I can see IsAppInfoEnabled() but don't really like it (since 'enabled'
doesn't really define its location).

How's this... move IsAppInfoEnabled into app_info_dialog as suggested below, and
make it take the location parameter. Now the logic is:

// Returns whether the app info dialog can be launched on this platform from the
given location.
bool CanLaunchAppInfoDialogFrom(location) {
if location == app list
   <old IsAppInfoEnabled logic here>
   return

if mac
  return false
return true
}

Thoughts?

https://codereview.chromium.org/899173002/diff/40001/chrome/browser/ui/apps/a...
File chrome/browser/ui/apps/app_info_dialog.h (right):

https://codereview.chromium.org/899173002/diff/40001/chrome/browser/ui/apps/a...
chrome/browser/ui/apps/app_info_dialog.h:25: FROM_EXTENSIONS_PAGE,  // Launched
from the chrome://extensions page.
Almost forgot! We'll need a new Launch Source for launching from the apps page,
and the UMA to record it :) Also remember to update histograms.xml.

https://codereview.chromium.org/899173002/diff/40001/chrome/browser/ui/apps/a...
chrome/browser/ui/apps/app_info_dialog.h:29: // Size of native window container
for app info dialog
Nit: . at end of comment. Also sentence tense: "Returns the size of..."

https://codereview.chromium.org/899173002/diff/40001/chrome/browser/ui/apps/a...
chrome/browser/ui/apps/app_info_dialog.h:31: 
I'd be happy to put ShouldShowAppInfoDialog here. If you wanted to be really
fancy, you could have ShouldShowAppInfoDialog(AppInfoLaunchSource), which varies
depending on the source.

Although, I'm pretty sure this function will just be if (mac) return false, else
return true? If so its independent of the launch source - we can come back and
add that in once it varies depending on where we're launching from. So we could
even do something like...
// Whether or not the app info dialog can be shown on this platform.
bool CanShowAppInfoDialogOnPlatform();

https://codereview.chromium.org/899173002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

https://codereview.chromium.org/899173002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:90: // Handles the
"showAppInfo" message with |args| containing [extension_id]
Nit: . at end of comment

[tsergeant, 2015-02-12 06:34:49]

Updated to address comments and consolidate down all of the CanShowAppInfoDialog
logic into one method. (Pending http://crrev.com/916333002 landing).

cc tapted: Is it safe to use IsMacViewsAppListListEnabled() to control launching
the app info dialog in a native window from other locations?

[tapted, 2015-02-12 22:45:28]

On 2015/02/12 06:34:49, tsergeant wrote:
> cc tapted: Is it safe to use IsMacViewsAppListListEnabled() to control
launching
> the app info dialog in a native window from other locations?

Yep! Good choice - I was going to have to do that at some point.

(and you get a review from me too - ha!)

[tapted, 2015-02-12 22:45:38]

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

[tapted, 2015-02-12 22:45:40]

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/resources...
chrome/browser/resources/ntp4/apps_page.js:72: this.onShowAppInfo_.bind(this));
nit: indent 2 more spaces

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/resources...
chrome/browser/resources/ntp4/apps_page.js:76: this.onShowDetails_.bind(this));
nit: indent

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_delegate.cc (right):

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:28: #include
"ui/app_list/app_list_switches.h"
can this be removed now?

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

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:42: const
int kAppInfoDialogWidth = 380;
nit: function-local constants are meant to be hacker_style rather than
kFooStyle. Although I reckon this is specific enough to just do `return
gfx::Size(380, 490);`

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:627: DCHECK(extension);
If this is an asynchronous call (it probably is), there's always the possibility
of the extension being uninstalled via sync between when the user clicked a menu
item, and a callback being processed, so I'd probably just check for null and
return early like in HandleCreateAppShortcut

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:636:
base::Bind(&base::DoNothing));
Unless there's a reason for (I don't think there is), I believe the preference
is pass a null closure (i.e. `base::Closure()`) rather than a non-null closure
that does nothing.

https://codereview.chromium.org/899173002/diff/80001/ui/app_list/app_list_swi...
File ui/app_list/app_list_switches.cc (left):

https://codereview.chromium.org/899173002/diff/80001/ui/app_list/app_list_swi...
ui/app_list/app_list_switches.cc:70: bool IsAppInfoEnabled() {
remove declaration from header?

[tsergeant, 2015-02-12 23:56:27]

Thanks, Trent! Updated with changes.

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/899173002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:636:
base::Bind(&base::DoNothing));
On 2015/02/12 22:45:39, tapted wrote:
> Unless there's a reason for (I don't think there is), I believe the preference
> is pass a null closure (i.e. `base::Closure()`) rather than a non-null closure
> that does nothing.

Ah, cool. This way of doing callbacks is new to me.

[tapted, 2015-02-13 08:11:59]

https://codereview.chromium.org/899173002/diff/100001/chrome/browser/ui/apps/...
File chrome/browser/ui/apps/app_info_dialog.h (right):

https://codereview.chromium.org/899173002/diff/100001/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:31: bool CanShowAppInfoDialog();
ooh. I think you'l need to "implement" these in
chrome/browser/ui/cocoa/apps/app_info_dialog_cocoa.mm as well, otherwise you
will get link errors on Chrome Mac.

[tsergeant, 2015-02-16 02:46:53]

Updated with empty implementation for Mac.

[tapted, 2015-02-18 05:22:10]

you should try out your trybot powers now, if that dependent CL has landed :)

https://codereview.chromium.org/899173002/diff/120001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/app_info_dialog_cocoa.mm (right):

https://codereview.chromium.org/899173002/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/app_info_dialog_cocoa.mm:14: }
return gfx::Size();

You'll also need

#include "ui/gfx/geometry/size.h"

up top, since it's forward declared.

[sashab, 2015-02-18 05:34:35]

Looking great!! Just one more tiny thing...

https://codereview.chromium.org/899173002/diff/120001/chrome/browser/ui/apps/...
File chrome/browser/ui/apps/app_info_dialog.h (right):

https://codereview.chromium.org/899173002/diff/120001/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:31: bool CanShowAppInfoDialog();
Do you feel these should all be static functions of a utility class, maybe
AppInfoDialog? It can have a private constructor/destructor and just a bunch of
these utility functions.

https://codereview.chromium.org/899173002/diff/120001/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:42: const base::Closure&
close_callback);
Alternatively, the top two could be static, but these two could be...

AppInfoDialog(parent, profile, app, close_callback)

ShowInAppList(app_list_bounds)
ShowInNativeDialog(size)

｡◕‿‿◕｡ soo nicee...

[tsergeant, 2015-02-18 05:58:38]

Updated!

https://codereview.chromium.org/899173002/diff/120001/chrome/browser/ui/apps/...
File chrome/browser/ui/apps/app_info_dialog.h (right):

https://codereview.chromium.org/899173002/diff/120001/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:42: const base::Closure&
close_callback);
On 2015/02/18 05:34:34, sasha_b wrote:
> Alternatively, the top two could be static, but these two could be...
> 
> AppInfoDialog(parent, profile, app, close_callback)
> 
> ShowInAppList(app_list_bounds)
> ShowInNativeDialog(size)
> 
> ｡◕‿‿◕｡ soo nicee...

Good idea! I've put in a TODO, since this is orthogonal to the current CL.

[sashab, 2015-02-18 06:02:14]

Awesome! lgtm :)

[tapted, 2015-02-18 06:19:08]

lgtm with a question. You'll still need a webui guru and an histograms owner
though

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:67: const
gfx::Size& size,
Is there a reason to keep this argument?

Then, you might be able to move GetAppInfoNativeDialogSize() into an anonymous
namespace just in this .cc

[tsergeant, 2015-02-18 06:24:46]

On 2015/02/18 06:19:08, tapted wrote:
> lgtm with a question. You'll still need a webui guru and an histograms owner
> though
> 
>
https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
> File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
> (right):
> 
>
https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
> chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:67:
const
> gfx::Size& size,
> Is there a reason to keep this argument?
> 
> Then, you might be able to move GetAppInfoNativeDialogSize() into an anonymous
> namespace just in this .cc

Hmm, good point. This might be something that can changed as part of the
refactor that sasha_b suggested above, since that will already change the
arguments.

[tsergeant, 2015-02-18 07:32:30]

tsergeant@chromium.org changed reviewers:
+ dbeam@chromium.org

[tsergeant, 2015-02-18 07:32:31]

dbeam@chromium.org: Please review changes in webui/ntp.

[tsergeant, 2015-02-25 01:59:23]

ping dbeam!

[Dan Beam, 2015-02-25 02:30:08]

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:34: #if
defined(OS_MACOSX)
when will this file be compiled on mac?

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1230: // be set
on Mac platforms.
^ still true?

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:627: 
nit: remove \n

[Dan Beam, 2015-02-25 02:30:52]

also, do you have screenshots of this UI?

[tsergeant, 2015-02-25 03:18:05]

Thanks for the feedback, I've addressed those two comments.

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:34: #if
defined(OS_MACOSX)
On 2015/02/25 02:30:08, Dan Beam wrote:
> when will this file be compiled on mac?

I believe this file is compiled when TOOLKIT_VIEWS=1 on Mac, which is what
tapted@ is currently working on.

[tsergeant, 2015-02-25 03:19:46]

And I've attached some screenshots to the bug.

[Dan Beam, 2015-02-25 03:41:18]

lgtm

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/899173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:34: #if
defined(OS_MACOSX)
On 2015/02/25 03:18:05, tsergeant wrote:
> On 2015/02/25 02:30:08, Dan Beam wrote:
> > when will this file be compiled on mac?
> 
> I believe this file is compiled when TOOLKIT_VIEWS=1 on Mac, which is what
> tapted@ is currently working on.

it's just crazy enough to [possibly] work!

[tsergeant, 2015-02-25 03:57:56]

tsergeant@chromium.org changed reviewers:
+ isherman@chromium.org

[tsergeant, 2015-02-25 03:57:56]

isherman@chromium.org: Please review changes in
tools/metrics/histograms/histograms.xml

(The only new UMA call is in
chrome/browser/ui/webui/ntp/app_launcher_handler.cc)

[Ilya Sherman, 2015-02-25 19:11:52]

histograms lgtm, with a suggestion:

https://codereview.chromium.org/899173002/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/899173002/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:632:
AppInfoLaunchSource::NUM_LAUNCH_SOURCES);
I'd recommend refactoring the code, so that there's a single function that wraps
this particular histogram, i.e. something like

  void RecordAppLaunch(AppInfoLaunchSource source) {
    UMA_HISTOGRAM_ENUMERATION("Apps.AppInfoDialog.Launches", source,
                              AppInfoLaunchSource::NUM_LAUNCH_SOURCES);
  }

This reduces the likelihood that you might end up with a typo along just one of
the codepaths.

[tsergeant, 2015-02-25 23:46:23]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2015-02-25 23:46:23]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/899173002/#ps160001
(title: "Review comments")

[commit-bot: I haz the power, 2015-02-25 23:47:22]

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

[commit-bot: I haz the power, 2015-02-26 00:52:14]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-26 00:53:03]

Patchset 9 (id:??) landed as
https://crrev.com/0094cacbe5113923a2442b7a4aa34779850d14ef
Cr-Commit-Position: refs/heads/master@{#318163}