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

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:
1 year, 5 months ago by benwells
Modified:
1 year, 5 months ago
Reviewers:
gab, sky, grt
CC:
chromium-reviews_chromium.org, tfarina, koz, tapted, jeremya, erikwright, huangs1
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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+176 lines, -41 lines) Lint Patch
M chrome/app/chrome_dll.rc View 1 2 3 4 5 1 chunk +0 lines, -4 lines 0 comments ? errors Download
M chrome/app/chrome_dll_resource.h View 1 2 3 4 5 6 1 chunk +0 lines, -1 line 0 comments 0 errors Download
M chrome/app/chrome_exe.rc View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments ? errors Download
M chrome/app/chromium_strings.grd View 1 2 3 2 chunks +11 lines, -0 lines 0 comments ? errors Download
M chrome/app/generated_resources.grd View 2 chunks +0 lines, -4 lines 0 comments ? errors Download
M chrome/app/google_chrome_strings.grd View 1 2 3 2 chunks +11 lines, -0 lines 0 comments ? errors Download
M chrome/browser/about_flags.cc View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments 1 errors Download
M chrome/browser/shell_integration.h View 1 1 chunk +3 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/shell_integration_win.cc View 1 2 3 3 chunks +14 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/ui/app_list/app_list_controller.h View 1 chunk +7 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/ui/app_list/app_list_controller.cc View 1 chunk +3 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/ui/startup/startup_browser_creator_impl.cc View 1 chunk +1 line, -0 lines 0 comments 0 errors 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 ? errors Download
M chrome/common/chrome_switches.h View 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M chrome/common/chrome_switches.cc View 1 chunk +3 lines, -0 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 24
benwells
sky@ for overall grt@ for windows shortcut related changes
1 year, 5 months ago #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 ...
1 year, 5 months ago #2
benwells
Also updated: - use chrome / chromium specific strings for the shortcut file name and ...
1 year, 5 months ago #3
sky
LGTM
1 year, 5 months ago #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 ...
1 year, 5 months ago #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@ ...
1 year, 5 months ago #6
grt
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: ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #8
grt
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. ...
1 year, 5 months ago #9
gab
If I understand correctly, this is a flag to be in chrome://flags (not a checkbox ...
1 year, 5 months ago #10
grt
looks good. my only remaining concern is with putting the icon in chrome.exe's resources. gab@: ...
1 year, 5 months ago #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: ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #14
grt
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: ...
1 year, 5 months ago #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: ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 > ...
1 year, 5 months ago #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 > ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #20
grt
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 ...
1 year, 5 months ago #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: ...
1 year, 5 months ago #22
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/11367002/14019
1 year, 5 months ago #23
I haz the power (commit-bot)
1 year, 5 months ago #24
Change committed as 166144
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6