Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(847)

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

Created:
8 years, 1 month ago by benwells
Modified:
3 years, 11 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

Messages

Total messages: 27 (1 generated)
benwells
sky@ for overall grt@ for windows shortcut related changes
8 years, 1 month 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 ...
8 years, 1 month ago (2012-10-31 14:20:52 UTC) #2
benwells
Also updated: - use chrome / chromium specific strings for the shortcut file name and ...
8 years, 1 month ago (2012-11-01 05:31:05 UTC) #3
sky
LGTM
8 years, 1 month 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 ...
8 years, 1 month 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@ ...
8 years, 1 month 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: ...
8 years, 1 month 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 ...
8 years, 1 month 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. ...
8 years, 1 month 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 ...
8 years, 1 month 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@: ...
8 years, 1 month 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: ...
8 years, 1 month 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 ...
8 years, 1 month 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 ...
8 years, 1 month 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: ...
8 years, 1 month 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: ...
8 years, 1 month 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 ...
8 years, 1 month 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 > ...
8 years, 1 month 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 > ...
8 years, 1 month 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 ...
8 years, 1 month 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 ...
8 years, 1 month 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: ...
8 years, 1 month 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
8 years, 1 month ago (2012-11-06 01:15:17 UTC) #23
commit-bot: I haz the power
Change committed as 166144
8 years, 1 month 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 years, 11 months ago (2017-01-25 19:43:57 UTC) #26
tapted
3 years, 11 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.

Powered by Google App Engine
This is Rietveld 408576698