Add a flag to control whether there is a shortcut for the app list / launcher in the Windows taskba…

Add a flag to control whether there is a shortcut for the app list / launcher in the Windows taskbar.

When the flag is on, a shortcut will be created and pinned if it doesn't exist. When it is off, the shortcut will be deleted if it exists.

BUG=158715
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166144

[benwells, 2012-10-31 08:39:29]

sky@ for overall
grt@ for windows shortcut related changes

[sky, 2012-10-31 14:20:52]

http://codereview.chromium.org/11367002/diff/9002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11367002/diff/9002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6730: +      <message
name="IDS_FLAGS_SHOW_APP_LIST_SHORTCUT_NAME" desc="Name for the flag to show the
app list shortcut in teh taskbar.">
teh -> the

http://codereview.chromium.org/11367002/diff/9002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6734: +        Show a shortcut to the Chrome
Apps Launcher in the Windows TaskBar.
If you're going to use Chrome in the name, then you nee to separate this into
chromium_strings and google_chrome_strings.

http://codereview.chromium.org/11367002/diff/9002/chrome/browser/ui/views/app...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/9002/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:364: void
DoCheckAppListTaskbarShortcut(const FilePath user_data_dir,
Add a description.

http://codereview.chromium.org/11367002/diff/9002/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:366: const wchar_t
kAppListShortcutFileName[] = L"Chrome Apps Launcher.lnk";
DCHECK file thread.

[benwells, 2012-11-01 05:31:05]

Also updated:

- use chrome / chromium specific strings for the shortcut file name and
description, and for the app id.

- used the constant i create for the resource index :-/

- removed the TODO i added in shell_integration_win, on reflection there are
problems but I need to look into them more before committing to a specific todo

https://codereview.chromium.org/11367002/diff/9002/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/11367002/diff/9002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6730: +      <message
name="IDS_FLAGS_SHOW_APP_LIST_SHORTCUT_NAME" desc="Name for the flag to show the
app list shortcut in teh taskbar.">
On 2012/10/31 14:20:52, sky wrote:
> teh -> the

Done.

https://codereview.chromium.org/11367002/diff/9002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6734: +        Show a shortcut to the Chrome
Apps Launcher in the Windows TaskBar.
On 2012/10/31 14:20:52, sky wrote:
> If you're going to use Chrome in the name, then you nee to separate this into
> chromium_strings and google_chrome_strings.

Done.

https://codereview.chromium.org/11367002/diff/9002/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/11367002/diff/9002/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:364: void
DoCheckAppListTaskbarShortcut(const FilePath user_data_dir,
On 2012/10/31 14:20:52, sky wrote:
> Add a description.

Done.

https://codereview.chromium.org/11367002/diff/9002/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:366: const wchar_t
kAppListShortcutFileName[] = L"Chrome Apps Launcher.lnk";
On 2012/10/31 14:20:52, sky wrote:
> DCHECK file thread.

It was down, it's now at the top.

[sky, 2012-11-01 16:11:39]

LGTM

[gab, 2012-11-01 22:18:11]

General comment for now.

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integ...
File chrome/browser/shell_integration_win.cc (right):

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integ...
chrome/browser/shell_integration_win.cc:424: return
ShellIntegration::GetAppModelIdForProfile(kAppListAppName,
huangs@ is introducing something very similar in
http://codereview.chromium.org/11359013/, your appid should start from the
distribution's BaseAppId, then optionally (but I highly recommend it to avoid
trouble later, e.g. say you ever want to support system-level app launcher
installs) add a per-user suffix, and finally add the profile part of the appid.

What I suggested to huangs@ and erikwright@ is that you modify
ShellUtil::GetBrowserModelId to be more generic (i.e. not browser only -- IIRC
might just need a name change...).

[benwells, 2012-11-01 22:59:08]

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integ...
File chrome/browser/shell_integration_win.cc (right):

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integ...
chrome/browser/shell_integration_win.cc:424: return
ShellIntegration::GetAppModelIdForProfile(kAppListAppName,
On 2012/11/01 22:18:11, gab wrote:
> huangs@ is introducing something very similar in
> http://codereview.chromium.org/11359013/, your appid should start from the
> distribution's BaseAppId, then optionally (but I highly recommend it to avoid
> trouble later, e.g. say you ever want to support system-level app launcher
> installs) add a per-user suffix, and finally add the profile part of the
appid.
> 
> What I suggested to huangs@ and erikwright@ is that you modify
> ShellUtil::GetBrowserModelId to be more generic (i.e. not browser only -- IIRC
> might just need a name change...).

Yes I saw that CL this morning. I had assumed that going through
ShellIntegration would get the proper suffix, but I just traced through and it
doesn't.

Proposal: leave this as is for now, to enable the dogfooding to commence, and
update to use a properly suffixed AppModelId after Sam's change lands.

[grt (UTC plus 2), 2012-11-01 23:18:56]

apologies for the delay. some comments below.

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1079: }
ubernit: comma at the end of this line

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:52: if
(current->HasSwitch(switches::kUserDataDir)) {
use command_line.CopySwitchesFrom(*current, ...) instead

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:298: const int
kArrowSize = 10;
just curious: why these changes?

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:371: void
DoCheckAppListTaskbarShortcut(const FilePath user_data_dir,
nit: i think there's a pseudo-convention of naming funcs like this
"BlahBlahOnFileThread".

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:409: const int
kIconIndex = 26;
is there a constant in a generated .h file that can be used here instead?

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:440:
content::BrowserThread::PostTask(
i think the blocking pool is appropriate here (see
BrowserThread::GetBlockingPool in
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OaA-9rnjOqI/H0cDC...)

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:442: FROM_HERE,
nit: move to previous line

[benwells, 2012-11-02 01:33:55]

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1079: }
On 2012/11/01 23:18:57, grt wrote:
> ubernit: comma at the end of this line

Done.

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:52: if
(current->HasSwitch(switches::kUserDataDir)) {
On 2012/11/01 23:18:57, grt wrote:
> use command_line.CopySwitchesFrom(*current, ...) instead

Done.

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:298: const int
kArrowSize = 10;
On 2012/11/01 23:18:57, grt wrote:
> just curious: why these changes?

I was asked in another change just to use const, not static const, in a similar
situation.

I got the impression it was a stylistic thing, I'm not aware of the practical
difference in this code. Is it allocated in a different section?

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:371: void
DoCheckAppListTaskbarShortcut(const FilePath user_data_dir,
On 2012/11/01 23:18:57, grt wrote:
> nit: i think there's a pseudo-convention of naming funcs like this
> "BlahBlahOnFileThread".

Done.

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:409: const int
kIconIndex = 26;
On 2012/11/01 23:18:57, grt wrote:
> is there a constant in a generated .h file that can be used here instead?

Not that I could find. See
chrome/installer/util/google_chrome_sxs_distribution.cc / GetIconIndex for a
similar case.

I would like to be proven wrong here though, I don't like this constant.
Annoyingly in a different API call windows accepts IDI_APP_LIST (which is
different).

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:440:
content::BrowserThread::PostTask(
On 2012/11/01 23:18:57, grt wrote:
> i think the blocking pool is appropriate here (see
> BrowserThread::GetBlockingPool in
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OaA-9rnjOqI/H0cDC...)

Is that safe? chrome/browser/shell_integration_win.cc MigrateWin7ShortcutsInPath
runs on the FILE thread and will mess with this icon.

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:442: FROM_HERE,
On 2012/11/01 23:18:57, grt wrote:
> nit: move to previous line

Done.

[grt (UTC plus 2), 2012-11-02 03:09:52]

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:398: if
(!PathService::Get(base::DIR_MODULE, &icon_path))
this is unsafe, see comment below.

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:409: const int
kIconIndex = 26;
On 2012/11/02 01:33:55, benwells wrote:
> On 2012/11/01 23:18:57, grt wrote:
> > is there a constant in a generated .h file that can be used here instead?
> 
> Not that I could find. See
> chrome/installer/util/google_chrome_sxs_distribution.cc / GetIconIndex for a
> similar case.
>
> I would like to be proven wrong here though, I don't like this constant.
> Annoyingly in a different API call windows accepts IDI_APP_LIST (which is
> different).

Since the path to chrome.dll changes on each update, it isn't safe to have
shortcuts fetch their icons from it. Rather, this icon should be duplicated in
chrome_exe.rc like IDR_MAINFRAME and friends. The icon path for the shortcut
should be the path to chrome.exe. I believe the icon index doesn't take the
resource identifiers into account, so I think you are stuck with making the
index either 5 (one more than the SxS index) or 1 (one more than the mainframe
index).

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:440:
content::BrowserThread::PostTask(
On 2012/11/02 01:33:55, benwells wrote:
> On 2012/11/01 23:18:57, grt wrote:
> > i think the blocking pool is appropriate here (see
> > BrowserThread::GetBlockingPool in
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OaA-9rnjOqI/H0cDC...)
> 
> Is that safe? chrome/browser/shell_integration_win.cc
MigrateWin7ShortcutsInPath
> runs on the FILE thread and will mess with this icon.

I could be mistaken, but at a high level this operation seems appropriate for
the blocking pool.   The comments around PostBlockingPoolTask in
browser_thread.h seem to agree.  File IO is certainly allowed there.  If there
are DCHECKs in shell_integration_win that require the FILE thread (are there?),
then perhaps those can be relaxed to allow operation in the blocking pool.

[gab, 2012-11-02 03:47:15]

If I understand correctly, this is a flag to be in chrome://flags (not a
checkbox in the UI -- I didn't read the whole CL). In that case this shortcut is
only temporary and for people with the flag, so it's fine to have the wrong
appid imo.

Some other comments inline below as I read some of the code (let me know if you
want a full review from me, but it looks like grt already went over it).

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integ...
File chrome/browser/shell_integration_win.cc (right):

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integ...
chrome/browser/shell_integration_win.cc:424: return
ShellIntegration::GetAppModelIdForProfile(kAppListAppName,
On 2012/11/01 22:59:08, benwells wrote:
> On 2012/11/01 22:18:11, gab wrote:
> > huangs@ is introducing something very similar in
> > http://codereview.chromium.org/11359013/, your appid should start from the
> > distribution's BaseAppId, then optionally (but I highly recommend it to
avoid
> > trouble later, e.g. say you ever want to support system-level app launcher
> > installs) add a per-user suffix, and finally add the profile part of the
> appid.
> > 
> > What I suggested to huangs@ and erikwright@ is that you modify
> > ShellUtil::GetBrowserModelId to be more generic (i.e. not browser only --
IIRC
> > might just need a name change...).
> 
> Yes I saw that CL this morning. I had assumed that going through
> ShellIntegration would get the proper suffix, but I just traced through and it
> doesn't.
> 
> Proposal: leave this as is for now, to enable the dogfooding to commence, and
> update to use a properly suffixed AppModelId after Sam's change lands.

The problem that you will run into however is that, although you can update the
shortcut saved in user_data_dir, you can't update the shortcut on the taskbar
itself (at least not easily); so if you prefer for it to be easy to keep
updating your dogfood users to stable without having shortcut issues (i.e.
appids dont match) then it's easier to have the right appid from the beginning.

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:411:
shortcut_properties.set_dual_mode(false);
You don't need to explicitly set dual_mode to false here.

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:435: string16
app_model_id = GetAppModelId();
nit: Also use () initialization here instead of '='.

[grt (UTC plus 2), 2012-11-02 04:00:35]

looks good.  my only remaining concern is with putting the icon in chrome.exe's
resources.

gab@: i only make a cursory pass over the shortcut operations.  could you give
them a thorough read to make sure it's all good?  thanks.

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:298: const int
kArrowSize = 10;
On 2012/11/02 01:33:55, benwells wrote:
> On 2012/11/01 23:18:57, grt wrote:
> > just curious: why these changes?
> 
> I was asked in another change just to use const, not static const, in a
similar
> situation.
> 
> I got the impression it was a stylistic thing, I'm not aware of the practical
> difference in this code. Is it allocated in a different section?

i think they're equivalent (the compiler should know that it doesn't really need
to store the constant anywhere since the address of it isn't taken).  i was just
wondering.  thanks.

http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:440:
content::BrowserThread::PostTask(
On 2012/11/02 03:09:52, grt wrote:
> On 2012/11/02 01:33:55, benwells wrote:
> > On 2012/11/01 23:18:57, grt wrote:
> > > i think the blocking pool is appropriate here (see
> > > BrowserThread::GetBlockingPool in
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OaA-9rnjOqI/H0cDC...)
> > 
> > Is that safe? chrome/browser/shell_integration_win.cc
> MigrateWin7ShortcutsInPath
> > runs on the FILE thread and will mess with this icon.
> 
> I could be mistaken, but at a high level this operation seems appropriate for
> the blocking pool.   The comments around PostBlockingPoolTask in
> browser_thread.h seem to agree.  File IO is certainly allowed there.  If there
> are DCHECKs in shell_integration_win that require the FILE thread (are
there?),
> then perhaps those can be relaxed to allow operation in the blocking pool.

After chatting, I now understand your concern.  I think you're right that it's a
good idea to keep this on the FILE thread.  Please add a comment here so that
future generations don't convert this to the blocking pool without considering
that other bits and piece of Chrome might be updating shortcuts on the FILE
thread.

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:49: const char*
kSwitchesToCopy[] = {switches::kUserDataDir};
"const char* const " so the whole thing is truly const and ends up in .rdata. 
also, i usually put spaces inside the braces ("{ switches::kUserDataDir }"), but
i don't know if the style guide says anything about this.

[gab, 2012-11-02 04:36:28]

lgtm for shortcuts w/ some comments below.

Cheers,
Gab

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:59: // The
AppModelId should be the same for all profiles in a user data directory
Are you sure this is what you want? i.e. you will never have per-profile app
launchers? There will be a single app-launcher handling profile inside the app?

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:367: void
CheckAppListTaskbarShortcutOnFileThread(const FilePath user_data_dir,
nit: Should this be const FilePath&?

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:374:
AddExtension(L"lnk"));
nit: put '.' on this line to make it more obvious the call is continued from
above.

http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:385:
base::win::ShortcutProperties shortcut_properties;
It will probably be easier to use the ShellUtil API huangs is currently
designing here, but since I believe this is temporary while waiting for that
anyways than I don't see a problem going straight to the base::win shortcut API.

[benwells, 2012-11-02 04:54:42]

https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:49: const char*
kSwitchesToCopy[] = {switches::kUserDataDir};
On 2012/11/02 04:00:36, grt wrote:
> "const char* const " so the whole thing is truly const and ends up in .rdata. 
> also, i usually put spaces inside the braces ("{ switches::kUserDataDir }"),
but
> i don't know if the style guide says anything about this.

Done.

https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:59: // The
AppModelId should be the same for all profiles in a user data directory
On 2012/11/02 04:36:28, gab wrote:
> Are you sure this is what you want? i.e. you will never have per-profile app
> launchers? There will be a single app-launcher handling profile inside the
app?

Yes that's correct. It will get profile switching eventually, at which point
maybe we'll need to figure out if we want shortcuts to start it with an initial
profile (note we don't have this for chrome itself yet).

https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:374:
AddExtension(L"lnk"));
On 2012/11/02 04:36:28, gab wrote:
> nit: put '.' on this line to make it more obvious the call is continued from
> above.

Done.

https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:385:
base::win::ShortcutProperties shortcut_properties;
On 2012/11/02 04:36:28, gab wrote:
> It will probably be easier to use the ShellUtil API huangs is currently
> designing here, but since I believe this is temporary while waiting for that
> anyways than I don't see a problem going straight to the base::win shortcut
API.

Yes this is temporary, and will be removed when that comes in and is all hooked
up.

https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:411:
shortcut_properties.set_dual_mode(false);
On 2012/11/02 03:47:15, gab wrote:
> You don't need to explicitly set dual_mode to false here.

Done.

https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:435: string16
app_model_id = GetAppModelId();
On 2012/11/02 03:47:15, gab wrote:
> nit: Also use () initialization here instead of '='.

Done.

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/app/chrome_e...
File chrome/app/chrome_exe.rc (right):

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/app/chrome_e...
chrome/app/chrome_exe.rc:48: // alphabetically after IDR_SXS.
This really did matter. Incredible!

[gab, 2012-11-02 12:37:27]

Still lgtm, some last comments below.

https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:367: void
CheckAppListTaskbarShortcutOnFileThread(const FilePath user_data_dir,
On 2012/11/02 04:36:28, gab wrote:
> nit: Should this be const FilePath&?

Ping.

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/browser/ui/v...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/browser/ui/v...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:377:
.AddExtension(L"lnk"));
optional: There is a kLnkExt constant in chrome\installer\util\util_constants.h
defined as L".lnk", I didn't know there was a method .AddExtension() on
FilePath; could be nice to change the constant to L"lnk" and change usages from
.Append(name.append(kLnkExt)) to .Append(name).AddExtension(kLnkExt), I've
always hated the previous paradigm with the mix of path appends and string
appends.

Feel free to disregard and add it to our fixit:
https://docs.google.com/a/google.com/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1fH...

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/browser/ui/v...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:398: return;
Add NOTREACHED() before return;.

[grt (UTC plus 2), 2012-11-02 19:39:38]

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/app/chrome_e...
File chrome/app/chrome_exe.rc (right):

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/app/chrome_e...
chrome/app/chrome_exe.rc:48: // alphabetically after IDR_SXS.
On 2012/11/02 04:54:43, benwells wrote:
> This really did matter. Incredible!

Sadly, these names (e.g., "IDR_MAINFRAME") are used as the indices in the
generated resource. I think this is a historical accident. 
chrome_dll_resource.h would need to be included to pick up the macros that map
the names to the numbers.  sigh.

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/browser/ui/v...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/browser/ui/v...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:323: ss << ",-" <<
IDR_APP_LIST;
i suspect this doesn't work; see my comment in the .rc file.

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/browser/ui/v...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:369: // threads
fiddle with the same threads could cause race issues.
"same threads" -> "same shortcuts"

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/browser/ui/v...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:404: // icon (SXS)
the icon for Google Chrome builds need to be one higher.
the "one higher" portion of this is out of date

https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/browser/ui/v...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:410:
shortcut_properties.set_icon(icon_path, kIconIndex);
icon_path -> chrome_exe and remove lines 396-399.

[benwells, 2012-11-03 00:54:18]

http://codereview.chromium.org/11367002/diff/7006/chrome/app/chrome_exe.rc
File chrome/app/chrome_exe.rc (right):

http://codereview.chromium.org/11367002/diff/7006/chrome/app/chrome_exe.rc#ne...
chrome/app/chrome_exe.rc:48: // alphabetically after IDR_SXS.
On 2012/11/02 19:39:38, grt wrote:
> On 2012/11/02 04:54:43, benwells wrote:
> > This really did matter. Incredible!
> 
> Sadly, these names (e.g., "IDR_MAINFRAME") are used as the indices in the
> generated resource. I think this is a historical accident. 
> chrome_dll_resource.h would need to be included to pick up the macros that map
> the names to the numbers.  sigh.

Ah, thanks for the explanation. This black magic is starting to make a little
more sense :)

http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:323: ss << ",-" <<
IDR_APP_LIST;
On 2012/11/02 19:39:38, grt wrote:
> i suspect this doesn't work; see my comment in the .rc file.

Thanks for that, it was broken. Fixed now, and tested. It needed to have the '-'
removed, contrary to the documentation
(http://msdn.microsoft.com/en-us/library/windows/desktop/dd391573(v=vs.85).aspx).
I guess .exe's are treated as .ico's for the purposes of this command. Is there
some logical explanation of that?

http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:369: // threads
fiddle with the same threads could cause race issues.
On 2012/11/02 19:39:38, grt wrote:
> "same threads" -> "same shortcuts"

Done.

http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:377:
.AddExtension(L"lnk"));
On 2012/11/02 12:37:28, gab wrote:
> optional: There is a kLnkExt constant in
chrome\installer\util\util_constants.h
> defined as L".lnk", I didn't know there was a method .AddExtension() on
> FilePath; could be nice to change the constant to L"lnk" and change usages
from
> .Append(name.append(kLnkExt)) to .Append(name).AddExtension(kLnkExt), I've
> always hated the previous paradigm with the mix of path appends and string
> appends.
> 
> Feel free to disregard and add it to our fixit:
>
https://docs.google.com/a/google.com/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1fH...

If it wasn't Saturday maybe I'd do it, I'll just add it to the fixit for now :)
Or I would if I had edit / comment.

http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:398: return;
On 2012/11/02 12:37:28, gab wrote:
> Add NOTREACHED() before return;.

Done.

http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:404: // icon (SXS)
the icon for Google Chrome builds need to be one higher.
On 2012/11/02 19:39:38, grt wrote:
> the "one higher" portion of this is out of date

Done.

http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:410:
shortcut_properties.set_icon(icon_path, kIconIndex);
On 2012/11/02 19:39:38, grt wrote:
> icon_path -> chrome_exe and remove lines 396-399.

Done.

[gab, 2012-11-04 16:50:33]

http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:377:
.AddExtension(L"lnk"));
On 2012/11/03 00:54:18, benwells wrote:
> On 2012/11/02 12:37:28, gab wrote:
> > optional: There is a kLnkExt constant in
> chrome\installer\util\util_constants.h
> > defined as L".lnk", I didn't know there was a method .AddExtension() on
> > FilePath; could be nice to change the constant to L"lnk" and change usages
> from
> > .Append(name.append(kLnkExt)) to .Append(name).AddExtension(kLnkExt), I've
> > always hated the previous paradigm with the mix of path appends and string
> > appends.
> > 
> > Feel free to disregard and add it to our fixit:
> >
>
https://docs.google.com/a/google.com/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1fH...
> 
> If it wasn't Saturday maybe I'd do it, I'll just add it to the fixit for now
:)
> Or I would if I had edit / comment.

It's open to "anyone at google.com" maybe I should make it "anyone with the
link"..?

[benwells, 2012-11-04 22:35:37]

On 2012/11/04 16:50:33, gab wrote:
>
http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
> File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):
> 
>
http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
> chrome/browser/ui/views/app_list/app_list_controller_win.cc:377:
> .AddExtension(L"lnk"));
> On 2012/11/03 00:54:18, benwells wrote:
> > On 2012/11/02 12:37:28, gab wrote:
> > > optional: There is a kLnkExt constant in
> > chrome\installer\util\util_constants.h
> > > defined as L".lnk", I didn't know there was a method .AddExtension() on
> > > FilePath; could be nice to change the constant to L"lnk" and change usages
> > from
> > > .Append(name.append(kLnkExt)) to .Append(name).AddExtension(kLnkExt), I've
> > > always hated the previous paradigm with the mix of path appends and string
> > > appends.
> > > 
> > > Feel free to disregard and add it to our fixit:
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1fH...
> > 
> > If it wasn't Saturday maybe I'd do it, I'll just add it to the fixit for now
> :)
> > Or I would if I had edit / comment.
> 
> It's open to "anyone at google.com" maybe I should make it "anyone with the
> link"..?

Either should work but you might need to change the bit about 'can view' to 'can
comment' or 'can edit'

[benwells, 2012-11-04 22:35:40]

On 2012/11/04 16:50:33, gab wrote:
>
http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
> File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):
> 
>
http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
> chrome/browser/ui/views/app_list/app_list_controller_win.cc:377:
> .AddExtension(L"lnk"));
> On 2012/11/03 00:54:18, benwells wrote:
> > On 2012/11/02 12:37:28, gab wrote:
> > > optional: There is a kLnkExt constant in
> > chrome\installer\util\util_constants.h
> > > defined as L".lnk", I didn't know there was a method .AddExtension() on
> > > FilePath; could be nice to change the constant to L"lnk" and change usages
> > from
> > > .Append(name.append(kLnkExt)) to .Append(name).AddExtension(kLnkExt), I've
> > > always hated the previous paradigm with the mix of path appends and string
> > > appends.
> > > 
> > > Feel free to disregard and add it to our fixit:
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1fH...
> > 
> > If it wasn't Saturday maybe I'd do it, I'll just add it to the fixit for now
> :)
> > Or I would if I had edit / comment.
> 
> It's open to "anyone at google.com" maybe I should make it "anyone with the
> link"..?

Either should work but you might need to change the bit about 'can view' to 'can
comment' or 'can edit'

[gab, 2012-11-05 14:22:03]

On 2012/11/04 22:35:40, benwells wrote:
> On 2012/11/04 16:50:33, gab wrote:
> >
>
http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
> > File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):
> > 
> >
>
http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app...
> > chrome/browser/ui/views/app_list/app_list_controller_win.cc:377:
> > .AddExtension(L"lnk"));
> > On 2012/11/03 00:54:18, benwells wrote:
> > > On 2012/11/02 12:37:28, gab wrote:
> > > > optional: There is a kLnkExt constant in
> > > chrome\installer\util\util_constants.h
> > > > defined as L".lnk", I didn't know there was a method .AddExtension() on
> > > > FilePath; could be nice to change the constant to L"lnk" and change
usages
> > > from
> > > > .Append(name.append(kLnkExt)) to .Append(name).AddExtension(kLnkExt),
I've
> > > > always hated the previous paradigm with the mix of path appends and
string
> > > > appends.
> > > > 
> > > > Feel free to disregard and add it to our fixit:
> > > >
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1fH...
> > > 
> > > If it wasn't Saturday maybe I'd do it, I'll just add it to the fixit for
now
> > :)
> > > Or I would if I had edit / comment.
> > 
> > It's open to "anyone at google.com" maybe I should make it "anyone with the
> > link"..?
> 
> Either should work but you might need to change the bit about 'can view' to
'can
> comment' or 'can edit'

Duh, my bad, "everyone at google.com" can edit now :).

Cheers,
Gab

[grt (UTC plus 2), 2012-11-05 19:02:51]

lgtm

http://codereview.chromium.org/11367002/diff/5018/chrome/app/chrome_exe.rc
File chrome/app/chrome_exe.rc (right):

http://codereview.chromium.org/11367002/diff/5018/chrome/app/chrome_exe.rc#ne...
chrome/app/chrome_exe.rc:48: // alphabetically after IDR_SXS.
consider noting in the comment that the index of this icon must not change.  the
one in app_list_controller_win.cc is good, but it'd be bad if an enterprising
soul were to attempt a cleanup in this file that silently breaks the use of the
icon.

[benwells, 2012-11-06 01:15:03]

http://codereview.chromium.org/11367002/diff/5018/chrome/app/chrome_exe.rc
File chrome/app/chrome_exe.rc (right):

http://codereview.chromium.org/11367002/diff/5018/chrome/app/chrome_exe.rc#ne...
chrome/app/chrome_exe.rc:48: // alphabetically after IDR_SXS.
On 2012/11/05 19:02:51, grt wrote:
> consider noting in the comment that the index of this icon must not change. 
the
> one in app_list_controller_win.cc is good, but it'd be bad if an enterprising
> soul were to attempt a cleanup in this file that silently breaks the use of
the
> icon.

Done.

[commit-bot: I haz the power, 2012-11-06 01:15:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11367002/14019

[commit-bot: I haz the power, 2012-11-06 04:35:53]

Change committed as 166144

[Nico, 2017-01-25 19:43:56]

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

[Nico, 2017-01-25 19:43:57]

https://codereview.chromium.org/11367002/diff/14019/chrome/app/chrome_exe.rc
File chrome/app/chrome_exe.rc (right):

https://codereview.chromium.org/11367002/diff/14019/chrome/app/chrome_exe.rc#...
chrome/app/chrome_exe.rc:53: IDR_X_APP_LIST          ICON                   
"theme\\app_list.ico"
Is this still used? I noticed that the ICON is still here but nothing seems to
reference it.

[tapted, 2017-01-27 00:55:53]

On 2017/01/25 19:43:57, Nico wrote:
> https://codereview.chromium.org/11367002/diff/14019/chrome/app/chrome_exe.rc
> File chrome/app/chrome_exe.rc (right):
> 
>
https://codereview.chromium.org/11367002/diff/14019/chrome/app/chrome_exe.rc#...
> chrome/app/chrome_exe.rc:53: IDR_X_APP_LIST          ICON                   
> "theme\\app_list.ico"
> Is this still used? I noticed that the ICON is still here but nothing seems to
> reference it.

Shortcuts that exist on users' machines may still reference it. So simply
removing the resource may break the appearance of those shortcuts, or make the
icon fallback to the Chrome logo - neither of which are great options.
http://crbug.com/614575 is about hunting for such shortcuts on users machines
and deleting them.