Refactor of BrowserDistribution.

chrome/installer/util/browser_distribution.h

 // 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.
 //
 // This file declares a class that contains various method related to branding.
 
 #ifndef CHROME_INSTALLER_UTIL_BROWSER_DISTRIBUTION_H_
 #define CHROME_INSTALLER_UTIL_BROWSER_DISTRIBUTION_H_
 
 #include <string>
@@ skipping 11 matching lines @@
 class BrowserDistribution {
  public:
   enum Type {
     CHROME_BROWSER,
     CHROME_FRAME,
     CHROME_BINARIES,
     CHROME_APP_HOST,
     NUM_TYPES
   };
 
+  enum ShortcutType {
+    SHORTCUT_CHROME,
+    SHORTCUT_ALTERNATE_CHROME,

[grt (UTC plus 2), 2013/07/05 17:27:37]

I prefer SHORTCUT_CHROME_ALTERNATE for this since it's the alternate shortcut to
Chrome rather than the shortcut to an alternate Chrome. I'm okay with this
suggesting being vetoed if the current name makes more sense to everyone else.

[gab, 2013/07/08 12:59:02]

Suggestion sgtm, please use SHORTCUT_CHROME_ALTERNATE.

[huangs, 2013/07/08 19:43:57]

(Optional) please consider changing SHORTCUT_CHROME to SHORTCUT_CHROME_DEFAULT
to facilitate code search; otherwise searching for SHORTCUT_CHROME would return
matches for both.

[gab, 2013/07/08 20:20:32]

This is easily mitigated by a simple regex though "SHORTCUT_CHROME\b".

I'd keep the shorter name as it is used most and the callsites already result in
long wrap calls.

[huangs, 2013/07/09 15:02:53]

Sounds good.

[calamity, 2013/07/16 04:05:17]

Done.

+    SHORTCUT_APP_LAUNCHER
+  };
+
+  enum Subfolder {
+    SUBFOLDER_CHROME,
+    // TODO(calamity): add SUBFOLDER_APPS when refactoring chrome app dir code.
+  };
+
   virtual ~BrowserDistribution() {}
 
   static BrowserDistribution* GetDistribution();
 
   static BrowserDistribution* GetSpecificDistribution(Type type);
 
   Type GetType() const { return type_; }
 
   virtual void DoPostUninstallOperations(const Version& version,
                                          const base::FilePath& local_data_path,
                                          const string16& distribution_data);
 
   // Returns the GUID to be used when registering for Active Setup.
   virtual string16 GetActiveSetupGuid();
 
   virtual string16 GetAppGuid();
 
   // Returns the unsuffixed application name of this program.
   // This is the base of the name registered with Default Programs on Windows.
   // IMPORTANT: This should only be called by the installer which needs to make
   // decisions on the suffixing of the upcoming install, not by external callers
   // at run-time.
   virtual string16 GetBaseAppName();
 
-  // Returns the localized name of the program.
-  virtual string16 GetAppShortCutName();
+  // Returns the display name of this distribution.

[gab, 2013/07/05 14:23:37]

s/display name/localized display name

to be explicit about the fact that this is localized and shouldn't be used for
static registry entry names, etc.
(I know some implementations are not localized, but the point is for callers to
be aware to it should be treated as a localized string).

[calamity, 2013/07/16 04:05:17]

Done.

+  virtual string16 GetDisplayName();
 
-  virtual string16 GetAlternateApplicationName();
+  // Returns the name of the shortcut identified by |shortcut_type| for this

[gab, 2013/07/05 14:23:37]

s/name/localized name

(same idea as above)

[calamity, 2013/07/16 04:05:17]

Done.

+  // distribution.
+  virtual string16 GetShortcutName(ShortcutType shortcut_type);
+
+  // Returns the index of the icon for the product identified by
+  // |shortcut_type|, inside the file specified by GetIconFilename().
+  virtual int GetIconIndex(ShortcutType shortcut_type);
+
+  // Returns the executable filename (not path) that contains the product icon.
+  virtual string16 GetIconFilename();
+
+  // Returns the name of the subfolder in the Start Menu identified by

[gab, 2013/07/05 14:23:37]

s/name/localized name

(same idea as above)

[calamity, 2013/07/16 04:05:17]

Done.

+  // |subfolder_type| that this distribution should create shortcuts in.

[grt (UTC plus 2), 2013/07/05 17:27:37]

Please add to this comment that this returns GetShortcutName(SHORTCUT_CHROME)
for SUBFOLDER_CHROME. The subclasses of BD assume this, so it must be part of
the contract for this method.

[gab, 2013/07/08 12:59:02]

I don't see why you say that subclasses of BD assume this? None of them even
call into this method? Or do you mean they rely on this by not overriding it?

[grt (UTC plus 2), 2013/07/08 15:15:50]

Exactly. GoogleChromeDistribution and GoogleChromeSxSDistribution implicitly
assume that they don't need to provide their own GetStartMenuShortcutSubfolder
methods since their GetShortcutName overrides will be used to get the
SUBFOLDER_CHROME shortcut folder.

[calamity, 2013/07/16 04:05:17]

Done.

+  virtual string16 GetStartMenuShortcutSubfolder(Subfolder subfolder_type);
 
   // Returns the unsuffixed appid of this program.
   // The AppUserModelId is a property of Windows programs.
   // IMPORTANT: This should only be called by ShellUtil::GetAppId as the appid
   // should be suffixed in all scenarios.
   virtual string16 GetBaseAppId();
 
   virtual string16 GetInstallSubDir();
 
   virtual string16 GetPublisherName();
@@ skipping 19 matching lines @@
   virtual string16 GetUninstallLinkName();
 
   virtual string16 GetUninstallRegPath();
 
   virtual string16 GetVersionKey();
 
   virtual bool CanSetAsDefault();
 
   virtual bool CanCreateDesktopShortcuts();
 
-  // Returns the executable filename (not path) that contains the product icon.
-  virtual string16 GetIconFilename();
-
-  // Returns the index of the icon for the product, inside the file specified by
-  // GetIconFilename().
-  virtual int GetIconIndex();
-
   virtual bool GetChromeChannel(string16* channel);
 
   // Returns true if this distribution includes a DelegateExecute verb handler,
   // and provides the CommandExecuteImpl class UUID if |handler_class_uuid| is
   // non-NULL.
   virtual bool GetCommandExecuteImplClsid(string16* handler_class_uuid);
 
   // Returns true if this distribution uses app_host.exe to run platform apps.
   virtual bool AppHostIsSupported();
 
   virtual void UpdateInstallStatus(bool system_install,
       installer::ArchiveType archive_type,
       installer::InstallStatus install_status);
 
   // Returns true if this distribution should set the Omaha experiment_labels
   // registry value.
   virtual bool ShouldSetExperimentLabels();
 
   virtual bool HasUserExperiments();
 
  protected:
   explicit BrowserDistribution(Type type);
 
   template<class DistributionClass>
   static BrowserDistribution* GetOrCreateBrowserDistribution(
       BrowserDistribution** dist);
 
+  static const char* kDefaultAlternateChromeShortcutName;

[grt (UTC plus 2), 2013/07/05 17:27:37]

This appears to be unused. Remove it?

[calamity, 2013/07/16 04:05:17]

Done.

+
   const Type type_;
 
  private:
   BrowserDistribution();
 
   DISALLOW_COPY_AND_ASSIGN(BrowserDistribution);
 };
 
 #endif  // CHROME_INSTALLER_UTIL_BROWSER_DISTRIBUTION_H_