Windows: Automatically associate file types with Chrome apps.

Windows: Automatically associate file types with Chrome apps.

Currently only works if --enable-apps-file-associations is on.

The Chrome app appears on the "Open with" menu and "Choose default
program" dialog for files of any type mentioned in the app's manifest.
If there is no existing app associated with the file type, the Chrome
app becomes the default handler.

Upon app installation, creates a copy of the app_shim executable to
serve as a portable and uniquely named way to run the app. Any file
types mentioned in the file_handlers section of the app manifest are
associated with the shim.

BUG=130455
Committed: https://crrev.com/fcffcb17a43f6d0ac30cc5b8260028ad6489d6e9
Cr-Commit-Position: refs/heads/master@{#302755}

[Matt Giuca, 2014-10-24 09:47:51]

Patchset #3 (id:40001) has been deleted

[Matt Giuca, 2014-10-24 09:54:35]

mgiuca@chromium.org changed reviewers:
+ jackhou@chromium.org

[Matt Giuca, 2014-10-24 09:54:36]

Here's the initial CL to get a basic file association (with the shim; see the
other CL) behind the --enable-apps-file-associations flag.

Note that I still have some logging and such in there, as it is not completely
finalised. But it is good for review.

Also note that there is nowhere where I have actually turned on compilation of
the shim. So if this were to be released today, turning on the flag would just
be an error because the user would not have a shim. But I think we will want to
wait a bit before shipping the shim with Chrome.

[jackhou1, 2014-10-27 02:13:28]

Looks good, but I guess we have to wait for the outcome of that other CL.

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1805: kOsWin | kOsMac,
Is this necessary for testing? If not we should probably wait until it works to
some degree before adding it here, since it's user-visible-ish.

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:391: // |file_name| is the full
path of the shim binary to be created.
Should this say "|path| is the full ..."?

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:476: return false;
Blank line after return.

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:498: if
(!ShellUtil::AddFileAssociations(
Just something to think about: Would any issues come up if any/all of these
parameters change? I.e. if the progid changes, we should be adding a new
association. If any of the others change, we should be modifying the existing
association.

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:500: return false;
Should we delete the shim if this fails?

[Matt Giuca, 2014-11-04 05:32:23]

Patchset #7 (id:140001) has been deleted

[Matt Giuca, 2014-11-04 05:32:30]

Patchset #6 (id:120001) has been deleted

[Matt Giuca, 2014-11-04 06:00:09]

PTAL.

(Still blocked on the app_shim CL, but we should be able to get this review done
and in the pipeline for submitting.)

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1805: kOsWin | kOsMac,
It lets us tell other people to test it without them having to do custom
command-line arguments.

I think by definition, it's OK to add a flag that doesn't completely work (since
the flags has a big warning at the top of the page).

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:391: // |file_name| is the full
path of the shim binary to be created.
On 2014/10/27 02:13:28, jackhou1 wrote:
> Should this say "|path| is the full ..."?

Done.

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:476: return false;
On 2014/10/27 02:13:28, jackhou1 wrote:
> Blank line after return.

Done.

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:498: if
(!ShellUtil::AddFileAssociations(
Well if the progid doesn't change, then it will just overwrite the existing
association. (We should be careful to clean up any fileexts that have been
removed since the last version though.)

If the progid does change, then we'll create a second one. But that is fine
because the progid is keyed to the app ID (so if it changes, then effectively
we've installed a new app).

https://codereview.chromium.org/181493011/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:500: return false;
On 2014/10/27 02:13:28, jackhou1 wrote:
> Should we delete the shim if this fails?

Done.

[jackhou1, 2014-11-04 06:52:55]

https://codereview.chromium.org/181493011/diff/220001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/181493011/diff/220001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.cc:498:
base::DeleteFile(app_shim_path, false);
Do we need to remove any just-added file associations as well?

https://codereview.chromium.org/181493011/diff/220001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.cc:662:
internals::GetIconFilePath(web_app_path, shortcut_info.title);
Is this intentional?

[Matt Giuca, 2014-11-04 07:16:24]

https://codereview.chromium.org/181493011/diff/220001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/181493011/diff/220001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.cc:498:
base::DeleteFile(app_shim_path, false);
Ugh, yeah we do if we're going to delete the shim.

This is a horrible rat's nest (for something that basically will never fail, and
if it does it will fail on the first one). For example, we may end up deleting a
file association that already existed, and was updated successfully, just
because a subsequent one failed.

What do you think of this plan:
- Add all the file associations and don't exit early if one fails.
- Count the number of successes.
- If there were 0 successes, delete the shim.
- If there were any successes, run SHChangeNotify.
- If there were no failures, return true.

That way, we create as many associations as we can, without having to recover
from failures, and we only return true in the full success case.

https://codereview.chromium.org/181493011/diff/220001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.cc:662:
internals::GetIconFilePath(web_app_path, shortcut_info.title);
On 2014/11/04 06:52:55, jackhou1 wrote:
> Is this intentional?

Done.

[jackhou1, 2014-11-04 21:40:17]

lgtm

https://codereview.chromium.org/181493011/diff/220001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/181493011/diff/220001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.cc:498:
base::DeleteFile(app_shim_path, false);
On 2014/11/04 07:16:24, Matt Giuca wrote:
> Ugh, yeah we do if we're going to delete the shim.
> 
> This is a horrible rat's nest (for something that basically will never fail,
and
> if it does it will fail on the first one). For example, we may end up deleting
a
> file association that already existed, and was updated successfully, just
> because a subsequent one failed.
> 
> What do you think of this plan:
> - Add all the file associations and don't exit early if one fails.
> - Count the number of successes.
> - If there were 0 successes, delete the shim.
> - If there were any successes, run SHChangeNotify.
> - If there were no failures, return true.
> 
> That way, we create as many associations as we can, without having to recover
> from failures, and we only return true in the full success case.

Yeah that sounds good.

[Matt Giuca, 2014-11-05 03:27:23]

As discussed, I've removed kOsWin from about_flags. The flag shouldn't be
available to Windows users until we are actually bundling the shim with the
binary (because it currently requires manual building of the shim to make it
work; anyone who is prepared to do that can manually pass a switch to
chrome.exe).

[Matt Giuca, 2014-11-05 03:27:54]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-05 03:28:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/181493011/270001

[Matt Giuca, 2014-11-05 03:38:51]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2014-11-05 03:39:38]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-05 03:40:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/181493011/270001

[commit-bot: I haz the power, 2014-11-05 04:19:30]

Committed patchset #12 (id:270001)

[commit-bot: I haz the power, 2014-11-05 04:20:10]

Patchset 12 (id:??) landed as
https://crrev.com/fcffcb17a43f6d0ac30cc5b8260028ad6489d6e9
Cr-Commit-Position: refs/heads/master@{#302755}

[Nico, 2014-11-18 23:22:12]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-11-18 23:22:13]

https://codereview.chromium.org/181493011/diff/270001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/181493011/diff/270001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_win.cc:621: if
(switches::kEnableAppsFileAssociations) {
..\..\chrome\browser\web_applications\web_app_win.cc(621,7) :  warning(clang):
address of array 'switches::kEnableAppsFileAssociations' will always evaluate to
'true' [-Wpointer-bool-conversion]
  if (switches::kEnableAppsFileAssociations) {
  ~~  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

You probably want to check if this is in the current command line?

[Matt Giuca, 2014-11-19 00:37:09]

Wow... that's a bad one. Thanks for spotting this. http://crbug.com/434552.