Issue 15255004: Refactor of BrowserDistribution.

Issue 15255004: Refactor of BrowserDistribution. (Closed)

Created:
7 years, 7 months ago by calamity

Modified:
7 years, 4 months ago

Reviewers:
gab, grt (UTC plus 2), sky, huangs

CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, sail+watch_chromium.org, grt+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Refactor of BrowserDistribution. This is a precursor to https://codereview.chromium.org/13864015/. * Added GetStartMenuShortcutSubfolder, which returns the Start Menu subfolder path for a given subfolder. * GetShortcutName and GetIconIndex now return data specific to a type of shortcut (browser, alternate name of browser, or app launcher). * GetDisplayName is now used rather than GetAppShortcutName where a localized name is needed (e.g., in logs and the registry). BUG=233434, 238895, 161985 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217469

Patch Set 1 #

Total comments: 48

Patch Set 2 : rework #

Total comments: 32

Patch Set 3 : rework #

Total comments: 37

Patch Set 4 : rework #

Total comments: 19

Patch Set 5 : rework #

Total comments: 71

Patch Set 6 : rework #

Total comments: 8

Patch Set 7 : rework #

Patch Set 8 : oops, missed one #

Patch Set 9 : rebase #

Patch Set 10 : fix unit test #

Total comments: 9

Patch Set 11 : fix install.cc and install_unittest.cc #

Patch Set 12 : rework #

Patch Set 13 : rebase #

Patch Set 14 : #

Patch Set 15 : rebase #

Patch Set 16 : rebase #

Patch Set 17 : #

Created: 7 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+344 lines, -215 lines)			Patch
M	chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	5 chunks	+14 lines, -7 lines	0 comments	Download
M	chrome/browser/profiles/profile_shortcut_manager_win.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/shell_integration.h	View	1 2 3 4 5 6 7 8	1 chunk	+0 lines, -5 lines	0 comments	Download
M	chrome/browser/shell_integration_win.cc	View	1 2 3 4 5 6 7 8	2 chunks	+2 lines, -15 lines	0 comments	Download
M	chrome/browser/ui/views/app_list/app_list_controller_win.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+4 lines, -2 lines	0 comments	Download
M	chrome/installer/setup/install.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	4 chunks	+10 lines, -5 lines	0 comments	Download
M	chrome/installer/setup/install_unittest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+11 lines, -4 lines	0 comments	Download
M	chrome/installer/setup/install_worker.cc	View	1 2 3 4 5 6 7 8	9 chunks	+16 lines, -16 lines	0 comments	Download
M	chrome/installer/setup/setup_main.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	9 chunks	+10 lines, -10 lines	0 comments	Download
M	chrome/installer/setup/setup_util.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	3 chunks	+4 lines, -4 lines	0 comments	Download
M	chrome/installer/setup/uninstall.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	3 chunks	+3 lines, -3 lines	0 comments	Download
M	chrome/installer/util/browser_distribution.h	View	1 2 3 4 5	3 chunks	+28 lines, -10 lines	0 comments	Download
M	chrome/installer/util/browser_distribution.cc	View	1 2 3 4 5 6 7	3 chunks	+37 lines, -14 lines	0 comments	Download
M	chrome/installer/util/chrome_app_host_distribution.h	View	1 2 3 4 5	2 chunks	+6 lines, -5 lines	0 comments	Download
M	chrome/installer/util/chrome_app_host_distribution.cc	View	1 2 3 4 5	1 chunk	+6 lines, -8 lines	0 comments	Download
M	chrome/installer/util/chrome_app_host_operations.cc	View	1 2	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/installer/util/chrome_browser_operations.cc	View	1 2 3 4 5 6 7 8	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/installer/util/chrome_frame_distribution.h	View	1 2 3 4	2 chunks	+3 lines, -5 lines	0 comments	Download
M	chrome/installer/util/chrome_frame_distribution.cc	View	1 2 3 4 5	2 chunks	+17 lines, -12 lines	0 comments	Download
M	chrome/installer/util/chromium_binaries_distribution.h	View	1 2 3 4	2 chunks	+5 lines, -5 lines	0 comments	Download
M	chrome/installer/util/chromium_binaries_distribution.cc	View	1 2 3 4	2 chunks	+4 lines, -3 lines	0 comments	Download
M	chrome/installer/util/google_chrome_binaries_distribution.h	View	1 2 3 4	1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/installer/util/google_chrome_binaries_distribution.cc	View	1 2 3 4	1 chunk	+7 lines, -1 line	0 comments	Download
M	chrome/installer/util/google_chrome_distribution.h	View	1 2 3 4	2 chunks	+5 lines, -5 lines	0 comments	Download
M	chrome/installer/util/google_chrome_distribution.cc	View	1 2 3 4 5 6	2 chunks	+23 lines, -8 lines	0 comments	Download
M	chrome/installer/util/google_chrome_distribution_dummy.cc	View	1 2 3 4	1 chunk	+3 lines, -3 lines	0 comments	Download
M	chrome/installer/util/google_chrome_sxs_distribution.h	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/installer/util/google_chrome_sxs_distribution.cc	View	1 2 3 4 5 6	3 chunks	+23 lines, -5 lines	0 comments	Download
M	chrome/installer/util/install_util.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	2 chunks	+4 lines, -4 lines	0 comments	Download
M	chrome/installer/util/installation_validator.cc	View	1 2 3	10 chunks	+13 lines, -13 lines	0 comments	Download
M	chrome/installer/util/installer_state.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	4 chunks	+6 lines, -6 lines	0 comments	Download
M	chrome/installer/util/prebuild/create_string_rc.py	View	1 2 3 4 5	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/installer/util/product.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/installer/util/shell_util.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/installer/util/shell_util.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	9 chunks	+26 lines, -14 lines	0 comments	Download
M	chrome/installer/util/shell_util_unittest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	12 chunks	+39 lines, -15 lines	0 comments	Download

Messages

Total messages: 42 (0 generated)

Expand Messages | Collapse Messages

gab

A quick review. FYI, Monday is a holiday in Canada. Cheers, Gab https://codereview.chromium.org/15255004/diff/1/chrome/browser/shell_integration_win.cc File chrome/browser/shell_integration_win.cc ...

7 years, 7 months ago (2013-05-18 23:57:22 UTC) #2

huangs

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc#newcode226 chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:226: distribution->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME) What happened to .name? https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/setup/install.cc File chrome/installer/setup/install.cc (right): ...

7 years, 7 months ago (2013-05-21 21:35:41 UTC) #3

gab

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc#newcode226 chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:226: distribution->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME) This should be using the subfolder anyways. https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/setup/install.cc ...

7 years, 7 months ago (2013-05-21 21:46:45 UTC) #4

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:226:
distribution->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME)
This should be using the subfolder anyways.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/setup...
File chrome/installer/setup/install.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/setup...
chrome/installer/setup/install.cc:90:
message.append(UTF16ToUTF8(dist->GetDisplayName()));
On 2013/05/21 21:35:41, huangs1 wrote:
> I don't think we should localize our logs. Please see comment in
> browser_distribution.h.

grt@ started this a while back, I forget his argument for doing so.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
File chrome/installer/util/browser_distribution.h (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/browser_distribution.h:74: virtual string16
GetDisplayName();
On 2013/05/21 21:35:41, huangs1 wrote:
> The old code uses GetAppShortCutName() for non-shortcut contexts, which I
think
> should be cleaned up. Currently I can think of the following contexts:
> - Shortcuts: Localized, handled in this CL.
> - Directories: Localized, handled in this CL.

> - Logging: Fixed, for ease of processing?
> - shell_util.h: GetProgIdEntries(): localized?
> - shell_util.h: GetShellIntegrationEntries(): localized?

I think GetDisplayName() is appropriate for these. (registry entries need to be
distribution specific to allow side-by-side installations of Chrome, Chromium,
and Canary -- I don't like that some use the full localized Google Chrome name
though, but we can't change that).

> 
> We'll have to bug grt@ for this.

calamity

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc#newcode226 chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:226: distribution->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME) On 2013/05/21 21:46:45, gab wrote: > This should ...

7 years, 7 months ago (2013-05-24 02:03:11 UTC) #5

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:226:
distribution->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME)
On 2013/05/21 21:46:45, gab wrote:
> This should be using the subfolder anyways.

Why? Isn't this chrome.lnk?

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/shell_i...
File chrome/browser/shell_integration_win.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:360: string16
ShellIntegration::GetChromiumIconLocation() {
On 2013/05/18 23:57:22, gab wrote:
> Looks like this method is no longer used.

Removed.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/setup...
File chrome/installer/setup/install_unittest.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/setup...
chrome/installer/setup/install_unittest.cc:132:
dist_->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name)
On 2013/05/21 21:35:41, huangs1 wrote:
> GetStartMenuShortcutSubfolder()?

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/setup...
chrome/installer/setup/install_unittest.cc:140:
dist_->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name)
On 2013/05/21 21:35:41, huangs1 wrote:
> GetStartMenuShortcutSubfolder()?

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/setup...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/setup...
chrome/installer/setup/install_worker.cc:442:
GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name);
On 2013/05/18 23:57:22, gab wrote:
> This is a case where GetAppShortCutName() was used for the subfolder name.
> 
> Use SUBFOLDER_CHROME now.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
File chrome/installer/util/browser_distribution.h (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/browser_distribution.h:35: SHORTCUT_APP_LAUNCHER
On 2013/05/18 23:57:22, gab wrote:
> I wouldn't add the SHORTCUT_APP_LAUNCHER entry just yet if you don't implement
> it in this CL.
> 
> Simply add a TODO.

Implemented it because chrome_app_host_operations.cc needed the icon index of
the app launcher. Strings are hardcoded with TODOs to change them to a localized
string.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/browser_distribution.h:40: SUBFOLDER_APPS
On 2013/05/18 23:57:22, gab wrote:
> Same for SUBFOLDER_APPS... don't add it yet if not implementing it.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/browser_distribution.h:74: virtual string16
GetDisplayName();
On 2013/05/21 21:46:45, gab wrote:
> On 2013/05/21 21:35:41, huangs1 wrote:
> > The old code uses GetAppShortCutName() for non-shortcut contexts, which I
> think
> > should be cleaned up. Currently I can think of the following contexts:
> > - Shortcuts: Localized, handled in this CL.
> > - Directories: Localized, handled in this CL.
> 
> 
> 
> > - Logging: Fixed, for ease of processing?
> > - shell_util.h: GetProgIdEntries(): localized?
> > - shell_util.h: GetShellIntegrationEntries(): localized?
> 
> I think GetDisplayName() is appropriate for these. (registry entries need to
be
> distribution specific to allow side-by-side installations of Chrome, Chromium,
> and Canary -- I don't like that some use the full localized Google Chrome name
> though, but we can't change that).
> 
> > 
> > We'll have to bug grt@ for this.
> 

Changed the comment. Is there anything else to do here? It doesn't seem to have
any use in profile_shortcut_manager_win..?

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/browser_distribution.h:144: virtual string16
GetAppShortCutName();
On 2013/05/18 23:57:22, gab wrote:
> I think we should get rid of GetAppShortCutName(), GetIconIndex(),
> GetIconFilename(), etc. and simply hardcode values in GetShortcutInfo().
> Otherwise we will still need to add 3 more overridable calls to implement
> SHORTCUT_APP_LAUNCHER.
> 
> I'd prefer overriding GetShortcutInfo() directly in the other distributions.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/browser_distribution.h:156: virtual int
GetAppListIconIndex();
On 2013/05/18 23:57:22, gab wrote:
> Why implement this in this CL if it not implementing SHORTCUT_APPLAUNCHER yet?

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/browser_distribution.h:156: virtual int
GetAppListIconIndex();
On 2013/05/18 23:57:22, gab wrote:
> Why implement this in this CL if it not implementing SHORTCUT_APPLAUNCHER yet?

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/chrome_app_host_operations.cc:112:
dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).icon_index);
On 2013/05/21 21:35:41, huangs1 wrote:
> SHORTCUT_APP_LAUNCHER?

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:222:
GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).icon_index));
On 2013/05/18 23:57:22, gab wrote:
> In general for this sort of wrapping use something like:
> 
> string16 icon_path(ShellUtil::FormatIconLocation(
>     chrome_exe,
>     dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).icon_index));
> 
> (i.e. wrapping at the parenthesis for params is only ok if it fits, but can't
be
> mixed with 4 space indent if it doesn't).

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:933: string16 shortcut_name;
On 2013/05/21 21:35:41, huangs1 wrote:
> string shortcut_name(
>     properties.has_shortcut_name() ? properties.shortcut_name :
>     dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name);

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:933: string16 shortcut_name;
On 2013/05/21 21:35:41, huangs1 wrote:
> string shortcut_name(
>     properties.has_shortcut_name() ? properties.shortcut_name :
>     dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name);

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:1409:
dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name);
On 2013/05/21 21:35:41, huangs1 wrote:
> Add TODO.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:1503: string16
ShellUtil::FormatIconLocation(const string16& icon_path,
On 2013/05/21 21:35:41, huangs1 wrote:
> This is a convenience routine. Consider replacing |icon_index| with:
>   BrowserDistribution* dist,
>   BrowserDistribution::ShortcutEnum* shortcut_enum
> to simplify caller work.
> 
> (Need to change browser/shell_integration_win.cc).

Any reason we pass in an icon_path? Shouldn't ShortcutInfo have the "correct"
icon path so that we can just that in? In fact, why do most distributions have
IconFilename set to an empty string16?

gab

Meta-comment: I hadn't realized how my assumption that "most of the time when we need ...

7 years, 7 months ago (2013-05-24 15:01:34 UTC) #6

Meta-comment:
I hadn't realized how my assumption that "most of the time when we need the
shortcut index, we need the shortcut name, and vice-versa" was so wrong (I
mostly only looked at the browser_distribution.h interface in the first review
and missed that then too)... most uses of GetShortcutInfo are followed by a '.'
to get a specific value... sorry for guiding you down this path...

I still think most of this CL is good though, I just think that instead of
having ShortcutInfo GetShortcutInfo(ShortcutEnum), we should have string16
GetShortcutName(ShortcutEnum), int GetIconIndex(ShortcutEnum), and string16
GetIconFilename(ShortcutEnum) (and as discussed in comments below
GetIconFilename() should be removed soon with the merge of app_host.exe and
chrome.exe anyways...).

That still gets us the benefits of the enum (i.e., preventing adding 3 more
calls for the app launcher) while not having to generate a struct for which most
callers only care about a single member... (the model I suggested earlier (i.e.,
GetShortcutInfo()) bloats many callsites for no apparent benefit... sorry about
that...).

WDYT? (this meta-comment may nullify some comments made below; in case of
conflicts, this meta-comment has priority over other comments I made below).

Cheers,
Gab

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:226:
distribution->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME)
On 2013/05/24 02:03:11, calamity wrote:
> On 2013/05/21 21:46:45, gab wrote:
> > This should be using the subfolder anyways.
> 
> Why? Isn't this chrome.lnk?

Oh, yes, my bad!

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:1503: string16
ShellUtil::FormatIconLocation(const string16& icon_path,
On 2013/05/24 02:03:11, calamity wrote:
> On 2013/05/21 21:35:41, huangs1 wrote:
> > This is a convenience routine. Consider replacing |icon_index| with:
> >   BrowserDistribution* dist,
> >   BrowserDistribution::ShortcutEnum* shortcut_enum
> > to simplify caller work.
> > 
> > (Need to change browser/shell_integration_win.cc).
> 
> Any reason we pass in an icon_path? Shouldn't ShortcutInfo have the "correct"
> icon path so that we can just that in? In fact, why do most distributions have
> IconFilename set to an empty string16?

Distributions with an empty string16 as the icon filename are supposed to be
distributions with no shortcuts (I think).
I see that this is not the case for browser_distribution.cc (Chromium) and this
is probably a bug.

The reason the icon_path is passed by the caller here is because it's the full
path to chrome.exe (which the BrowserDistribution constants can't hardcode as it
is install-path-dependent).

Since the icons will soon always be stored in chrome.exe (such is the plan for
the app launcher icon I think if that's not already the case) it probably makes
sense to remove GetIconFilename() eventually (not in this CL) -- in fact
GetIconFilename() was added exactly to support the double executables holding
icons in http://crrev.com/168722 and we should soon be able to get rid of it.

Only the icon index in chrome.exe changes based on the distribution.

https://chromiumcodereview.appspot.com/15255004/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:1503: string16
ShellUtil::FormatIconLocation(const string16& icon_path,
On 2013/05/21 21:35:41, huangs1 wrote:
> This is a convenience routine. Consider replacing |icon_index| with:
>   BrowserDistribution* dist,
>   BrowserDistribution::ShortcutEnum* shortcut_enum
> to simplify caller work.
> 
> (Need to change browser/shell_integration_win.cc).

I agree with Sam though that for convenience for the caller we can probably just
pass in the ShortcutEnum here instead of the icon_index directly (haven't looked
at all callers to see if that's how it's used everywhere though).

I don't think we can work around passing |icon_path| though.

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:228: +
installer::kLnkExt);
'+' on previous line
and indent 4 spaces in

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:317: +
installer::kLnkExt;
'+' on previous line
+indent 4 spaces in.

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:327: +
installer::kLnkExt,
nit: '+' on previous line

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/s...
File chrome/installer/setup/install_unittest.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/s...
chrome/installer/setup/install_unittest.cc:122: installer::kLnkExt);
nit: Align with |dist_| above.

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/s...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:728:
GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name,
GetDisplayName()

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:795:
dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name);
GetDisplayName()

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:854: GetDisplayName();
Indent 1 more space

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
File chrome/installer/util/browser_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
chrome/installer/util/browser_distribution.cc:174: // TODO(calamity): Replace
with a localized string.
I don't think we have localized Chromium strings in the installer.

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
chrome/installer/util/chrome_app_host_distribution.cc:47: case
SHORTCUT_ALTERNATE_CHROME:
I don't think SHORTCUT_CHROME/SHORTCUT_ALTERNATE_CHROME should be used for this
distribution, move them to the NOTREACHED() section at the end with a comment.

(in fact this distribution can probably be removed soon now that App Launcher is
being made a part of Chrome and that this refactoring makes
Chrome/Chromium/Canary distributions aware of the App Launcher constants)

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
chrome/installer/util/chrome_app_host_distribution.cc:54: info.name = L"Chromium
App Launcher";
Shouldn't this simply become
installer::GetLocalizedString(IDS_PRODUCT_APP_LAUNCHER_NAME_BASE) as line 48
used to be (it was wrong for that to be in GetAlternateApplicationName() imo,
but wtv...).

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
File chrome/installer/util/chrome_frame_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
chrome/installer/util/chrome_frame_distribution.cc:45: case
SHORTCUT_ALTERNATE_CHROME:
SHORTCUT_ALTERNATE_CHROME shouldn't be needed by CF, add to NOTREACHED() section
below with a comment.

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
File chrome/installer/util/chromium_binaries_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
chrome/installer/util/chromium_binaries_distribution.cc:32:
BrowserDistribution::ShortcutInfo ChromiumBinariesDistribution::GetShortcutInfo(
The binaries shouldn't need GetShortcutInfo; if this is only required for
GetDisplayName(), I say make GetShortcutInfo NOTREACHED() as the other methods
and override GetDisplayName() to return kChromiumBinariesName.

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
File chrome/installer/util/google_chrome_binaries_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
chrome/installer/util/google_chrome_binaries_distribution.cc:28:
BrowserDistribution::ShortcutInfo
Same comment here then on chromium_binaries_distribution.cc

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
File chrome/installer/util/google_chrome_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
chrome/installer/util/google_chrome_distribution.cc:140: info.name = L"Chrome
App Launcher Canary";
This should be Google Chrome, not Canary.

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
File chrome/installer/util/google_chrome_sxs_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/u...
chrome/installer/util/google_chrome_sxs_distribution.cc:46: info.name = L"Chrome
App Launcher";
This should be "Canary" not "Google Chrome".

grt (UTC plus 2)

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/util/browser_distribution.cc File chrome/installer/util/browser_distribution.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/installer/util/browser_distribution.cc#newcode174 chrome/installer/util/browser_distribution.cc:174: // TODO(calamity): Replace with a localized string. On 2013/05/24 ...

7 years, 6 months ago (2013-05-29 13:50:09 UTC) #7

calamity

https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/33001/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc#newcode228 chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:228: + installer::kLnkExt); On 2013/05/24 15:01:35, gab wrote: > '+' ...

7 years, 6 months ago (2013-05-31 00:11:18 UTC) #8

gab

Sorry for the long wait, I've been on vacation for 2.5 weeks, back now! Very ...

7 years, 6 months ago (2013-06-18 19:50:29 UTC) #9

calamity

https://chromiumcodereview.appspot.com/15255004/diff/64004/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/64004/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc#newcode607 chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:607: BrowserDistribution::SHORTCUT_CHROME)); On 2013/06/18 19:50:29, gab wrote: > Indent 4 ...

7 years, 5 months ago (2013-06-27 01:27:44 UTC) #10

gab

Very nice, almost there! Cheers! Gab PS: Next time please do separate patch sets for ...

7 years, 5 months ago (2013-06-27 11:49:43 UTC) #11

grt (UTC plus 2)

minuscule drive-by. please update the CL description so that it explains what is being changed ...

7 years, 5 months ago (2013-06-28 04:35:20 UTC) #12

calamity

Description updated. https://codereview.chromium.org/15255004/diff/64004/chrome/installer/util/google_chrome_sxs_distribution.cc File chrome/installer/util/google_chrome_sxs_distribution.cc (right): https://codereview.chromium.org/15255004/diff/64004/chrome/installer/util/google_chrome_sxs_distribution.cc#newcode38 chrome/installer/util/google_chrome_sxs_distribution.cc:38: return L"The Internet"; On 2013/06/27 11:49:43, gab ...

7 years, 5 months ago (2013-07-05 09:00:29 UTC) #13

Description updated.

https://codereview.chromium.org/15255004/diff/64004/chrome/installer/util/goo...
File chrome/installer/util/google_chrome_sxs_distribution.cc (right):

https://codereview.chromium.org/15255004/diff/64004/chrome/installer/util/goo...
chrome/installer/util/google_chrome_sxs_distribution.cc:38: return L"The
Internet";
On 2013/06/27 11:49:43, gab wrote:
> On 2013/06/27 01:27:44, calamity wrote:
> > On 2013/06/18 19:50:29, gab wrote:
> > > Make this a constant now that it is used in multiple places.
> > 
> > Where should I put it?
> 
> Good question. There is already a localized string for it
> (IDS_OEM_MAIN_SHORTCUT_NAME_BASE); perhaps it makes sense to use this here as
in
> google_chrome_distribution.cc and have a chromium version of this string for
the
> other instance of this in browser_distribution.cc (or leave that one hardcoded
> for now?)?
> 
> Otherwise I guess the only common, yet non-public, place to put it would be
> either:
> 1) As a protected member of the BrowserDistribution class.
> 2) In an internals:: namespace in browser_distribution.h (e.g., it's public,
but
> clear by namespace naming that it shouldn't be used outside; we use that
> paradigm in a few places in chrome already, but I don't think it's worth it
for
> this simple use-case...). 

IDS_OEM_MAIN_SHORTCUT_NAME_BASE is "Internet Browser", not "The Internet". I
don't think I want to change that.

Made it a static protected member of BrowserDistribution.

https://codereview.chromium.org/15255004/diff/102003/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://codereview.chromium.org/15255004/diff/102003/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:587:
BrowserDistribution::SHORTCUT_CHROME));
On 2013/06/27 11:49:43, gab wrote:
> nit: Indent 4 more spaces.

Done.

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/br...
File chrome/installer/util/browser_distribution.h (right):

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:32: enum ShortcutEnum {
On 2013/06/28 04:35:21, grt wrote:
> i don't think the suffix "Enum" is idiomatic. please remove it. name the type
> for what it represents, not what it is (i.e., you wouldn't change
> BrowserDistribution to BrowserDistributionClass).

I think this name was suggested offhand in another review. Fixed.

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:70: virtual string16
GetShortcutName(ShortcutEnum shortcut_enum);
On 2013/06/28 04:35:21, grt wrote:
> please remove _enum here and elsewhere

Done.

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:72: // GetIconFilename().
On 2013/06/27 11:49:43, gab wrote:
> Mention what is |shortcut_enum| in this comment (ref. comment proposition
> above).

Done.

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:78: // should create shortcuts in.
On 2013/06/27 11:49:43, gab wrote:
> Mention what is |subfolder_enum| in this comment.

Done.

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/go...
File chrome/installer/util/google_chrome_sxs_distribution.cc (right):

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/go...
chrome/installer/util/google_chrome_sxs_distribution.cc:29: return
installer::GetLocalizedString(IDS_SXS_SHORTCUT_NAME_BASE);
On 2013/06/27 11:49:43, gab wrote:
> GetBaseAppName() is sometimes used as a registry value (I don't like that, but
> it's legacy code and there is nothing we can do about it given the millions of
> installs using this fact atm...).
> 
> This must thus return a constant, put the localized code directly in
> GetShortcutName() -- like in google_chrome_distribution.cc

Done.

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/sh...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/sh...
chrome/installer/util/shell_util.cc:1413: dist->GetStartMenuShortcutSubfolder(
On 2013/06/27 11:49:43, gab wrote:
> nit: This likely fits on the previous line now.

Done.

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/sh...
chrome/installer/util/shell_util.cc:1414:
BrowserDistribution::SUBFOLDER_CHROME));
On 2013/06/27 11:49:43, gab wrote:
> nit: Indent 2 more spaces.
> 
> (or 2 less if comment above applies!)

Done.

https://codereview.chromium.org/15255004/diff/102003/chrome/installer/util/sh...
chrome/installer/util/shell_util.cc:1510: BrowserDistribution* dist =
BrowserDistribution::GetDistribution();
On 2013/06/27 11:49:43, gab wrote:
> Remove this line.

Done.

gab

lgtm w/ changes below. Thanks a lot! Gab PS: Please wait for grt's review as ...

7 years, 5 months ago (2013-07-05 14:23:36 UTC) #14

grt (UTC plus 2)

It seems that ChromeAppHostDistribution has shortcut-related methods that should never be called now. This is ...

7 years, 5 months ago (2013-07-05 17:27:37 UTC) #15

It seems that ChromeAppHostDistribution has shortcut-related methods that should
never be called now. This is confusing. Is there a short-term plan to eradicate
CAHD from the codebase?

Please add enough detail to the CL description so that anyone can read it and
get a better sense of what's going on here. Try to think about how the message
can be meaningful to you in six months, or to a new member of the team in six
days. For example, you could include:

* Added GetStartMenuShortcutSubfolder, which returns the Start Menu
  subfolder for a type of shortcut.

* GetShortcutName and GetIconIndex now return data specific to a type of
  shortcut (browser, alternate name of browser, or app launcher). These
  are used on a Type=CHROME_BROWSER distribution henceforth to get the
  names of browser and app launcher shortcuts.

* GetDisplayName is now used rather than GetAppShortcutName where a
  localized name is needed (e.g., in logs and the registry).

Try to wrap at 72 cols for for happier log messages in git.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:728: true);
nit: either put each argument on its own line, or pack them as tightly as
possible (i.e., move "true" up to the previous line). i prefer the latter here.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
File chrome/installer/util/browser_distribution.cc (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.cc:69: const char*
BrowserDistribution::kDefaultAlternateChromeShortcutName =
On 2013/07/05 14:23:37, gab wrote:
> Actually, this probably belongs more at the bottom of
> chrome/installer/util/util_constants.cc

comment on comment: my gut tells me that static initializers should come before
the ctors at the top of the implementation, not at the end of the file.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.cc:165: switch (shortcut_type) {
for all functions like this, make the NOTREACHED case do something sensible.
it's better to return some wrong value in a release build than to return a
nonsensical value. my suggestion here is:
  switch (shortcut_type) {
    case SHORTCUT_ALTERNATE_CHROME:
      // TODO(calamity): Change IDS_OEM_MAIN_SHORTCUT_NAME in
      // chromium_strings.grd to "The Internet" (so that it doesn't collide with
      // the value in google_chrome_strings.grd) then change this to
      // installer::GetLocalizedString(IDS_OEM_MAIN_SHORTCUT_NAME_BASE)
      return L"The Internet";
    case SHORTCUT_APP_LAUNCHER:
      return installer::GetLocalizedString(IDS_APP_LIST_SHORTCUT_NAME_BASE);
    default:
      DCHECK_EQ(shortcut_type, SHORTCUT_CHROME);
      return GetBaseAppName();
  }

you'll also need to add IDS_APP_LIST_SHORTCUT_NAME to kStringIds in
chrome/installer/util/prebuild/create_string_rc.py.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.cc:180: switch (subfolder_type) {
as above:
  DCHECK_EQ(subfolder_type, SUBFOLDER_CHROME);
  return GetShortcutName(SHORTCUT_CHROME);

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.cc:255: string16
BrowserDistribution::GetIconFilename() {
move this up so that it immediately precedes GetStartMenuShortcutSubfolder

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.cc:256: return string16();
how do icons work for Chromium builds?

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.cc:259: int
BrowserDistribution::GetIconIndex(ShortcutType shortcut_type) {
move this up so that it immediately follows GetShortcutName

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.cc:260: switch (shortcut_type) {
as elsewhere:
  if (shortcut_type == SHORTCUT_APP_LAUNCHER)
    return kAppLauncherIconIndex;  (1, defined in the unnamed namespace)
  DCHECK(shortcut_type == SHORTCUT_CHROME ||
         shortcut_type == SHORTCUT_ALTERNATE_CHROME) << shortcut_type;
  return 0;

Also, please make kAppLauncherIconIndex a file-local constant in the unnamed
namespace and put a comment next to it along the lines of
// The Chromium App Launcher icon is index 1; see chrome_exe.rc.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
File chrome/installer/util/browser_distribution.h (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:34: SHORTCUT_ALTERNATE_CHROME,
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.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:82: // |subfolder_type| that this
distribution should create shortcuts in.
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.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:150: static const char*
kDefaultAlternateChromeShortcutName;
This appears to be unused. Remove it?

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
chrome/installer/util/chrome_app_host_distribution.cc:47: switch (shortcut_type)
{
as elsewhere:
  DCHECK_EQ(shortcut_type, SHORTCUT_APP_LAUNCHER);
  return installer::GetLocalizedString(IDS_PRODUCT_APP_LAUNCHER_NAME_BASE);

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
chrome/installer/util/chrome_app_host_distribution.cc:137: return
installer::kChromeAppHostExe;
Is this correct now? Shouldn't the IDR_X_APP_LIST (1 or 5) or IDR_X_APP_LIST_SXS
(6) icon in chrome.exe be used for the app launcher? Shouldn't all code that
used to get the app launcher icon by way of ChromeAppHostDistribution now
instead get it by way of the active Type=CHROME_BROWSER BrowserDistribution? Is
ChromeAppHostOperations every used (by way of instantiating Product for a
CHROME_APP_HOST distribution)?

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
File chrome/installer/util/chrome_app_host_distribution.h (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
chrome/installer/util/chrome_app_host_distribution.h:20: virtual string16
GetShortcutName(ShortcutType shortcut_type) OVERRIDE;
Please document here that this can only be called for shortcut_type ==
SHORTCUT_APP_LAUNCHER.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/go...
File chrome/installer/util/google_chrome_distribution.cc (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/go...
chrome/installer/util/google_chrome_distribution.cc:129: switch (shortcut_type)
{
  int string_id = IDS_PRODUCT_NAME_BASE;
  switch (shortcut_type) {
    case SHORTCUT_ALTERNATE_CHROME:
      string_id = IDS_OEM_MAIN_SHORTCUT_NAME_BASE;
      break;
    case SHORTCUT_APP_LAUNCHER:
      string_id = IDS_APP_LIST_SHORTCUT_NAME_BASE;
      break;
    default:
      DCHECK_EQ(shortcut_type, SHORTCUT_CHROME);
      break;
  }
  return installer::GetLocalizedString(string_id);

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/go...
chrome/installer/util/google_chrome_distribution.cc:144: switch (shortcut_type)
{
  if (shortcut_type == SHORTCUT_APP_LAUNCHER)
    return kAppLauncherIconIndex;  (5, defined in the unnamed namespace)
  DCHECK(shortcut_type == SHORTCUT_CHROME ||
         shortcut_type == SHORTCUT_ALTERNATE_CHROME) << shortcut_type;
  return 0;

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/go...
chrome/installer/util/google_chrome_distribution.cc:149: return 6;
Looks like this should be 5 according to chrome_exe.rc. Also, please make it a
file-local constant in the unnamed namespace (e.g., kAppLauncherIconIndex) and
put a comment next to it along the lines of
// The Google Chrome App Launcher icon is index 5; see chrome_exe.rc.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/go...
File chrome/installer/util/google_chrome_sxs_distribution.cc (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/go...
chrome/installer/util/google_chrome_sxs_distribution.cc:34: switch
(shortcut_type) {
I think it's bad for this to ever return the empty string. How about:
  switch (shortcut_type) {
    case SHORTCUT_ALTERNATE_CHROME:
      // This should never be called. Returning the same string as Google Chrome
      // preserves behavior, but it will result in a naming collision.
      NOTREACHED();
      return GoogleChromeDistribution::GetShortcutName(shortcut_type);
    case SHORTCUT_APP_LAUNCHER:
      return installer::GetLocalizedString(
          IDS_APP_LIST_SHORTCUT_NAME_CANARY_BASE);
    default:
      DCHECK_EQ(shortcut_type, SHORTCUT_CHROME);
      return installer::GetLocalizedString(IDS_SXS_SHORTCUT_NAME_BASE);
  }

You'll need to add IDS_APP_LIST_SHORTCUT_NAME_CANARY to kStringIds in
chrome/installer/util/prebuild/create_string_rc.py.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/go...
chrome/installer/util/google_chrome_sxs_distribution.cc:75: return 0;
this should never return 0, since that would result in the regular chrome icon.
how about:
  if (shortcut_type == SHORTCUT_APP_LAUNCHER)
    return kSxSAppLauncherIconIndex;  (6, defined in the unnamed namespace)
  DCHECK(shortcut_type == SHORTCUT_CHROME ||
         shortcut_type == SHORTCUT_ALTERNATE_CHROME) << shortcut_type;
  return kSxSIconIndex;

Also, please make kSxSAppLauncherIconIndexa file-local constant in the unnamed
namespace and put a comment next to it along the lines of
// The Chrome App Launcher Canary icon is index 6; see chrome_exe.rc.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/sh...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/sh...
chrome/installer/util/shell_util.cc:367: dist->GetDisplayName()));
nit: unwrap

gab

Greg brings up a lot of good points (thanks!). I agree that we need a ...

7 years, 5 months ago (2013-07-08 12:59:01 UTC) #16

Greg brings up a lot of good points (thanks!).

I agree that we need a plan to get rid of ChromeAppHostDistribution asap (I
still think this is a prereq CL for this though, but let's put a quick doc
together of what needs to change before we can just tank the CAHD).

Cheers!
Gab

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
File chrome/installer/util/browser_distribution.h (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:34: SHORTCUT_ALTERNATE_CHROME,
On 2013/07/05 17:27:37, grt wrote:
> 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.

Suggestion sgtm, please use SHORTCUT_CHROME_ALTERNATE.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:82: // |subfolder_type| that this
distribution should create shortcuts in.
On 2013/07/05 17:27:37, grt wrote:
> 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.

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?

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
chrome/installer/util/chrome_app_host_distribution.cc:137: return
installer::kChromeAppHostExe;
On 2013/07/05 17:27:37, grt wrote:
> Is this correct now? Shouldn't the IDR_X_APP_LIST (1 or 5) or
IDR_X_APP_LIST_SXS
> (6) icon in chrome.exe be used for the app launcher? Shouldn't all code that
> used to get the app launcher icon by way of ChromeAppHostDistribution now
> instead get it by way of the active Type=CHROME_BROWSER BrowserDistribution?
Is
> ChromeAppHostOperations every used (by way of instantiating Product for a
> CHROME_APP_HOST distribution)?

@huangs: I'm not sure what the exact current state is, let's put a plan together
to remove ChromeAppHostDistribution (and product) and make sure this CL is
inline with this plan.

Having a distribution that has to know about Google Chrome, Canary, and Chromium
just doesn't make sense (hence why we are introducing app launcher knowledge in
the distributions in this CL).

I'm ok with getting rid of the CAHD as a follow-up step, but this CL and the
CAHD shouldn't co-exist for long or things will get confusing.

grt (UTC plus 2)

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/browser_distribution.h File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/browser_distribution.h#newcode82 chrome/installer/util/browser_distribution.h:82: // |subfolder_type| that this distribution should create shortcuts in. ...

7 years, 5 months ago (2013-07-08 15:15:49 UTC) #17

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
File chrome/installer/util/browser_distribution.h (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/br...
chrome/installer/util/browser_distribution.h:82: // |subfolder_type| that this
distribution should create shortcuts in.
On 2013/07/08 12:59:02, gab wrote:
> On 2013/07/05 17:27:37, grt wrote:
> > 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.
> 
> 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?

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.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
chrome/installer/util/chrome_app_host_distribution.cc:137: return
installer::kChromeAppHostExe;
On 2013/07/08 12:59:02, gab wrote:
> On 2013/07/05 17:27:37, grt wrote:
> > Is this correct now? Shouldn't the IDR_X_APP_LIST (1 or 5) or
> IDR_X_APP_LIST_SXS
> > (6) icon in chrome.exe be used for the app launcher? Shouldn't all code that
> > used to get the app launcher icon by way of ChromeAppHostDistribution now
> > instead get it by way of the active Type=CHROME_BROWSER BrowserDistribution?
> Is
> > ChromeAppHostOperations every used (by way of instantiating Product for a
> > CHROME_APP_HOST distribution)?
> 
> @huangs: I'm not sure what the exact current state is, let's put a plan
together
> to remove ChromeAppHostDistribution (and product) and make sure this CL is
> inline with this plan.
> 
> Having a distribution that has to know about Google Chrome, Canary, and
Chromium
> just doesn't make sense (hence why we are introducing app launcher knowledge
in
> the distributions in this CL).
> 
> I'm ok with getting rid of the CAHD as a follow-up step, but this CL and the
> CAHD shouldn't co-exist for long or things will get confusing.

Just a thought: if it's a challenge to get rid of ChromeAppHostDistribution in
the short term, does it make sense to modify its implementation so that it
delegates to the active Type=CHROME_BROWSER BrowserDistribution? This could help
mitigate the risks associated with having two implementations for the same
thing. I haven't thought this through in detail, so I'm not sure it makes
absolute sense.

gab

@huangs, see discussion below. https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/chrome_app_host_distribution.cc File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/chrome_app_host_distribution.cc#newcode137 chrome/installer/util/chrome_app_host_distribution.cc:137: return installer::kChromeAppHostExe; On 2013/07/08 15:15:50, ...

7 years, 5 months ago (2013-07-08 15:22:40 UTC) #18

@huangs, see discussion below.

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/ch...
chrome/installer/util/chrome_app_host_distribution.cc:137: return
installer::kChromeAppHostExe;
On 2013/07/08 15:15:50, grt wrote:
> On 2013/07/08 12:59:02, gab wrote:
> > On 2013/07/05 17:27:37, grt wrote:
> > > Is this correct now? Shouldn't the IDR_X_APP_LIST (1 or 5) or
> > IDR_X_APP_LIST_SXS
> > > (6) icon in chrome.exe be used for the app launcher? Shouldn't all code
that
> > > used to get the app launcher icon by way of ChromeAppHostDistribution now
> > > instead get it by way of the active Type=CHROME_BROWSER
BrowserDistribution?
> > Is
> > > ChromeAppHostOperations every used (by way of instantiating Product for a
> > > CHROME_APP_HOST distribution)?
> > 
> > @huangs: I'm not sure what the exact current state is, let's put a plan
> together
> > to remove ChromeAppHostDistribution (and product) and make sure this CL is
> > inline with this plan.
> > 
> > Having a distribution that has to know about Google Chrome, Canary, and
> Chromium
> > just doesn't make sense (hence why we are introducing app launcher knowledge
> in
> > the distributions in this CL).
> > 
> > I'm ok with getting rid of the CAHD as a follow-up step, but this CL and the
> > CAHD shouldn't co-exist for long or things will get confusing.
> 
> Just a thought: if it's a challenge to get rid of ChromeAppHostDistribution in
> the short term, does it make sense to modify its implementation so that it
> delegates to the active Type=CHROME_BROWSER BrowserDistribution? This could
help
> mitigate the risks associated with having two implementations for the same
> thing. I haven't thought this through in detail, so I'm not sure it makes
> absolute sense.

I thought about that too, but it feels scary to me, we can think about this some
more if you insist, but I'd really rather get rid of CAHD asap (is that
possible?).

huangs

https://codereview.chromium.org/15255004/diff/129004/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/15255004/diff/129004/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc#newcode216 chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:216: BrowserDistribution* distribution = GetDistribution(); If you want, you may ...

7 years, 5 months ago (2013-07-08 19:43:56 UTC) #19

gab

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/browser_distribution.h File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/browser_distribution.h#newcode34 chrome/installer/util/browser_distribution.h:34: SHORTCUT_ALTERNATE_CHROME, On 2013/07/08 19:43:57, huangs1 wrote: > (Optional) please ...

7 years, 5 months ago (2013-07-08 20:20:32 UTC) #20

huangs

https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/browser_distribution.h File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/15255004/diff/129004/chrome/installer/util/browser_distribution.h#newcode34 chrome/installer/util/browser_distribution.h:34: SHORTCUT_ALTERNATE_CHROME, Sounds good.

7 years, 5 months ago (2013-07-09 15:02:52 UTC) #21

calamity

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc#newcode216 chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:216: BrowserDistribution* distribution = GetDistribution(); On 2013/07/08 19:43:57, huangs1 wrote: ...

7 years, 5 months ago (2013-07-16 04:05:17 UTC) #22

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:216:
BrowserDistribution* distribution = GetDistribution();
On 2013/07/08 19:43:57, huangs1 wrote:
> If you want, you may use |dist| instead of |distribution|. This is a commonly
> used abbreviation in the installer.

I'd prefer to keep the diff of this CL focused on the BrowserDistribution
refactor as much as possible.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:543:
shortcut_name.append(distribution->
On 2013/07/05 14:23:37, gab wrote:
> nit: wrap |distribution| to the next line -- i.e., either indent at the
> parenthesis or 4 spaces in, not a mix of both.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/setup/install_worker.cc:728: true);
On 2013/07/05 17:27:37, grt wrote:
> nit: either put each argument on its own line, or pack them as tightly as
> possible (i.e., move "true" up to the previous line). i prefer the latter
here.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/setup/install_worker.cc:1438: BrowserDistribution* distribution
= product.distribution();
On 2013/07/08 19:43:57, huangs1 wrote:
> Nit: |dist| instead of |distribution| for conciseness and consistency within
> file.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/setup/install_worker.cc:1500: BrowserDistribution* distribution
= product.distribution();
On 2013/07/08 19:43:57, huangs1 wrote:
> Nit: |dist| instead of |distribution| for conciseness and consistency within
> file.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
File chrome/installer/util/browser_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.cc:165: switch (shortcut_type) {
On 2013/07/05 17:27:37, grt wrote:
> for all functions like this, make the NOTREACHED case do something sensible.
> it's better to return some wrong value in a release build than to return a
> nonsensical value. my suggestion here is:
>   switch (shortcut_type) {
>     case SHORTCUT_ALTERNATE_CHROME:
>       // TODO(calamity): Change IDS_OEM_MAIN_SHORTCUT_NAME in
>       // chromium_strings.grd to "The Internet" (so that it doesn't collide
with
>       // the value in google_chrome_strings.grd) then change this to
>       // installer::GetLocalizedString(IDS_OEM_MAIN_SHORTCUT_NAME_BASE)
>       return L"The Internet";
>     case SHORTCUT_APP_LAUNCHER:
>       return installer::GetLocalizedString(IDS_APP_LIST_SHORTCUT_NAME_BASE);
>     default:
>       DCHECK_EQ(shortcut_type, SHORTCUT_CHROME);
>       return GetBaseAppName();
>   }
> 
> you'll also need to add IDS_APP_LIST_SHORTCUT_NAME to kStringIds in
> chrome/installer/util/prebuild/create_string_rc.py.

Done. Any reason to use DCHECK_EQ instead of NOTREACHED()?

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.cc:180: switch (subfolder_type) {
On 2013/07/05 17:27:37, grt wrote:
> as above:
>   DCHECK_EQ(subfolder_type, SUBFOLDER_CHROME);
>   return GetShortcutName(SHORTCUT_CHROME);

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.cc:255: string16
BrowserDistribution::GetIconFilename() {
On 2013/07/05 17:27:37, grt wrote:
> move this up so that it immediately precedes GetStartMenuShortcutSubfolder

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.cc:256: return string16();
On 2013/07/05 17:27:37, grt wrote:
> how do icons work for Chromium builds?

I'm not sure. Does anybody else know?

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.cc:259: int
BrowserDistribution::GetIconIndex(ShortcutType shortcut_type) {
On 2013/07/05 17:27:37, grt wrote:
> move this up so that it immediately follows GetShortcutName

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.cc:260: switch (shortcut_type) {
On 2013/07/05 17:27:37, grt wrote:
> as elsewhere:
>   if (shortcut_type == SHORTCUT_APP_LAUNCHER)
>     return kAppLauncherIconIndex;  (1, defined in the unnamed namespace)
>   DCHECK(shortcut_type == SHORTCUT_CHROME ||
>          shortcut_type == SHORTCUT_ALTERNATE_CHROME) << shortcut_type;
>   return 0;
> 
> Also, please make kAppLauncherIconIndex a file-local constant in the unnamed
> namespace and put a comment next to it along the lines of
> // The Chromium App Launcher icon is index 1; see chrome_exe.rc.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
File chrome/installer/util/browser_distribution.h (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.h:34: SHORTCUT_ALTERNATE_CHROME,
On 2013/07/08 12:59:02, gab wrote:
> On 2013/07/05 17:27:37, grt wrote:
> > 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.
> 
> Suggestion sgtm, please use SHORTCUT_CHROME_ALTERNATE.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.h:67: // Returns the display name of
this distribution.
On 2013/07/05 14:23:37, gab wrote:
> 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).

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.h:70: // Returns the name of the
shortcut identified by |shortcut_type| for this
On 2013/07/05 14:23:37, gab wrote:
> s/name/localized name
> 
> (same idea as above)

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.h:81: // Returns the name of the
subfolder in the Start Menu identified by
On 2013/07/05 14:23:37, gab wrote:
> s/name/localized name
> 
> (same idea as above)

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.h:82: // |subfolder_type| that this
distribution should create shortcuts in.
On 2013/07/08 15:15:50, grt wrote:
> On 2013/07/08 12:59:02, gab wrote:
> > On 2013/07/05 17:27:37, grt wrote:
> > > 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.
> > 
> > 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?
> 
> 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.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.h:150: static const char*
kDefaultAlternateChromeShortcutName;
On 2013/07/05 17:27:37, grt wrote:
> This appears to be unused. Remove it?

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/chrome_app_host_distribution.cc:47: switch (shortcut_type)
{
On 2013/07/05 17:27:37, grt wrote:
> as elsewhere:
>   DCHECK_EQ(shortcut_type, SHORTCUT_APP_LAUNCHER);
>   return installer::GetLocalizedString(IDS_PRODUCT_APP_LAUNCHER_NAME_BASE);

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
File chrome/installer/util/chrome_app_host_distribution.h (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/chrome_app_host_distribution.h:20: virtual string16
GetShortcutName(ShortcutType shortcut_type) OVERRIDE;
On 2013/07/05 17:27:37, grt wrote:
> Please document here that this can only be called for shortcut_type ==
> SHORTCUT_APP_LAUNCHER.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
File chrome/installer/util/google_chrome_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/google_chrome_distribution.cc:129: switch (shortcut_type)
{
On 2013/07/05 17:27:37, grt wrote:
>   int string_id = IDS_PRODUCT_NAME_BASE;
>   switch (shortcut_type) {
>     case SHORTCUT_ALTERNATE_CHROME:
>       string_id = IDS_OEM_MAIN_SHORTCUT_NAME_BASE;
>       break;
>     case SHORTCUT_APP_LAUNCHER:
>       string_id = IDS_APP_LIST_SHORTCUT_NAME_BASE;
>       break;
>     default:
>       DCHECK_EQ(shortcut_type, SHORTCUT_CHROME);
>       break;
>   }
>   return installer::GetLocalizedString(string_id);

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/google_chrome_distribution.cc:144: switch (shortcut_type)
{
On 2013/07/05 17:27:37, grt wrote:
>   if (shortcut_type == SHORTCUT_APP_LAUNCHER)
>     return kAppLauncherIconIndex;  (5, defined in the unnamed namespace)
>   DCHECK(shortcut_type == SHORTCUT_CHROME ||
>          shortcut_type == SHORTCUT_ALTERNATE_CHROME) << shortcut_type;
>   return 0;

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/google_chrome_distribution.cc:149: return 6;
On 2013/07/05 17:27:37, grt wrote:
> Looks like this should be 5 according to chrome_exe.rc. Also, please make it a
> file-local constant in the unnamed namespace (e.g., kAppLauncherIconIndex) and
> put a comment next to it along the lines of
> // The Google Chrome App Launcher icon is index 5; see chrome_exe.rc.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
File chrome/installer/util/google_chrome_sxs_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/google_chrome_sxs_distribution.cc:34: switch
(shortcut_type) {
On 2013/07/05 17:27:37, grt wrote:
> I think it's bad for this to ever return the empty string. How about:
>   switch (shortcut_type) {
>     case SHORTCUT_ALTERNATE_CHROME:
>       // This should never be called. Returning the same string as Google
Chrome
>       // preserves behavior, but it will result in a naming collision.
>       NOTREACHED();
>       return GoogleChromeDistribution::GetShortcutName(shortcut_type);
>     case SHORTCUT_APP_LAUNCHER:
>       return installer::GetLocalizedString(
>           IDS_APP_LIST_SHORTCUT_NAME_CANARY_BASE);
>     default:
>       DCHECK_EQ(shortcut_type, SHORTCUT_CHROME);
>       return installer::GetLocalizedString(IDS_SXS_SHORTCUT_NAME_BASE);
>   }
> 
> You'll need to add IDS_APP_LIST_SHORTCUT_NAME_CANARY to kStringIds in
> chrome/installer/util/prebuild/create_string_rc.py.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/google_chrome_sxs_distribution.cc:36: return
GetBaseAppName();
On 2013/07/05 14:23:37, gab wrote:
> Make this
> installer::GetLocalizedString(IDS_SXS_SHORTCUT_NAME_BASE);
> now that GetBaseAppName() is hardcoded though.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/google_chrome_sxs_distribution.cc:38: return L"The
Internet";
On 2013/07/05 14:23:37, gab wrote:
> Actually, this should just use
> GoogleChromeDsitribution::GetShortcutName(SHORTCUT_ALTERNATE_CHROME);
> I think, to keep the old behavior.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/google_chrome_sxs_distribution.cc:75: return 0;
On 2013/07/05 17:27:37, grt wrote:
> this should never return 0, since that would result in the regular chrome
icon.
> how about:
>   if (shortcut_type == SHORTCUT_APP_LAUNCHER)
>     return kSxSAppLauncherIconIndex;  (6, defined in the unnamed namespace)
>   DCHECK(shortcut_type == SHORTCUT_CHROME ||
>          shortcut_type == SHORTCUT_ALTERNATE_CHROME) << shortcut_type;
>   return kSxSIconIndex;
> 
> Also, please make kSxSAppLauncherIconIndexa file-local constant in the unnamed
> namespace and put a comment next to it along the lines of
> // The Chrome App Launcher Canary icon is index 6; see chrome_exe.rc.

Done.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/shell_util.cc:367: dist->GetDisplayName()));
On 2013/07/05 17:27:37, grt wrote:
> nit: unwrap

Done.

grt (UTC plus 2)

looking good. almost there. https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/util/browser_distribution.cc File chrome/installer/util/browser_distribution.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/util/browser_distribution.cc#newcode165 chrome/installer/util/browser_distribution.cc:165: switch (shortcut_type) { On 2013/07/16 ...

7 years, 5 months ago (2013-07-16 19:38:43 UTC) #23

looking good. almost there.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
File chrome/installer/util/browser_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.cc:165: switch (shortcut_type) {
On 2013/07/16 04:05:17, calamity wrote:
> On 2013/07/05 17:27:37, grt wrote:
> > for all functions like this, make the NOTREACHED case do something sensible.
> > it's better to return some wrong value in a release build than to return a
> > nonsensical value. my suggestion here is:
> >   switch (shortcut_type) {
> >     case SHORTCUT_ALTERNATE_CHROME:
> >       // TODO(calamity): Change IDS_OEM_MAIN_SHORTCUT_NAME in
> >       // chromium_strings.grd to "The Internet" (so that it doesn't collide
> with
> >       // the value in google_chrome_strings.grd) then change this to
> >       // installer::GetLocalizedString(IDS_OEM_MAIN_SHORTCUT_NAME_BASE)
> >       return L"The Internet";
> >     case SHORTCUT_APP_LAUNCHER:
> >       return installer::GetLocalizedString(IDS_APP_LIST_SHORTCUT_NAME_BASE);
> >     default:
> >       DCHECK_EQ(shortcut_type, SHORTCUT_CHROME);
> >       return GetBaseAppName();
> >   }
> > 
> > you'll also need to add IDS_APP_LIST_SHORTCUT_NAME to kStringIds in
> > chrome/installer/util/prebuild/create_string_rc.py.
> 
> Done. Any reason to use DCHECK_EQ instead of NOTREACHED()?

DCHECK_EQ will show the value that shouldn't have happened in the log. Also, I'm
not sure how you'd structure the code to use NOTREACHED without having any
duplicate code. I suppose you could have case SHORTCUT_CHROME: break; default:
NOTREACHED(); break; } return GetBaseAppName(), but I find my suggested
structure more concise and clear.

https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/...
chrome/installer/util/browser_distribution.cc:256: return string16();
On 2013/07/16 04:05:17, calamity wrote:
> On 2013/07/05 17:27:37, grt wrote:
> > how do icons work for Chromium builds?
> 
> I'm not sure. Does anybody else know?

I just installed Chromium r211796, and the uninstall link in the Programs and
Features control panel is missing its icon because of this. I wonder how long
it's been broken. Please change this to return installer::kChromeExe and add
issue 161985 to the list of those fixed by this CL.

https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/...
chrome/installer/setup/install_worker.cc:1512:
InstallUtil::GetActiveSetupPath(dist));
nit: unwrap

https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/...
File chrome/installer/util/browser_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/...
chrome/installer/util/browser_distribution.cc:194: switch (subfolder_type) {
remove this switch statement. the only two lines needed in this function are the
DCHECK and the return.

https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/...
File chrome/installer/util/google_chrome_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/...
chrome/installer/util/google_chrome_distribution.cc:146: // The Google Chrome
App Launcher icon is index 5; see chrome_exe.rc.
please move this comment to the definition of kAppLauncherIconIndex.

https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/...
File chrome/installer/util/google_chrome_sxs_distribution.cc (right):

https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/...
chrome/installer/util/google_chrome_sxs_distribution.cc:69: // The Chrome App
Launcher Canary icon is index 6; see chrome_exe.rc.
please move this comment up to the definition of kSxSAppLauncherIconIndex.

calamity

https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/setup/install_worker.cc File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/155001/chrome/installer/setup/install_worker.cc#newcode1512 chrome/installer/setup/install_worker.cc:1512: InstallUtil::GetActiveSetupPath(dist)); On 2013/07/16 19:38:44, grt wrote: > nit: unwrap ...

7 years, 5 months ago (2013-07-18 07:56:14 UTC) #24

grt (UTC plus 2)

lgtm with change below to make BrowserDistribution::GetIconFilename() return installer::kChromeExe. https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/util/browser_distribution.cc File chrome/installer/util/browser_distribution.cc (right): https://chromiumcodereview.appspot.com/15255004/diff/129004/chrome/installer/util/browser_distribution.cc#newcode256 chrome/installer/util/browser_distribution.cc:256: ...

7 years, 5 months ago (2013-07-18 17:43:42 UTC) #25

calamity

+sky for chrome/browser/profiles, chrome/browser/shell_integration.h and chrome/browser/ui/views/app_list/app_list_controller_win.cc These files just use functions affected by this refactor. ...

7 years, 5 months ago (2013-07-19 04:05:18 UTC) #26

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/15255004/197001

7 years, 5 months ago (2013-07-22 06:13:10 UTC) #28

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/15255004/197001

7 years, 5 months ago (2013-07-22 06:14:54 UTC) #29

commit-bot: I haz the power

Sorry for I got bad news for ya. Compile failed with a clobber build on ...

7 years, 5 months ago (2013-07-22 07:27:43 UTC) #30

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/15255004/216001

7 years, 5 months ago (2013-07-22 08:16:13 UTC) #31

commit-bot: I haz the power

Sorry for I got bad news for ya. Compile failed with a clobber build on ...

7 years, 5 months ago (2013-07-22 09:16:43 UTC) #32

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/15255004/233001

7 years, 5 months ago (2013-07-22 10:25:23 UTC) #33

gab

A few nits in your latest patchsets -- some calls should use GetSubFolder instead of ...

7 years, 5 months ago (2013-07-22 16:09:13 UTC) #34

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/15255004/248001

7 years, 5 months ago (2013-07-23 06:33:27 UTC) #35

commit-bot: I haz the power

Sorry for I got bad news for ya. Compile failed with a clobber build on ...

7 years, 5 months ago (2013-07-23 08:00:52 UTC) #36

gab

On 2013/07/23 08:00:52, I haz the power (commit-bot) wrote: > Sorry for I got bad ...

7 years, 4 months ago (2013-08-12 16:38:15 UTC) #37

calamity

https://codereview.chromium.org/15255004/diff/197001/chrome/installer/util/shell_util_unittest.cc File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/15255004/diff/197001/chrome/installer/util/shell_util_unittest.cc#newcode104 chrome/installer/util/shell_util_unittest.cc:104: dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME)); On 2013/07/22 16:09:14, gab wrote: > This should ...

7 years, 4 months ago (2013-08-13 10:29:07 UTC) #38

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/15255004/301001

7 years, 4 months ago (2013-08-13 10:29:22 UTC) #39

commit-bot: I haz the power

Retried try job too often on mac_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=159262

7 years, 4 months ago (2013-08-13 11:45:12 UTC) #40

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/15255004/328001

7 years, 4 months ago (2013-08-14 00:51:10 UTC) #41

Message was sent while issue was closed.

Change committed as 217469

Expand Messages | Collapse Messages