Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 11367002: Add a flag to control whether there is a shortcut for the app list / launcher in the Windows taskba… (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 6 months ago by benwells
Modified:
3 months ago
Reviewers:
gab, Nico, sky, grt (UTC plus 2)
CC:
chromium-reviews, tfarina, koz (OOO until 15th September), jeremya, erikwright (departed), huangs
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Cleanup / compile #

Patch Set 3 : woops #

Total comments: 8

Patch Set 4 : Feedback #

Total comments: 22

Patch Set 5 : Feedback #

Total comments: 14

Patch Set 6 : Next round #

Total comments: 16

Patch Set 7 : Moar #

Total comments: 2

Patch Set 8 : Added comment #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+176 lines, -41 lines) Patch
M chrome/app/chrome_dll.rc View 1 2 3 4 5 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/app/chrome_dll_resource.h View 1 2 3 4 5 6 1 chunk +0 lines, -1 line 0 comments Download
M chrome/app/chrome_exe.rc View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 1 comment Download
M chrome/app/chromium_strings.grd View 1 2 3 2 chunks +11 lines, -0 lines 0 comments Download
M chrome/app/generated_resources.grd View 2 chunks +0 lines, -4 lines 0 comments Download
M chrome/app/google_chrome_strings.grd View 1 2 3 2 chunks +11 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/shell_integration.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/shell_integration_win.cc View 1 2 3 3 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/ui/app_list/app_list_controller.h View 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/ui/app_list/app_list_controller.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/startup/startup_browser_creator_impl.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/views/app_list/app_list_controller_win.cc View 1 2 3 4 5 6 8 chunks +108 lines, -32 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 chunk +3 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 27 (1 generated)
benwells
sky@ for overall grt@ for windows shortcut related changes
4 years, 6 months ago (2012-10-31 08:39:29 UTC) #1
sky
http://codereview.chromium.org/11367002/diff/9002/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/11367002/diff/9002/chrome/app/generated_resources.grd#newcode6730 chrome/app/generated_resources.grd:6730: + <message name="IDS_FLAGS_SHOW_APP_LIST_SHORTCUT_NAME" desc="Name for the flag to show ...
4 years, 6 months ago (2012-10-31 14:20:52 UTC) #2
benwells
Also updated: - use chrome / chromium specific strings for the shortcut file name and ...
4 years, 6 months ago (2012-11-01 05:31:05 UTC) #3
sky
LGTM
4 years, 6 months ago (2012-11-01 16:11:39 UTC) #4
gab
General comment for now. http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integration_win.cc File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integration_win.cc#newcode424 chrome/browser/shell_integration_win.cc:424: return ShellIntegration::GetAppModelIdForProfile(kAppListAppName, huangs@ is introducing ...
4 years, 6 months ago (2012-11-01 22:18:11 UTC) #5
benwells
http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integration_win.cc File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/11367002/diff/11005/chrome/browser/shell_integration_win.cc#newcode424 chrome/browser/shell_integration_win.cc:424: return ShellIntegration::GetAppModelIdForProfile(kAppListAppName, On 2012/11/01 22:18:11, gab wrote: > huangs@ ...
4 years, 6 months ago (2012-11-01 22:59:08 UTC) #6
grt (UTC plus 2)
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.cc#newcode1079 chrome/browser/about_flags.cc:1079: } ubernit: ...
4 years, 6 months ago (2012-11-01 23:18:56 UTC) #7
benwells
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.cc#newcode1079 chrome/browser/about_flags.cc:1079: } On 2012/11/01 23:18:57, grt wrote: > ubernit: comma ...
4 years, 6 months ago (2012-11-02 01:33:55 UTC) #8
grt (UTC plus 2)
http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/app_list/app_list_controller_win.cc File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): http://codereview.chromium.org/11367002/diff/11005/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode398 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. ...
4 years, 6 months ago (2012-11-02 03:09:52 UTC) #9
gab
If I understand correctly, this is a flag to be in chrome://flags (not a checkbox ...
4 years, 6 months ago (2012-11-02 03:47:15 UTC) #10
grt (UTC plus 2)
looks good. my only remaining concern is with putting the icon in chrome.exe's resources. gab@: ...
4 years, 6 months ago (2012-11-02 04:00:35 UTC) #11
gab
lgtm for shortcuts w/ some comments below. Cheers, Gab http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/app_list/app_list_controller_win.cc File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): http://codereview.chromium.org/11367002/diff/10002/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode59 chrome/browser/ui/views/app_list/app_list_controller_win.cc:59: ...
4 years, 6 months ago (2012-11-02 04:36:28 UTC) #12
benwells
https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/views/app_list/app_list_controller_win.cc File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode49 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 ...
4 years, 6 months ago (2012-11-02 04:54:42 UTC) #13
gab
Still lgtm, some last comments below. https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/views/app_list/app_list_controller_win.cc File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://chromiumcodereview.appspot.com/11367002/diff/10002/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode367 chrome/browser/ui/views/app_list/app_list_controller_win.cc:367: void CheckAppListTaskbarShortcutOnFileThread(const FilePath ...
4 years, 5 months ago (2012-11-02 12:37:27 UTC) #14
grt (UTC plus 2)
https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/app/chrome_exe.rc File chrome/app/chrome_exe.rc (right): https://chromiumcodereview.appspot.com/11367002/diff/7006/chrome/app/chrome_exe.rc#newcode48 chrome/app/chrome_exe.rc:48: // alphabetically after IDR_SXS. On 2012/11/02 04:54:43, benwells wrote: ...
4 years, 5 months ago (2012-11-02 19:39:38 UTC) #15
benwells
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#newcode48 chrome/app/chrome_exe.rc:48: // alphabetically after IDR_SXS. On 2012/11/02 19:39:38, grt wrote: ...
4 years, 5 months ago (2012-11-03 00:54:18 UTC) #16
gab
http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app_list/app_list_controller_win.cc 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_list/app_list_controller_win.cc#newcode377 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 ...
4 years, 5 months ago (2012-11-04 16:50:33 UTC) #17
benwells
On 2012/11/04 16:50:33, gab wrote: > http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app_list/app_list_controller_win.cc > 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_list/app_list_controller_win.cc#newcode377 > ...
4 years, 5 months ago (2012-11-04 22:35:37 UTC) #18
benwells
On 2012/11/04 16:50:33, gab wrote: > http://codereview.chromium.org/11367002/diff/7006/chrome/browser/ui/views/app_list/app_list_controller_win.cc > 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_list/app_list_controller_win.cc#newcode377 > ...
4 years, 5 months ago (2012-11-04 22:35:40 UTC) #19
gab
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_list/app_list_controller_win.cc ...
4 years, 5 months ago (2012-11-05 14:22:03 UTC) #20
grt (UTC plus 2)
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#newcode48 chrome/app/chrome_exe.rc:48: // alphabetically after IDR_SXS. consider noting in the ...
4 years, 5 months ago (2012-11-05 19:02:51 UTC) #21
benwells
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#newcode48 chrome/app/chrome_exe.rc:48: // alphabetically after IDR_SXS. On 2012/11/05 19:02:51, grt wrote: ...
4 years, 5 months ago (2012-11-06 01:15:03 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/11367002/14019
4 years, 5 months ago (2012-11-06 01:15:17 UTC) #23
commit-bot: I haz the power
Change committed as 166144
4 years, 5 months ago (2012-11-06 04:35:53 UTC) #24
Nico
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#newcode53 chrome/app/chrome_exe.rc:53: IDR_X_APP_LIST ICON "theme\\app_list.ico" Is this still used? I noticed ...
3 months ago (2017-01-25 19:43:57 UTC) #26
tapted (OOO until 2017-05-01)
3 months ago (2017-01-27 00:55:53 UTC) #27
Message was sent while issue was closed.
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46