Start menu shortcuts for apps on Windows are now inside a "Chrome Apps" submenu.

Start menu shortcuts for apps on Windows are now inside a "Chrome Apps" submenu.

The facility to do this was already present, save for a bug fix: if the submenu
does not already exist, it is now created instead of failing silently. This
change does not affect URL shortcuts.

BUG=177579
TEST=Create shortcut in Start Menu for Chrome app; should go in "Chromium Apps"
     folder.
TEST=Create shortcut in Start Menu for URL; should go in top-level Start Menu.
TEST=Uninstall Chrome app; should delete shortcut from Chromium Apps folder.
     If the app is the last shortcut in the folder, should delete folder.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197304

[Matt Giuca, 2013-04-29 05:36:40]

Hi reviewers,
This is intended to be followed up with a CL that creates app shortcuts
automatically.

Sriram: Adding you for product approval on the behaviour change.
Sam: For a local Sydney review.
Ben: For OWNERS.

Note: I am sending out the Linux one in parallel:
https://codereview.chromium.org/14383003. Some of the code is shared between the
two CLs, and I will deal with the merge conflict on whichever one goes through
second.

[Ben Goodger (Google), 2013-04-29 16:10:28]

lgtm

[gab, 2013-04-29 21:43:56]

Drive-by.

https://codereview.chromium.org/14533004/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14533004/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/web_app_win.cc:312: file_util::Delete(*j,
false);
This will not delete the  "Chrome Apps" folder itself.

Is this only used for uninstall? If so, ShellUtil already mostly covers
uninstalling all shortcuts that point to the chrome.exe being uninstalled, and
will fully cover it after huangs' CL @
https://codereview.chromium.org/14287008/.

This code could thus be removed. It would only need to be replaced by a new
entry to delete the "Chrome Apps" folder which the code in ShellUtil isn't aware
of yet.

[Matt Giuca, 2013-04-30 00:53:43]

https://codereview.chromium.org/14533004/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14533004/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/web_app_win.cc:312: file_util::Delete(*j,
false);
On 2013/04/29 21:43:57, gab wrote:
> This will not delete the  "Chrome Apps" folder itself.

Noted. It should delete the folder if it becomes empty after this. I think that
is suitable to delay for another CL.

> Is this only used for uninstall? If so, ShellUtil already mostly covers
> uninstalling all shortcuts that point to the chrome.exe being uninstalled, and
> will fully cover it after huangs' CL @
> https://codereview.chromium.org/14287008/.

I think you are talking about uninstall of Chrome. This is used for
uninstallation of a particular app, which I don't believe huangs' code will
cover.

[Matt Giuca, 2013-04-30 01:16:43]

At Ben Wells' suggestion, adding benwells@ as reviewer and moving saroop@ to CC.

Ben says: Why are regular website applications being put into the "Chrome Apps"
submenu? They should continue to be created at the top level. Hosted apps and V1
Packaged apps are fine to be in the "Chrome Apps" menu.

[Matt Giuca, 2013-04-30 02:58:03]

https://codereview.chromium.org/14533004/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14533004/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/web_app_win.cc:312: file_util::Delete(*j,
false);
On 2013/04/30 00:53:43, Matt Giuca wrote:
> On 2013/04/29 21:43:57, gab wrote:
> > This will not delete the  "Chrome Apps" folder itself.
> 
> Noted. It should delete the folder if it becomes empty after this. I think
that
> is suitable to delay for another CL.

Actually, I just did it here (not too big after all).

https://codereview.chromium.org/14533004/diff/15001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/14533004/diff/15001/chrome/browser/ui/views/c...
chrome/browser/ui/views/create_application_shortcut_view.cc:437: // top-level
menu.
Ben Wells: Now it will create URL apps' shortcuts in the top-level start menu,
and all Chrome apps (Hosted, V1 and V2 packaged) in the "Chrome Apps" start menu
folder.

[benwells, 2013-04-30 03:22:19]

lgtm with a nit

https://codereview.chromium.org/14533004/diff/15001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14533004/diff/15001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:22: #include
"grit/chromium_strings.h"
I think you'll need grit/google_chrome_strings.h as well.

https://codereview.chromium.org/14533004/diff/15001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:322: if
(file_util::IsDirectoryEmpty(chrome_apps_dir)) {
Nit: remove braces around single line if

[Matt Giuca, 2013-04-30 03:50:36]

https://codereview.chromium.org/14533004/diff/15001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14533004/diff/15001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:22: #include
"grit/chromium_strings.h"
On 2013/04/30 03:22:19, benwells wrote:
> I think you'll need grit/google_chrome_strings.h as well.

Hmm. I want to be sure about this because I don't want Chromium to work but
Google Chrome to break. But almost all examples include only chromium_strings.h
and not google_chrome_strings.h:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome_frame/utils...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/jum...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome_frame/chrom...

However there are eleven files in the codebase which do include
google_chrome_strings.h:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/abo...

My feeling is that it doesn't really matter. I'll just go with the most common
case which is to just include chromium_strings.h.

https://codereview.chromium.org/14533004/diff/15001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:322: if
(file_util::IsDirectoryEmpty(chrome_apps_dir)) {
On 2013/04/30 03:22:19, benwells wrote:
> Nit: remove braces around single line if

Done.

[commit-bot: I haz the power, 2013-04-30 03:51:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/14533004/22001

[commit-bot: I haz the power, 2013-04-30 06:07:25]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-04-30 06:08:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/14533004/22001

[commit-bot: I haz the power, 2013-04-30 06:30:57]

Change committed as 197304