Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11359133:
Add default icon to app_host.exe, and use it in shortcuts during installation. (Closed)
|
Created:
8 years, 1 month ago by huangs Modified:
8 years, 1 month ago CC:
chromium-reviews, Aaron Boodman, grt+watch_chromium.org, chromium-apps-reviews_chromium.org, benwells Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd default icon to app_host.exe, and use it in shortcuts during installation.
BUG=151626
TBR=ben@
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168722
Patch Set 1 #
Total comments: 7
Patch Set 2 : Clean up to get Program and Features uninstall icon to work more generally. #
Total comments: 29
Patch Set 3 : Renaming and cleanups. #
Total comments: 2
Patch Set 4 : Fix .gypi for new file. #
Total comments: 27
Patch Set 5 : More renames and comments. #
Total comments: 8
Patch Set 6 : Fixing comments. #
Total comments: 4
Patch Set 7 : Small comment change. #
Total comments: 6
Patch Set 8 : Comment fixes to .rc file, and prefixing icon id with IDI_. #
Total comments: 8
Patch Set 9 : Fixing comments; renaming. #
Total comments: 2
Patch Set 10 : Renaming GetChromiumIconString() => GetChromiumIconLocation(). #Messages
Total messages: 42 (0 generated)
The icon is also found in chrome.exe (index 5), which is used by app_list_control_win.cc . Right now we're not touching that. PTAL.
I don't know much about resources, +grt Thanks, Gab http://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_a... File chrome/installer/util/chrome_app_host_distribution.cc (right): http://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_a... chrome/installer/util/chrome_app_host_distribution.cc:138: return 0; Same icon for Chrome and Chromium builds?!
http://codereview.chromium.org/11359133/diff/1/chrome/browser/extensions/app_... File chrome/browser/extensions/app_host/app_host.rc (right): http://codereview.chromium.org/11359133/diff/1/chrome/browser/extensions/app_... chrome/browser/extensions/app_host/app_host.rc:33: // Note: chrome/installer/util/shell_util.cc depends on the order and number of does the shell_util.cc comment apply to this, or only to chrome?
Now doing this properly by introducing and using BrowserDistribution::GetIconIndex(). PTAL. https://codereview.chromium.org/11359133/diff/1/chrome/browser/extensions/app... File chrome/browser/extensions/app_host/app_host.rc (right): https://codereview.chromium.org/11359133/diff/1/chrome/browser/extensions/app... chrome/browser/extensions/app_host/app_host.rc:33: // Note: chrome/installer/util/shell_util.cc depends on the order and number of On 2012/11/09 21:46:25, erikwright wrote: > does the shell_util.cc comment apply to this, or only to chrome? This should apply in general; icon index 0 is the main application icon. https://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_... chrome/installer/util/chrome_app_host_distribution.cc:138: return 0; On 2012/11/09 19:29:28, gab wrote: > Same icon for Chrome and Chromium builds?! Yes; this was the previous behaviour. However, I'm reamoving this routine because the base class returns 0 anyway.
(removing myself from reviewer list, I'll defer to erikwright@ and grt@) https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/app_host/app_host.rc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:40: Nit: extra blank line
Initial comments. @grt: I only need you to take a look at the .rc file, feel free to defer the rest of the review to me. Thanks, Gab https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:438: app_path.value()); If this call is going to be used this way (i.e. to get non-Chrome icons); it should at least by renamed to something more generic. I also feel it shouldn't take a distribution just to get the icon index out of it, taking the icon index directly makes more sense to me. Maybe something like "string16 GetResourcePath(FilePath resource_path, int resource_index);" since this applies to more than just icons? https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... chrome/installer/util/browser_distribution.cc:233: // Assuming that IDR_MAINFRAME appears first alphabetically. Thanks for this comment, now the .rc file makes more sense to me :). https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:124: virtual string16 GetIconExe(); I don't really like this new call, I feel if this existed it should return a full path and since me and grt want to move towards a BrowserDistribution class that only returns constants I don't like adding this. The icon index itself is a constant so that's different, the path to the exe is built differently depending on the state the caller has and I think it should remain the caller's responsibility to build that path. At best this could return a constant relative path to the install root, but this easily becomes confusing to read at the call site imo (I'm thinking system-level installs where User Data and Application aren't in the same folder) and some path logic needs to be applied by the caller anyways so I don't really see the benefit (not even mentioning that the caller will have to make sure the path they are adding the relative path to is at the correct depth for this relative path to add up... meh). https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_distribution.cc (left): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:142: int ChromeAppHostDistribution::GetIconIndex() { Although you use the default in browser_distribution.cc, I think I'm still in favor of overriding GetIconIndex() here, makes it more readable and easier to change later; also isn't there a different icon for Chromium vs Chrome AppLauncher. Same comment applies for other distribution where you removed the override.
gab: PTAL. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:627: dist, install_path.Append(dist->GetIconExe()).value()); gab: This callsite is the main motivation for the new method on BrowserDistribution. Other options: 1) Add it to Product/ProductOperations instead. 2) Add an 'if (app_launcher) { ... } else { // chrome }' block here https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:124: virtual string16 GetIconExe(); On 2012/11/13 14:15:12, gab wrote: > I don't really like this new call, I feel if this existed it should return a > full path and since me and grt want to move towards a BrowserDistribution class > that only returns constants I don't like adding this. > > The icon index itself is a constant so that's different, the path to the exe is > built differently depending on the state the caller has and I think it should > remain the caller's responsibility to build that path. Which .exe the icon is in is also constant. > At best this could return a constant relative path to the install root, but this > easily becomes confusing to read at the call site imo (I'm thinking system-level > installs where User Data and Application aren't in the same folder) and some > path logic needs to be applied by the caller anyways so I don't really see the > benefit (not even mentioning that the caller will have to make sure the path > they are adding the relative path to is at the correct depth for this relative > path to add up... meh). The question is whether the caller has to apply a single path logic for all cases or whether the caller needs to have an if/else block checking specific products. See my other comment in this CL for alternatives.
i don't see a problem with putting the name of the file that holds the dist's icon(s) in BrowserDistribution. feel free to tap me on the shoulder to discuss. https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/app_host/app_host.rc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:38: IDR_MAINFRAME ICON "..\\..\\..\\app\\theme\\app_list.ico" the first field here can either be a name or a uint16 identifier. in the case of chrome_dll.rc, IDR_MAINFRAME is a macro that's given the value 101. that isn't true in this file, so it ends up being the name of the resource. there is no good reason to do this. i think resource ordering will be more predictable if you use uint16s. with that in mind, i suggest you make an app_host_resources.h file that #defines SOMETHING 101 and then use that SOMETHING here. please choose a SOMETHING that's unique to the app host (don't overload IDR_MAINFRAME any more than it already is). https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:38: IDR_MAINFRAME ICON "..\\..\\..\\app\\theme\\app_list.ico" also, i don't think you need the double-backslashes here. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:626: string16 chrome_icon = ShellUtil::GetChromeIcon( why not just ShellUtil::GetChromeIcon(dist).value()? GetChromeIcon should call both GetIconFile and getIconIndex. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:124: virtual string16 GetIconExe(); GetIconExe -> GetIconFile or somesuch, since icons can be in any PE Image file (.exe or .dll or whatever extension you want). also, please please please don't follow the terrible precedent of not documenting methods in this file. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_operations.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_operations.cc:131: properties->set_icon(target_exe, dist->GetIconIndex()); target_exe -> dist->GetIconFile()
On Tue, Nov 13, 2012 at 11:19 AM, <grt@chromium.org> wrote: > i don't see a problem with putting the name of the file that holds the > dist's > icon(s) in BrowserDistribution. feel free to tap me on the shoulder to > discuss. grt: should it be a file name (assumed to be in 'Application/') or a path relative to the install directory? It's hard to imagine a scenario where the file pointed to wouldn't be in the Application/ directory at the moment. > > > https://codereview.chromium.**org/11359133/diff/6001/chrome/** > browser/extensions/app_host/**app_host.rc<https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/app_host/app_host.rc> > File chrome/browser/extensions/app_**host/app_host.rc (right): > > https://codereview.chromium.**org/11359133/diff/6001/chrome/** > browser/extensions/app_host/**app_host.rc#newcode38<https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/app_host/app_host.rc#newcode38> > chrome/browser/extensions/app_**host/app_host.rc:38: IDR_MAINFRAME ICON > > "..\\..\\..\\app\\theme\\app_**list.ico" > the first field here can either be a name or a uint16 identifier. in > the case of chrome_dll.rc, IDR_MAINFRAME is a macro that's given the > value 101. that isn't true in this file, so it ends up being the name > of the resource. there is no good reason to do this. i think resource > ordering will be more predictable if you use uint16s. with that in > mind, i suggest you make an app_host_resources.h file that #defines > SOMETHING 101 and then use that SOMETHING here. please choose a > SOMETHING that's unique to the app host (don't overload IDR_MAINFRAME > any more than it already is). > > https://codereview.chromium.**org/11359133/diff/6001/chrome/** > browser/extensions/app_host/**app_host.rc#newcode38<https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/app_host/app_host.rc#newcode38> > chrome/browser/extensions/app_**host/app_host.rc:38: IDR_MAINFRAME ICON > > "..\\..\\..\\app\\theme\\app_**list.ico" > also, i don't think you need the double-backslashes here. > > > https://codereview.chromium.**org/11359133/diff/6001/chrome/** > installer/setup/install_**worker.cc<https://codereview.chromium.org/11359133/diff/6001/chrome/installer/setup/install_worker.cc> > File chrome/installer/setup/**install_worker.cc (right): > > https://codereview.chromium.**org/11359133/diff/6001/chrome/** > installer/setup/install_**worker.cc#newcode626<https://codereview.chromium.org/11359133/diff/6001/chrome/installer/setup/install_worker.cc#newcode626> > chrome/installer/setup/**install_worker.cc:626: string16 chrome_icon = > ShellUtil::GetChromeIcon( > why not just ShellUtil::GetChromeIcon(dist)**.value()? GetChromeIcon > should call both GetIconFile and getIconIndex. > > > https://codereview.chromium.**org/11359133/diff/6001/chrome/** > installer/util/browser_**distribution.h<https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/browser_distribution.h> > File chrome/installer/util/browser_**distribution.h (right): > > https://codereview.chromium.**org/11359133/diff/6001/chrome/** > installer/util/browser_**distribution.h#newcode124<https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/browser_distribution.h#newcode124> > chrome/installer/util/browser_**distribution.h:124: virtual string16 > GetIconExe(); > GetIconExe -> GetIconFile or somesuch, since icons can be in any PE > Image file (.exe or .dll or whatever extension you want). > > also, please please please don't follow the terrible precedent of not > documenting methods in this file. > > https://codereview.chromium.**org/11359133/diff/6001/chrome/** > installer/util/chrome_app_**host_operations.cc<https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chrome_app_host_operations.cc> > File chrome/installer/util/chrome_**app_host_operations.cc (right): > > https://codereview.chromium.**org/11359133/diff/6001/chrome/** > installer/util/chrome_app_**host_operations.cc#newcode131<https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chrome_app_host_operations.cc#newcode131> > chrome/installer/util/chrome_**app_host_operations.cc:131: > properties->set_icon(target_**exe, dist->GetIconIndex()); > target_exe -> dist->GetIconFile() > > https://codereview.chromium.**org/11359133/<https://codereview.chromium.org/1... >
On 2012/11/13 16:24:43, erikwright wrote: > On Tue, Nov 13, 2012 at 11:19 AM, <mailto:grt@chromium.org> wrote: > > > i don't see a problem with putting the name of the file that holds the > > dist's > > icon(s) in BrowserDistribution. feel free to tap me on the shoulder to > > discuss. > > > grt: should it be a file name (assumed to be in 'Application/') or a path > relative to the install directory? It's hard to imagine a scenario where > the file pointed to wouldn't be in the Application/ directory at the moment. Putting the icon file(s) in the version directory would be problematic due to updates. I think the Application dir is the right place, although this means that anything hosting icons needs to be rolled on updates like chrome.exe is.
PTAL. https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/app_host/app_host.rc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:38: IDR_MAINFRAME ICON "..\\..\\..\\app\\theme\\app_list.ico" On 2012/11/13 16:19:32, grt wrote: > the first field here can either be a name or a uint16 identifier. in the case > of chrome_dll.rc, IDR_MAINFRAME is a macro that's given the value 101. that > isn't true in this file, so it ends up being the name of the resource. there is > no good reason to do this. i think resource ordering will be more predictable > if you use uint16s. with that in mind, i suggest you make an > app_host_resources.h file that #defines SOMETHING 101 and then use that > SOMETHING here. please choose a SOMETHING that's unique to the app host (don't > overload IDR_MAINFRAME any more than it already is). Made the .h file and set name as IDR_APP_HOST_MAIN. https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:38: IDR_MAINFRAME ICON "..\\..\\..\\app\\theme\\app_list.ico" On 2012/11/13 16:19:32, grt wrote: > also, i don't think you need the double-backslashes here. The double-backslashes essential. single-backslash worked for chrome_dll.rc because \c, \g, and \v happened to be escape characters in C-style string... Meanwhile, \a and \t are, and using single-backslash there breaks the build. https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:40: On 2012/11/13 05:52:56, benwells wrote: > Nit: extra blank line Done. https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:438: app_path.value()); On 2012/11/13 14:15:12, gab wrote: > If this call is going to be used this way (i.e. to get non-Chrome icons); it > should at least by renamed to something more generic. > > I also feel it shouldn't take a distribution just to get the icon index out of > it, taking the icon index directly makes more sense to me. > > Maybe something like "string16 GetResourcePath(FilePath resource_path, int > resource_index);" since this applies to more than just icons? Done, calling it GetResourceString() per F2F. Also, passing resource_path as a string instead of FilePath, since many callers only have chrome_exe as string. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:626: string16 chrome_icon = ShellUtil::GetChromeIcon( This would be nice, but there are flows where chrome_exe is obtained from the current process, and is then passed to this routine to get icons. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:627: dist, install_path.Append(dist->GetIconExe()).value()); Pending decision. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... chrome/installer/util/browser_distribution.cc:233: // Assuming that IDR_MAINFRAME appears first alphabetically. On 2012/11/13 14:15:12, gab wrote: > Thanks for this comment, now the .rc file makes more sense to me :). Since IDR_MAINFRAME is not true anymore, so changing comments. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:124: virtual string16 GetIconExe(); I would prefer to have a method to get the full path. However, chrome_exe is being passed all over the place in ShellUtils, so some refactor is needed. What I have is a compromise to avoid changing too many things. Pending decisions. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:124: virtual string16 GetIconExe(); Renamed to GetIconFile(), and added comments. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_distribution.cc (left): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_distribution.cc:142: int ChromeAppHostDistribution::GetIconIndex() { On 2012/11/13 14:15:12, gab wrote: > Although you use the default in browser_distribution.cc, I think I'm still in > favor of overriding GetIconIndex() here, makes it more readable and easier to > change later; also isn't there a different icon for Chromium vs Chrome > AppLauncher. > > Same comment applies for other distribution where you removed the override. Done, but this is the only distribution where the override was removed. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_operations.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_operations.cc:131: properties->set_icon(target_exe, dist->GetIconIndex()); On 2012/11/13 16:19:32, grt wrote: > target_exe -> dist->GetIconFile() Cannot do this, since target_exe has the full path, but GetIconFile() only returns the bare executable name!
https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/app_host/app_host.rc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:38: IDR_MAINFRAME ICON "..\\..\\..\\app\\theme\\app_list.ico" On 2012/11/13 20:41:54, huangs1 wrote: > On 2012/11/13 16:19:32, grt wrote: > > also, i don't think you need the double-backslashes here. > > The double-backslashes essential. single-backslash worked for chrome_dll.rc > because \c, \g, and \v happened to be escape characters in C-style string... > Meanwhile, \a and \t are, and using single-backslash there breaks the build. Ah, awesome. Thanks for the clarification. https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:438: app_path.value()); On 2012/11/13 20:41:54, huangs1 wrote: > On 2012/11/13 14:15:12, gab wrote: > > If this call is going to be used this way (i.e. to get non-Chrome icons); it > > should at least by renamed to something more generic. > > > > I also feel it shouldn't take a distribution just to get the icon index out of > > it, taking the icon index directly makes more sense to me. > > > > Maybe something like "string16 GetResourcePath(FilePath resource_path, int > > resource_index);" since this applies to more than just icons? > > Done, calling it GetResourceString() per F2F. Also, passing resource_path as a > string instead of FilePath, since many callers only have chrome_exe as string. What resource types other than icons are typically addressed by "path,index" strings? If the answer is "none", then I think this fn should have Icon in the name. https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... File chrome/installer/util/chrome_app_host_operations.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/installer/util/chro... chrome/installer/util/chrome_app_host_operations.cc:131: properties->set_icon(target_exe, dist->GetIconIndex()); On 2012/11/13 20:41:54, huangs1 wrote: > On 2012/11/13 16:19:32, grt wrote: > > target_exe -> dist->GetIconFile() > > Cannot do this, since target_exe has the full path, but GetIconFile() only > returns the bare executable name! Oh yeah, duh. https://codereview.chromium.org/11359133/diff/2004/chrome/browser/extensions/... File chrome/browser/extensions/app_host/app_host_resource.h (right): https://codereview.chromium.org/11359133/diff/2004/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host_resource.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. add this file to the app_host target in chrome/chrome_browser_extensions.gypi
PTAL. https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/app_host/app_host.rc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:38: IDR_MAINFRAME ICON "..\\..\\..\\app\\theme\\app_list.ico" I meant \c, \g, \v are NOT escape characters. https://codereview.chromium.org/11359133/diff/2004/chrome/browser/extensions/... File chrome/browser/extensions/app_host/app_host_resource.h (right): https://codereview.chromium.org/11359133/diff/2004/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host_resource.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/11/14 15:55:14, grt wrote: > add this file to the app_host target in chrome/chrome_browser_extensions.gypi Doh! Done.
https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:438: app_path.value()); On 2012/11/14 15:55:14, grt wrote: > On 2012/11/13 20:41:54, huangs1 wrote: > > On 2012/11/13 14:15:12, gab wrote: > > > If this call is going to be used this way (i.e. to get non-Chrome icons); it > > > should at least by renamed to something more generic. > > > > > > I also feel it shouldn't take a distribution just to get the icon index out > of > > > it, taking the icon index directly makes more sense to me. > > > > > > Maybe something like "string16 GetResourcePath(FilePath resource_path, int > > > resource_index);" since this applies to more than just icons? > > > > Done, calling it GetResourceString() per F2F. Also, passing resource_path as > a > > string instead of FilePath, since many callers only have chrome_exe as string. > > What resource types other than icons are typically addressed by "path,index" > strings? If the answer is "none", then I think this fn should have Icon in the > name. ping
Some comments below. I agree with other comments by grt. Cheers, Gab https://codereview.chromium.org/11359133/diff/8004/chrome/browser/extensions/... File chrome/browser/extensions/app_host/app_host.rc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:2: // Why does this line have an empty comment? https://codereview.chromium.org/11359133/diff/8004/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:428: string16 ShellIntegration::GetChromiumIconPath() { Also rename this method to "...String" instead of "Path" for same reasons? https://codereview.chromium.org/11359133/diff/8004/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:431: FilePath app_path; s/app_path/chrome_exe For consistency with how we name this path (base::FILE_EXE) across the codebase. https://codereview.chromium.org/11359133/diff/8004/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:624: // DisplayIcon, NoModify and NoRepair I don't feel this remaining one line comment adds any value to the code below.. Please expand it or remove it. https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... chrome/installer/util/browser_distribution.cc:234: // as IDR_MAINFRAME. Does IDR_MAINFRAME still apply here? Either way please format this comment as: Comment foo fee faa (e.g. example here woot). https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:124: // Returns the executable file (no path) that Has the icon for the product. s/Has/has s/no path/not path s/file/filename https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:125: virtual string16 GetIconFile(); Ok, if this only returns the filename (which is constant) and simplifies callers, I'm fine with this. What about we call it GetIconFilename() to be even clearer? https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:128: // from GetIconFile(). s/from/by https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/chro... File chrome/installer/util/chrome_frame_distribution.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/chro... chrome/installer/util/chrome_frame_distribution.cc:120: return 0; Why does chrome_frame override this to 0 (the default already) and not other distributions which also use 0. I feel everyone should override this (or nobody... in which case why do we have it?!). https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/goog... File chrome/installer/util/google_chrome_distribution_dummy.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/goog... chrome/installer/util/google_chrome_distribution_dummy.cc:122: } Why isn't GetIconIndex() also found here? https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... chrome/installer/util/shell_util.h:317: static string16 GetResourceString(const string16& resource_path, Make resource_path a const FilePath&.
PTAL. https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11359133/diff/6001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:438: app_path.value()); On 2012/11/14 18:31:09, grt wrote: > On 2012/11/14 15:55:14, grt wrote: > > On 2012/11/13 20:41:54, huangs1 wrote: > > > On 2012/11/13 14:15:12, gab wrote: > > > > If this call is going to be used this way (i.e. to get non-Chrome icons); > it > > > > should at least by renamed to something more generic. > > > > > > > > I also feel it shouldn't take a distribution just to get the icon index > out > > of > > > > it, taking the icon index directly makes more sense to me. > > > > > > > > Maybe something like "string16 GetResourcePath(FilePath resource_path, int > > > > resource_index);" since this applies to more than just icons? > > > > > > Done, calling it GetResourceString() per F2F. Also, passing resource_path > as > > a > > > string instead of FilePath, since many callers only have chrome_exe as > string. > > > > What resource types other than icons are typically addressed by "path,index" > > strings? If the answer is "none", then I think this fn should have Icon in > the > > name. > > ping Yeah, GetIconString() sounds more appropriate. Also changed parameter + local variable names. https://codereview.chromium.org/11359133/diff/8004/chrome/browser/extensions/... File chrome/browser/extensions/app_host/app_host.rc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/browser/extensions/... chrome/browser/extensions/app_host/app_host.rc:2: // On 2012/11/14 18:47:32, gab wrote: > Why does this line have an empty comment? To maintain consistency with the other .rc files. All of them except /app/cf_resources.rc start the same way. https://codereview.chromium.org/11359133/diff/8004/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:428: string16 ShellIntegration::GetChromiumIconPath() { On 2012/11/14 18:47:32, gab wrote: > Also rename this method to "...String" instead of "Path" for same reasons? Done, leading to new changes in the .h file and browser.cc. https://codereview.chromium.org/11359133/diff/8004/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:431: FilePath app_path; On 2012/11/14 18:47:32, gab wrote: > s/app_path/chrome_exe > > For consistency with how we name this path (base::FILE_EXE) across the codebase. Done. https://codereview.chromium.org/11359133/diff/8004/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:624: // DisplayIcon, NoModify and NoRepair On 2012/11/14 18:47:32, gab wrote: > I don't feel this remaining one line comment adds any value to the code below.. > > Please expand it or remove it. Done. https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... chrome/installer/util/browser_distribution.cc:234: // as IDR_MAINFRAME. On 2012/11/14 18:47:32, gab wrote: > Does IDR_MAINFRAME still apply here? > > Either way please format this comment as: > Comment foo fee faa (e.g. example here woot). Done. (FYI "e.g." is always directly followed by a comma). https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:124: // Returns the executable file (no path) that Has the icon for the product. On 2012/11/14 18:47:32, gab wrote: > s/Has/has > s/no path/not path > s/file/filename Done. https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:125: virtual string16 GetIconFile(); On 2012/11/14 18:47:32, gab wrote: > Ok, if this only returns the filename (which is constant) and simplifies > callers, I'm fine with this. > > What about we call it GetIconFilename() to be even clearer? Done. https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... chrome/installer/util/browser_distribution.h:128: // from GetIconFile(). On 2012/11/14 18:47:32, gab wrote: > s/from/by Done. https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/chro... File chrome/installer/util/chrome_frame_distribution.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/chro... chrome/installer/util/chrome_frame_distribution.cc:120: return 0; GoogleChromeSxSDistribution uses non-0. I was expecting that ChromeFrame would have something different, and was surprised that it uses Chrome's. So I overloaded the functions to make this explicit. https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/goog... File chrome/installer/util/google_chrome_distribution_dummy.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/goog... chrome/installer/util/google_chrome_distribution_dummy.cc:122: } On 2012/11/14 18:47:32, gab wrote: > Why isn't GetIconIndex() also found here? Because in the test flow we have google_chrome_distribution.h google_chrome_distribution_dummy.cc GetIconIndex() was not overridden in the .h file, so it's not needed in the dummy .cc. However, GetIconFilename() is overridden, so it needs a stub implementation to prevent link error. (So no change). https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... chrome/installer/util/shell_util.h:317: static string16 GetResourceString(const string16& resource_path, Many callers only have chrome_exe as string16. I think we should refactor ShellUtil.cc to use FilePath for these as much as possible -- but not in this CL?
Looks great, last comments. https://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_... chrome/installer/util/chrome_app_host_distribution.cc:138: return 0; On 2012/11/12 20:00:30, huangs1 wrote: > On 2012/11/09 19:29:28, gab wrote: > > Same icon for Chrome and Chromium builds?! > > Yes; this was the previous behaviour. However, I'm reamoving this routine > because the base class returns 0 anyway. Hmmm... this wasn't the previous behavior... the diff shows you removed kAppListIconIndex which used to be branding specific... https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/brow... chrome/installer/util/browser_distribution.cc:234: // as IDR_MAINFRAME. On 2012/11/14 20:35:55, huangs1 wrote: > On 2012/11/14 18:47:32, gab wrote: > > Does IDR_MAINFRAME still apply here? > > > > Either way please format this comment as: > > Comment foo fee faa (e.g. example here woot). > > Done. (FYI "e.g." is always directly followed by a comma). Ah ok, thanks for the grammar tip :), and thanks for putting it in parans, feels cleaner (and less tightly integrated with the rest of the comment to me). https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... chrome/installer/util/shell_util.h:317: static string16 GetResourceString(const string16& resource_path, On 2012/11/14 20:35:55, huangs1 wrote: > Many callers only have chrome_exe as string16. I think we should refactor > ShellUtil.cc to use FilePath for these as much as possible -- but not in this > CL? I agree (it's already an item in the fixit), but this is not chrome_exe, it's the resource_path (I'm thinking about some of the callers that have a path and do .value() to call this method as it is now...). https://codereview.chromium.org/11359133/diff/7013/chrome/browser/shell_integ... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/11359133/diff/7013/chrome/browser/shell_integ... chrome/browser/shell_integration.h:149: // Returns the path to the Chromium icon. This is used to specify the icon s/path/string (and otherwise adapt the comment to specify exactly what it returns). https://codereview.chromium.org/11359133/diff/7013/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11359133/diff/7013/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:429: // Determine the app path. If we can't determine what that is, we have s/app path/path to chrome.exe https://codereview.chromium.org/11359133/diff/7013/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/11359133/diff/7013/chrome/installer/util/brow... chrome/installer/util/browser_distribution.cc:233: // Assuming that main icon appears first alphabetically (e.g., IDR_MAINFRAME). What do you think of: ... appears first alphabetically in the resource file for GetIconFilename(). ? https://codereview.chromium.org/11359133/diff/7013/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/11359133/diff/7013/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1329: string16 resource_string(icon_path); s/resource_string/icon_string
PTAL. https://codereview.chromium.org/11359133/diff/7013/chrome/browser/shell_integ... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/11359133/diff/7013/chrome/browser/shell_integ... chrome/browser/shell_integration.h:149: // Returns the path to the Chromium icon. This is used to specify the icon On 2012/11/14 21:34:34, gab wrote: > s/path/string (and otherwise adapt the comment to specify exactly what it > returns). Done. Added example. https://codereview.chromium.org/11359133/diff/7013/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11359133/diff/7013/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:429: // Determine the app path. If we can't determine what that is, we have On 2012/11/14 21:34:34, gab wrote: > s/app path/path to chrome.exe Done. https://codereview.chromium.org/11359133/diff/7013/chrome/installer/util/brow... File chrome/installer/util/browser_distribution.cc (right): https://codereview.chromium.org/11359133/diff/7013/chrome/installer/util/brow... chrome/installer/util/browser_distribution.cc:233: // Assuming that main icon appears first alphabetically (e.g., IDR_MAINFRAME). On 2012/11/14 21:34:34, gab wrote: > What do you think of: > ... appears first alphabetically in the resource file for GetIconFilename(). > ? Sounds good. Done. https://codereview.chromium.org/11359133/diff/7013/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/11359133/diff/7013/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1329: string16 resource_string(icon_path); On 2012/11/14 21:34:34, gab wrote: > s/resource_string/icon_string Done.
Pinging some unaddressed comments. https://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_... chrome/installer/util/chrome_app_host_distribution.cc:138: return 0; On 2012/11/14 21:34:34, gab wrote: > On 2012/11/12 20:00:30, huangs1 wrote: > > On 2012/11/09 19:29:28, gab wrote: > > > Same icon for Chrome and Chromium builds?! > > > > Yes; this was the previous behaviour. However, I'm reamoving this routine > > because the base class returns 0 anyway. > > Hmmm... this wasn't the previous behavior... the diff shows you removed > kAppListIconIndex which used to be branding specific... Ping. https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... chrome/installer/util/shell_util.h:317: static string16 GetResourceString(const string16& resource_path, On 2012/11/14 21:34:34, gab wrote: > On 2012/11/14 20:35:55, huangs1 wrote: > > Many callers only have chrome_exe as string16. I think we should refactor > > ShellUtil.cc to use FilePath for these as much as possible -- but not in this > > CL? > > I agree (it's already an item in the fixit), but this is not chrome_exe, it's > the resource_path (I'm thinking about some of the callers that have a path and > do .value() to call this method as it is now...). Ping https://codereview.chromium.org/11359133/diff/8009/chrome/browser/shell_integ... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/11359133/diff/8009/chrome/browser/shell_integ... chrome/browser/shell_integration.h:149: // Returns the string to the Chromium icon, e.g., if user-level, remove "if user-level" this is already an example so you don't need a conditional :) Again, I prefer: Comment (e.g., example). https://codereview.chromium.org/11359133/diff/8009/chrome/browser/shell_integ... chrome/browser/shell_integration.h:150: // "C:\\Users\\{Name}\\AppData\\Local\\G\\C\\A\\chrome.exe,0". You can make this even more generic since it's an example, i.e. C:\path\to\chrome.exe,0 and you don't need to use \\ since this is a comment where there is no need for escaping.
Replied to the missed comments, but have small change. PTAL. https://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_... File chrome/installer/util/chrome_app_host_distribution.cc (right): https://codereview.chromium.org/11359133/diff/1/chrome/installer/util/chrome_... chrome/installer/util/chrome_app_host_distribution.cc:138: return 0; By behaviour, I meant end result, i.e., the icon. Before the App Launcher icon was in chrome.exe, and has index 5 if Google Chrome and 1 if Chromium. Now the icon is copied to app_host.exe, and has index 0. (Note that the routine is now gone). https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... chrome/installer/util/shell_util.h:317: static string16 GetResourceString(const string16& resource_path, There are many callers in shell_util.cc that only has chrome_exe as a string16, and only after several layers did it reach a FilePath. So I figured the refactoring should be saved for later (I'll do it if you want). https://codereview.chromium.org/11359133/diff/8009/chrome/browser/shell_integ... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/11359133/diff/8009/chrome/browser/shell_integ... chrome/browser/shell_integration.h:149: // Returns the string to the Chromium icon, e.g., if user-level, On 2012/11/14 23:47:07, gab wrote: > remove "if user-level" this is already an example so you don't need a > conditional :) > > Again, I prefer: > Comment (e.g., example). Done. https://codereview.chromium.org/11359133/diff/8009/chrome/browser/shell_integ... chrome/browser/shell_integration.h:150: // "C:\\Users\\{Name}\\AppData\\Local\\G\\C\\A\\chrome.exe,0". On 2012/11/14 23:47:07, gab wrote: > You can make this even more generic since it's an example, i.e. > > C:\path\to\chrome.exe,0 > > and you don't need to use \\ since this is a comment where there is no need for > escaping. Done.
LGTM w/ single nit below. Please wait for grt's lgtm for chrome/browser/extensions/app_host/app_host.rc and chrome/browser/extensions/app_host/app_host_resource.h Thanks! Gab https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/11359133/diff/8004/chrome/installer/util/shel... chrome/installer/util/shell_util.h:317: static string16 GetResourceString(const string16& resource_path, On 2012/11/15 00:06:38, huangs1 wrote: > There are many callers in shell_util.cc that only has chrome_exe as a string16, > and only after several layers did it reach a FilePath. So I figured the > refactoring should be saved for later (I'll do it if you want). Ok, fine like this for now, this really needs to be fixed in general :(. https://codereview.chromium.org/11359133/diff/45/chrome/installer/setup/insta... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/11359133/diff/45/chrome/installer/setup/insta... chrome/installer/setup/install_worker.cc:627: dist->GetIconIndex()); nit: indent to to fit with '(' in line above.
https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/installer/setu... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/installer/setu... chrome/installer/setup/install_worker.cc:627: dist->GetIconIndex()); On 2012/11/15 15:04:07, gab wrote: > nit: indent to to fit with '(' in line above. dist->GetIconIndex() is a parameter to GetIconString() though.
https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/installer/setu... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/installer/setu... chrome/installer/setup/install_worker.cc:627: dist->GetIconIndex()); On 2012/11/15 16:05:19, huangs1 wrote: > On 2012/11/15 15:04:07, gab wrote: > > nit: indent to to fit with '(' in line above. > > dist->GetIconIndex() is a parameter to GetIconString() though. Duh, indeed!
https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/browser/extens... File chrome/browser/extensions/app_host/app_host.rc (right): https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/browser/extens... chrome/browser/extensions/app_host/app_host.rc:34: // Note: chrome/installer/util/shell_util.cc depends on the order and number of Is this 'Note: ...' relevant to app_host or only to chrome?
PTAL. https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/browser/extens... File chrome/browser/extensions/app_host/app_host.rc (right): https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/browser/extens... chrome/browser/extensions/app_host/app_host.rc:34: // Note: chrome/installer/util/shell_util.cc depends on the order and number of On 2012/11/15 16:35:12, erikwright wrote: > Is this 'Note: ...' relevant to app_host or only to chrome? Following mini_installer.rc's lead, and simplifying comment, as well as prefixing icon id with IDI_. https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/installer/setu... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/11359133/diff/45/chrome/installer/setu... chrome/installer/setup/install_worker.cc:627: dist->GetIconIndex()); On 2012/11/15 16:24:16, gab wrote: > On 2012/11/15 16:05:19, huangs1 wrote: > > On 2012/11/15 15:04:07, gab wrote: > > > nit: indent to to fit with '(' in line above. > > > > dist->GetIconIndex() is a parameter to GetIconString() though. > > Duh, indeed! So no change.
LGTM. Please let grt verify the resource files.
resource stuff lgtm. please load the resulting binary in VS to verify that the icon has the proper numeric name rather than the symbolic one. i have some nits about function naming. in general, i prefer if our names for things are as close to the platform's names as possible. feel free to disregard in this change if this seems too much like bikeshedding. oh, except that since "GetIconString" does nothing more than format the arguments it's given, i think "FormatIconLocation" is actually a better name, but maybe that's just me. https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/browser/she... chrome/browser/shell_integration.h:149: // Returns the string to the Chromium icon, (e.g., "C:\path\to\chrome.exe,0"). nit: "string to" -> "location (path and index) of" as per http://msdn.microsoft.com/library/windows/desktop/bb774950.aspx https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/installer/u... chrome/installer/util/shell_util.h:315: // Combines |icon_path| with |icon_index| into a string that can be // Returns the string "|icon_path|,|icon_index|" (see, for example, http://msdn.microsoft.com/library/windows/desktop/dd391573.aspx). https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/installer/u... chrome/installer/util/shell_util.h:317: static string16 GetIconString(const string16& icon_path, GetIconString -> FormatIconLocation for consistency with MSDN (see other comments) https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/installer/u... chrome/installer/util/shell_util.h:318: int icon_index); move to previous line
PTAL. https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/browser/she... chrome/browser/shell_integration.h:149: // Returns the string to the Chromium icon, (e.g., "C:\path\to\chrome.exe,0"). On 2012/11/16 05:05:03, grt wrote: > nit: "string to" -> "location (path and index) of" as per > http://msdn.microsoft.com/library/windows/desktop/bb774950.aspx Done. https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/installer/u... chrome/installer/util/shell_util.h:315: // Combines |icon_path| with |icon_index| into a string that can be On 2012/11/16 05:05:03, grt wrote: > // Returns the string "|icon_path|,|icon_index|" (see, for example, > http://msdn.microsoft.com/library/windows/desktop/dd391573.aspx). Done. https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/installer/u... chrome/installer/util/shell_util.h:317: static string16 GetIconString(const string16& icon_path, On 2012/11/16 05:05:03, grt wrote: > GetIconString -> FormatIconLocation for consistency with MSDN (see other > comments) Done. https://chromiumcodereview.appspot.com/11359133/diff/18021/chrome/installer/u... chrome/installer/util/shell_util.h:318: int icon_index); On 2012/11/16 05:05:03, grt wrote: > move to previous line Done.
did you load the resulting binary in VS to verify that the icon has the proper numeric name rather than the symbolic one? https://chromiumcodereview.appspot.com/11359133/diff/28001/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/11359133/diff/28001/chrome/browser/she... chrome/browser/shell_integration.h:152: static string16 GetChromiumIconString(); String -> Location
oh, lgtm with the final naming change assuming that you've verified the resources. On 2012/11/16 21:05:26, grt wrote: > did you load the resulting binary in VS to verify that the > icon has the proper numeric name rather than the symbolic one? > > https://chromiumcodereview.appspot.com/11359133/diff/28001/chrome/browser/she... > File chrome/browser/shell_integration.h (right): > > https://chromiumcodereview.appspot.com/11359133/diff/28001/chrome/browser/she... > chrome/browser/shell_integration.h:152: static string16 GetChromiumIconString(); > String -> Location
Only rename is done. Note that I resynched, so browser.cc has bogus changes. PTAL. https://chromiumcodereview.appspot.com/11359133/diff/28001/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/11359133/diff/28001/chrome/browser/she... chrome/browser/shell_integration.h:152: static string16 GetChromiumIconString(); On 2012/11/16 21:05:26, grt wrote: > String -> Location Done.
Also verified in VC that the icon ID is 101.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/11359133/27002
Presubmit check for 11359133-27002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
chrome/browser/ui
Presubmit checks took 2.5s to calculate.
Adding ben@ for OWNER review. PTAL.
adding correct Ben. Ben, PTAL for chrome_browser_extensions.gypi (adding a resources header to app_host.exe) and c/b/ui/browser.cc (API rename).
TBRing ben@ for trivial GYP and API rename.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/11359133/27002
Retried try job too often for step(s) interactive_ui_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/11359133/27002
Change committed as 168722
lgtm. |
