File association for app shims. (Mac)

File association for app shims. (Mac)

This enables file associations behind a flag in chrome://flags. This flag will be
used in future for file associations on Windows and Linux.

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

[tapted, 2014-04-17 04:37:50]

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6989: +      <message
name="IDS_FLAGS_ENABLE_APPS_FILE_ASSOCIATIONS_NAME" desc="Name file associations
for Chrome apps.">
nit: the description reads a little funny

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6993: +        Enable OS integration of file
associations for Chrome apps.
nit: Chrome Apps? (capitalise "A").

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.h (right):

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.h:108: extensions::FileHandlersInfo
file_handlers_info_;
#include chrome/common/extensions/file_handler_info.h?

also.. `struct FileHandlersInfo` doesn't look like it's safe to use as a data
member -- it has a raw pointer to vector. I think it's only been designed for
passing arguments. I guess WebAppShortcutCreator is kinda like this too, but it
scares me..

Maybe make this a `std::vector<FileHandlerInfo> file_handlers_;` ?

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:629: void
UpdateFileTypes(NSMutableDictionary* plist,
move to anonymous namespace

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:631: NSMutableArray*
document_types = [NSMutableArray array];
I think +[.. array] is only defined on NSArray.. Maybe [NSMutableArray
arrayWithCapacity:file_handlers_.size()]; More below

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:633: for (unsigned int i = 0; i <
file_handlers_info.handlers->size(); ++i) {
vector::const_iterator?

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:650: // TODO(jackhou): Add the
type name and and icon file once the manifest
nit: This.. probably needs more indent because it's inside [] function
arguments.

Maybe nicer to do 

NSDictionary* type_dictionary = @{
  foo : bar,
  baz : x
};
[document_types addObject:type_dictionary];

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:656:
app_mode::kCFBundleTypeRoleKey : app_mode::kBundleTypeRoleViewer,
nit: not sure if style guide encourages a trailing comma...

[jackhou1, 2014-04-22 04:40:19]

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6989: +      <message
name="IDS_FLAGS_ENABLE_APPS_FILE_ASSOCIATIONS_NAME" desc="Name file associations
for Chrome apps.">
On 2014/04/17 04:37:50, tapted wrote:
> nit: the description reads a little funny

Done.

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6993: +        Enable OS integration of file
associations for Chrome apps.
On 2014/04/17 04:37:50, tapted wrote:
> nit: Chrome Apps? (capitalise "A").

Done.

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.h (right):

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.h:108: extensions::FileHandlersInfo
file_handlers_info_;
On 2014/04/17 04:37:50, tapted wrote:
> #include chrome/common/extensions/file_handler_info.h?
> 
> also.. `struct FileHandlersInfo` doesn't look like it's safe to use as a data
> member -- it has a raw pointer to vector. I think it's only been designed for
> passing arguments. I guess WebAppShortcutCreator is kinda like this too, but
it
> scares me..
> 
> Maybe make this a `std::vector<FileHandlerInfo> file_handlers_;` ?

Ah, good point. Changed the struct to hold a copy of the vector.

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:629: void
UpdateFileTypes(NSMutableDictionary* plist,
On 2014/04/17 04:37:50, tapted wrote:
> move to anonymous namespace

Done.

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:631: NSMutableArray*
document_types = [NSMutableArray array];
On 2014/04/17 04:37:50, tapted wrote:
> I think +[.. array] is only defined on NSArray.. Maybe [NSMutableArray
> arrayWithCapacity:file_handlers_.size()]; More below

Done.

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:633: for (unsigned int i = 0; i <
file_handlers_info.handlers->size(); ++i) {
On 2014/04/17 04:37:50, tapted wrote:
> vector::const_iterator?

Done.

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:650: // TODO(jackhou): Add the
type name and and icon file once the manifest
On 2014/04/17 04:37:50, tapted wrote:
> nit: This.. probably needs more indent because it's inside [] function
> arguments.
> 
> Maybe nicer to do 
> 
> NSDictionary* type_dictionary = @{
>   foo : bar,
>   baz : x
> };
> [document_types addObject:type_dictionary];

Done.

https://codereview.chromium.org/64803005/diff/30001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:656:
app_mode::kCFBundleTypeRoleKey : app_mode::kBundleTypeRoleViewer,
On 2014/04/17 04:37:50, tapted wrote:
> nit: not sure if style guide encourages a trailing comma...

Checked style guide. It's not explicit, but all examples lack trailing comma.

[tapted, 2014-04-22 05:07:23]

https://codereview.chromium.org/64803005/diff/50001/chrome/common/extensions/...
File chrome/common/extensions/file_handler_info.cc (right):

https://codereview.chromium.org/64803005/diff/50001/chrome/common/extensions/...
chrome/common/extensions/file_handler_info.cc:13: : handlers(*handlers) {}
There's a call in web_app.cc / CreateShortcutsOnFileThread that passes NULL..
Maybe change the constructor argument to accept const reference, and update
callers?

[jackhou1, 2014-04-22 06:47:25]

https://codereview.chromium.org/64803005/diff/50001/chrome/common/extensions/...
File chrome/common/extensions/file_handler_info.cc (right):

https://codereview.chromium.org/64803005/diff/50001/chrome/common/extensions/...
chrome/common/extensions/file_handler_info.cc:13: : handlers(*handlers) {}
On 2014/04/22 05:07:23, tapted wrote:
> There's a call in web_app.cc / CreateShortcutsOnFileThread that passes NULL..
> Maybe change the constructor argument to accept const reference, and update
> callers?

Ugh of course, sorry.

Changed this to just a default constructor. It's more like ShortcutInfo that way
too.

[tapted, 2014-04-24 05:23:01]

lgtm with 2 nits

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6993: +        Enable OS integration of file
associations for Chrome apps.
On 2014/04/22 04:40:19, jackhou1 wrote:
> On 2014/04/17 04:37:50, tapted wrote:
> > nit: Chrome Apps? (capitalise "A").
> 
> Done.

This maybe got eaten by a rebase

https://codereview.chromium.org/64803005/diff/100013/chrome/common/extensions...
File chrome/common/extensions/file_handler_info.cc (right):

https://codereview.chromium.org/64803005/diff/100013/chrome/common/extensions...
chrome/common/extensions/file_handler_info.cc:13: 
nit: remove blank line

[jackhou1, 2014-04-24 07:50:03]

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/64803005/diff/30001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6993: +        Enable OS integration of file
associations for Chrome apps.
On 2014/04/24 05:23:01, tapted wrote:
> On 2014/04/22 04:40:19, jackhou1 wrote:
> > On 2014/04/17 04:37:50, tapted wrote:
> > > nit: Chrome Apps? (capitalise "A").
> > 
> > Done.
> 
> This maybe got eaten by a rebase

Done.

https://codereview.chromium.org/64803005/diff/100013/chrome/common/extensions...
File chrome/common/extensions/file_handler_info.cc (right):

https://codereview.chromium.org/64803005/diff/100013/chrome/common/extensions...
chrome/common/extensions/file_handler_info.cc:13: 
On 2014/04/24 05:23:01, tapted wrote:
> nit: remove blank line

Done.

[jackhou1, 2014-04-24 07:52:12]

thakis, please review for OWNERS:
chrome/common/mac/app_mode_common.h
chrome/common/mac/app_mode_common.mm

benwells, please review for OWNERS:
chrome/common/extensions/file_handler_info.h
chrome/common/extensions/file_handler_info.cc

[benwells, 2014-04-24 09:54:26]

https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
File chrome/common/extensions/file_handler_info.h (right):

https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
chrome/common/extensions/file_handler_info.h:28: struct FileHandlersInfo {
Remind me again why this struct is needed. If you think it will be useful at
some point in the future, how far away is that?

[jackhou1, 2014-04-24 14:48:20]

https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
File chrome/common/extensions/file_handler_info.h (right):

https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
chrome/common/extensions/file_handler_info.h:28: struct FileHandlersInfo {
On 2014/04/24 09:54:26, benwells wrote:
> Remind me again why this struct is needed. If you think it will be useful at
> some point in the future, how far away is that?

It'll need to hold an icon for each file type. They'll be added whenever we
implement the file_handlers changes mgiuca@ proposed here:
https://docs.google.com/a/google.com/document/d/14JlZxF2ZYyLgKEvBZ2X79bHyXtlU...

[Nico, 2014-04-24 19:04:09]

lgtm

[benwells, 2014-04-27 23:55:37]

On 2014/04/24 14:48:20, jackhou1 wrote:
>
https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
> File chrome/common/extensions/file_handler_info.h (right):
> 
>
https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
> chrome/common/extensions/file_handler_info.h:28: struct FileHandlersInfo {
> On 2014/04/24 09:54:26, benwells wrote:
> > Remind me again why this struct is needed. If you think it will be useful at
> > some point in the future, how far away is that?
> 
> It'll need to hold an icon for each file type. They'll be added whenever we
> implement the file_handlers changes mgiuca@ proposed here:
>
https://docs.google.com/a/google.com/document/d/14JlZxF2ZYyLgKEvBZ2X79bHyXtlU...

I'd prefer to introduce the class when it is needed, not preemptively.

[jackhou1, 2014-04-28 02:38:46]

On 2014/04/27 23:55:37, benwells wrote:
> On 2014/04/24 14:48:20, jackhou1 wrote:
> >
>
https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
> > File chrome/common/extensions/file_handler_info.h (right):
> > 
> >
>
https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
> > chrome/common/extensions/file_handler_info.h:28: struct FileHandlersInfo {
> > On 2014/04/24 09:54:26, benwells wrote:
> > > Remind me again why this struct is needed. If you think it will be useful
at
> > > some point in the future, how far away is that?
> > 
> > It'll need to hold an icon for each file type. They'll be added whenever we
> > implement the file_handlers changes mgiuca@ proposed here:
> >
>
https://docs.google.com/a/google.com/document/d/14JlZxF2ZYyLgKEvBZ2X79bHyXtlU...
> 
> I'd prefer to introduce the class when it is needed, not preemptively.

I refactored web_app::[Create|Update|Delete]Shortcuts to take a FileHandlersInfo
so that we wouldn't need to change those signatures again when adding the name
and icon fields. It's probably worth keeping if we're going to add those soon,
but if you think that's not likely, I can change them to take
std::vector<extensions::FileHandlerInfo> instead.

[benwells, 2014-04-28 04:25:36]

On 2014/04/28 02:38:46, jackhou1 wrote:
> On 2014/04/27 23:55:37, benwells wrote:
> > On 2014/04/24 14:48:20, jackhou1 wrote:
> > >
> >
>
https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
> > > File chrome/common/extensions/file_handler_info.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
> > > chrome/common/extensions/file_handler_info.h:28: struct FileHandlersInfo {
> > > On 2014/04/24 09:54:26, benwells wrote:
> > > > Remind me again why this struct is needed. If you think it will be
useful
> at
> > > > some point in the future, how far away is that?
> > > 
> > > It'll need to hold an icon for each file type. They'll be added whenever
we
> > > implement the file_handlers changes mgiuca@ proposed here:
> > >
> >
>
https://docs.google.com/a/google.com/document/d/14JlZxF2ZYyLgKEvBZ2X79bHyXtlU...
> > 
> > I'd prefer to introduce the class when it is needed, not preemptively.
> 
> I refactored web_app::[Create|Update|Delete]Shortcuts to take a
FileHandlersInfo
> so that we wouldn't need to change those signatures again when adding the name
> and icon fields. It's probably worth keeping if we're going to add those soon,
> but if you think that's not likely, I can change them to take
> std::vector<extensions::FileHandlerInfo> instead.

I don't know if we'll be doing that change soon. I don't think there is anyone
with cycles to work on it, so it may never happen.

[benwells, 2014-04-29 03:11:37]

On 2014/04/28 04:25:36, benwells wrote:
> On 2014/04/28 02:38:46, jackhou1 wrote:
> > On 2014/04/27 23:55:37, benwells wrote:
> > > On 2014/04/24 14:48:20, jackhou1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
> > > > File chrome/common/extensions/file_handler_info.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/64803005/diff/120001/chrome/common/extensions...
> > > > chrome/common/extensions/file_handler_info.h:28: struct FileHandlersInfo
{
> > > > On 2014/04/24 09:54:26, benwells wrote:
> > > > > Remind me again why this struct is needed. If you think it will be
> useful
> > at
> > > > > some point in the future, how far away is that?
> > > > 
> > > > It'll need to hold an icon for each file type. They'll be added whenever
> we
> > > > implement the file_handlers changes mgiuca@ proposed here:
> > > >
> > >
> >
>
https://docs.google.com/a/google.com/document/d/14JlZxF2ZYyLgKEvBZ2X79bHyXtlU...
> > > 
> > > I'd prefer to introduce the class when it is needed, not preemptively.
> > 
> > I refactored web_app::[Create|Update|Delete]Shortcuts to take a
> FileHandlersInfo
> > so that we wouldn't need to change those signatures again when adding the
name
> > and icon fields. It's probably worth keeping if we're going to add those
soon,
> > but if you think that's not likely, I can change them to take
> > std::vector<extensions::FileHandlerInfo> instead.
> 
> I don't know if we'll be doing that change soon. I don't think there is anyone
> with cycles to work on it, so it may never happen.

After discussing in person, lgtm.

If we don't do something with that struct soon please remove it.

[jackhou1, 2014-04-29 03:56:02]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-04-29 03:57:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/64803005/140001

[commit-bot: I haz the power, 2014-04-29 03:57:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 03:57:22]

Failed to apply patch for chrome/browser/about_flags.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/about_flags.cc
  Hunk #1 FAILED at 1778.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/about_flags.cc.rej

Patch:       chrome/browser/about_flags.cc
Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index
179766305c790acb2cacbb3dd02e9609ce429d66..ac1e3978588fe94fd513adf9af242281daaed931
100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -1778,6 +1778,13 @@ const Experiment kExperiments[] = {
     kOsAll,
     SINGLE_VALUE_TYPE(switches::kAllowInsecureWebSocketFromHttpsOrigin)
   },
+  {
+    "enable-apps-file-associations",
+    IDS_FLAGS_ENABLE_APPS_FILE_ASSOCIATIONS_NAME,
+    IDS_FLAGS_ENABLE_APPS_FILE_ASSOCIATIONS_DESCRIPTION,
+    kOsMac,
+    SINGLE_VALUE_TYPE(switches::kEnableAppsFileAssociations)
+  },
 };
 
 const Experiment* experiments = kExperiments;

[jackhou1, 2014-04-29 04:40:27]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-04-29 04:40:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/64803005/160001

[commit-bot: I haz the power, 2014-04-29 04:43:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 04:43:40]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[jackhou1, 2014-04-29 06:49:33]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-04-29 06:50:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/64803005/180001

[commit-bot: I haz the power, 2014-04-29 11:07:54]

Change committed as 266840