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

chrome/browser/resources/ntp4/apps_page.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 cr.define('ntp', function() {
   'use strict';
 
   var APP_LAUNCH = {
     // The histogram buckets (keep in sync with extension_constants.h).
     NTP_APPS_MAXIMIZED: 0,
@@ skipping 49 matching lines @@
 
       var self = this;
       this.forAllLaunchTypes_(function(launchTypeButton, id) {
         launchTypeButton.addEventListener('activate',
             self.onLaunchTypeChanged_.bind(self));
       });
 
       this.launchTypeMenuSeparator_ = cr.ui.MenuItem.createSeparator();
       menu.appendChild(this.launchTypeMenuSeparator_);
       this.options_ = this.appendMenuItem_('appoptions');
-      this.details_ = this.appendMenuItem_('appdetails');
       this.uninstall_ = this.appendMenuItem_('appuninstall');
+
+      if (loadTimeData.getBoolean('enableAppInfoDialog')) {

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

I'd rename this to 'canShowAppInfoDialog' ('enable' suggests its a flag).

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

Done.

+        this.appinfo_ = this.appendMenuItem_('appinfodialog');
+        this.appinfo_.addEventListener('activate',
+                                     this.onShowAppInfo_.bind(this));
+      } else {
+        this.details_ = this.appendMenuItem_('appdetails');

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

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).

+        this.details_.addEventListener('activate',
+                                     this.onShowDetails_.bind(this));
+      }
+
       this.options_.addEventListener('activate',
                                      this.onShowOptions_.bind(this));
-      this.details_.addEventListener('activate',
-                                     this.onShowDetails_.bind(this));
+
       this.uninstall_.addEventListener('activate',
                                        this.onUninstall_.bind(this));
 
       if (!cr.isChromeOS) {
         this.createShortcutSeparator_ =
             menu.appendChild(cr.ui.MenuItem.createSeparator());
         this.createShortcut_ = this.appendMenuItem_('appcreateshortcut');
         this.createShortcut_.addEventListener(
             'activate', this.onCreateShortcut_.bind(this));
       }
@@ skipping 51 matching lines @@
         launchTypeButton.checked = app.appData.launch_type == id;
         // If bookmark apps are enabled, only show the "Open as tab" button.
         launchTypeButton.hidden = app.appData.packagedApp ||
             (loadTimeData.getBoolean('enableNewBookmarkApps') &&
              launchTypeButton != launchTypeRegularTab);
       });
 
       this.launchTypeMenuSeparator_.hidden = app.appData.packagedApp;
 
       this.options_.disabled = !app.appData.optionsUrl || !app.appData.enabled;
-      this.details_.disabled = !app.appData.detailsUrl;
+      if (this.details_)
+        this.details_.disabled = !app.appData.detailsUrl;

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

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;

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

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.

[sashab, 2015/02/09 11:47:07]

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

       this.uninstall_.disabled = !app.appData.mayDisable;
 
       if (cr.isMac) {
         // On Windows and Linux, these should always be visible. On ChromeOS,
         // they are never created. On Mac, shortcuts can only be created for
         // new-style packaged apps, so hide the menu item.
         this.createShortcutSeparator_.hidden = this.createShortcut_.hidden =
             !app.appData.packagedApp;
       }
     },
@@ skipping 32 matching lines @@
       var url = this.app_.appData.detailsUrl;
       url = appendParam(url, 'utm_source', 'chrome-ntp-launcher');
       window.location = url;
     },
     onUninstall_: function(e) {
       chrome.send('uninstallApp', [this.app_.appData.id]);
     },
     onCreateShortcut_: function(e) {
       chrome.send('createAppShortcut', [this.app_.appData.id]);
     },
+    onShowAppInfo_: function(e) {
+      chrome.send('showAppInfo', [this.app_.appData.id]);
+    }
   };
 
   /**
    * Creates a new App object.
    * @param {Object} appData The data object that describes the app.
    * @constructor
    * @extends {HTMLDivElement}
    */
   function App(appData) {
     var el = cr.doc.createElement('div');
@@ skipping 560 matching lines @@
   function launchAppAfterEnable(appId) {
     chrome.send('launchApp', [appId, APP_LAUNCH.NTP_APP_RE_ENABLE]);
   }
 
   return {
     APP_LAUNCH: APP_LAUNCH,
     AppsPage: AppsPage,
     launchAppAfterEnable: launchAppAfterEnable,
   };
 });