Create Linux platform app shortcuts to run in their own process.

Create Linux platform app shortcuts to run in their own process.

This change also reorganises the web_app::CreateShortcut code a bit to
allow us to do this without losing any functionality on gtk. Specifically
the ability to create a shortcut on the file thread and get a return value
is still available for Linux, and goes through the web_app code path.

Some of the variable names and comments in web_app.h were also updated
to be clearer.

BUG=None.
TEST=Test that shortcut creation works on Linux, both for platform apps and
  normal hosted apps.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132151

[Elliot Glaysher, 2012-04-02 17:15:20]

Do you mean on their own thread? I didn't see any process creation stuff in this
patch.

[benwells, 2012-04-03 01:40:51]

On 2012/04/02 17:15:20, Elliot Glaysher wrote:
> Do you mean on their own thread? I didn't see any process creation stuff in
this
> patch.

Sorry I should have been more clear. The process stuff is enabled by using
CommandLineArgsForPlatformApp for platform apps, which adds the user_data_dir
argument. This was already there and used by Windows, but not Linux.

[Elliot Glaysher, 2012-04-03 17:32:55]

lgtm

[benwells, 2012-04-12 05:09:51]

sail: I'm thinking I'd still like to bring this change in (to make Linux and
Windows consistent), and then replace the user_data_dir stuff with the profile
stuff as a separate change.

How does that sound? If you're cool with the strategy, can you look at the
change?

[sail, 2012-04-12 15:06:29]

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
chrome/browser/shell_integration.h:122: #if defined(USE_X11)
could you move this to shell_integration_linux.h? Since these are just statics
you probably don't need them to be a part of the ShortcutInfo struct.

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:143:
!file_util::CreateDirectory(shortcut_data_dir)) {
is this really necessary? on the Mac the data directory is created by Chrome at
startup. Maybe we can fix the chrome startup code instead?

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:60: const FilePath& profile_path,
do you really need this? if you just call CreateShortcut things will still work
right?

[benwells, 2012-04-13 01:48:48]

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
chrome/browser/shell_integration.h:122: #if defined(USE_X11)
On 2012/04/12 15:06:29, sail wrote:
> could you move this to shell_integration_linux.h? Since these are just statics
> you probably don't need them to be a part of the ShortcutInfo struct.

They're part of the ShellIntegration class, not the ShortcutInfo struct. The
ShellIntegration class is more of a namespace than a class.

I guess we could convert it to a namespace and split out all the platform
specific stuff into separate headers, but that's orthogonal.

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:143:
!file_util::CreateDirectory(shortcut_data_dir)) {
On 2012/04/12 15:06:29, sail wrote:
> is this really necessary? on the Mac the data directory is created by Chrome
at
> startup. Maybe we can fix the chrome startup code instead?

Yes, some files are put in this folder at shortcut creation time. We could leave
this in each platform specific section but it felt good to make it common.

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:60: const FilePath& profile_path,
On 2012/04/12 15:06:29, sail wrote:
> do you really need this? if you just call CreateShortcut things will still
work
> right?

This function returns success or failure, which the other one doesn't (and can't
as it is asynchronous).

[sail, 2012-04-13 01:59:14]

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
chrome/browser/shell_integration.h:122: #if defined(USE_X11)
On 2012/04/13 01:48:48, benwells wrote:
> On 2012/04/12 15:06:29, sail wrote:
> > could you move this to shell_integration_linux.h? Since these are just
statics
> > you probably don't need them to be a part of the ShortcutInfo struct.
> 
> They're part of the ShellIntegration class, not the ShortcutInfo struct. The
> ShellIntegration class is more of a namespace than a class.
> 
> I guess we could convert it to a namespace and split out all the platform
> specific stuff into separate headers, but that's orthogonal.

Sorry, I mistook which struct this was in. Regardless. Adding more #ifdefed out
code is bad. Putting it in the platform specific header is better.

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:143:
!file_util::CreateDirectory(shortcut_data_dir)) {
On 2012/04/13 01:48:48, benwells wrote:
> On 2012/04/12 15:06:29, sail wrote:
> > is this really necessary? on the Mac the data directory is created by Chrome
> at
> > startup. Maybe we can fix the chrome startup code instead?
> 
> Yes, some files are put in this folder at shortcut creation time. We could
leave
> this in each platform specific section but it felt good to make it common.

ok

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:60: const FilePath& profile_path,
On 2012/04/13 01:48:48, benwells wrote:
> On 2012/04/12 15:06:29, sail wrote:
> > do you really need this? if you just call CreateShortcut things will still
> work
> > right?
> 
> This function returns success or failure, which the other one doesn't (and
can't
> as it is asynchronous).

what's the point of returning failure?
putting up an error dialog isn't helpful unless you're adding new UI to handle
the error somehow

[benwells, 2012-04-13 02:09:02]

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
chrome/browser/shell_integration.h:122: #if defined(USE_X11)
On 2012/04/13 01:59:14, sail wrote:
> On 2012/04/13 01:48:48, benwells wrote:
> > On 2012/04/12 15:06:29, sail wrote:
> > > could you move this to shell_integration_linux.h? Since these are just
> statics
> > > you probably don't need them to be a part of the ShortcutInfo struct.
> > 
> > They're part of the ShellIntegration class, not the ShortcutInfo struct. The
> > ShellIntegration class is more of a namespace than a class.
> > 
> > I guess we could convert it to a namespace and split out all the platform
> > specific stuff into separate headers, but that's orthogonal.
> 
> Sorry, I mistook which struct this was in. Regardless. Adding more #ifdefed
out
> code is bad. Putting it in the platform specific header is better.

Agreed. There are currently no platform specific headers. I could create them
and move the platform specific stuff in there, in the process converting from a
class to a namespace, but that feels orthogonal to this change.

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:60: const FilePath& profile_path,
On 2012/04/13 01:59:14, sail wrote:
> On 2012/04/13 01:48:48, benwells wrote:
> > On 2012/04/12 15:06:29, sail wrote:
> > > do you really need this? if you just call CreateShortcut things will still
> > work
> > > right?
> > 
> > This function returns success or failure, which the other one doesn't (and
> can't
> > as it is asynchronous).
> 
> what's the point of returning failure?
> putting up an error dialog isn't helpful unless you're adding new UI to handle
> the error somehow

In the NTP there is a command to create the shortcut which shows a dialog. The
gtk version of this dialog shows an error message if the command fails instead
of silently failing.

Prior to this change it basically replicated the CreatePlatformShortcut code in
the dialog, with this change we can make it all common in web_app.cc

[sail, 2012-04-13 02:16:41]

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
chrome/browser/shell_integration.h:122: #if defined(USE_X11)
On 2012/04/13 02:09:02, benwells wrote:
> On 2012/04/13 01:59:14, sail wrote:
> > On 2012/04/13 01:48:48, benwells wrote:
> > > On 2012/04/12 15:06:29, sail wrote:
> > > > could you move this to shell_integration_linux.h? Since these are just
> > statics
> > > > you probably don't need them to be a part of the ShortcutInfo struct.
> > > 
> > > They're part of the ShellIntegration class, not the ShortcutInfo struct.
The
> > > ShellIntegration class is more of a namespace than a class.
> > > 
> > > I guess we could convert it to a namespace and split out all the platform
> > > specific stuff into separate headers, but that's orthogonal.
> > 
> > Sorry, I mistook which struct this was in. Regardless. Adding more #ifdefed
> out
> > code is bad. Putting it in the platform specific header is better.
> 
> Agreed. There are currently no platform specific headers. I could create them
> and move the platform specific stuff in there, in the process converting from
a
> class to a namespace, but that feels orthogonal to this change.

You don't have to move everything. Just create a new namespace
ShellIntegrationX11 and add the new function in that.

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:60: const FilePath& profile_path,
On 2012/04/13 02:09:02, benwells wrote:
> On 2012/04/13 01:59:14, sail wrote:
> > On 2012/04/13 01:48:48, benwells wrote:
> > > On 2012/04/12 15:06:29, sail wrote:
> > > > do you really need this? if you just call CreateShortcut things will
still
> > > work
> > > > right?
> > > 
> > > This function returns success or failure, which the other one doesn't (and
> > can't
> > > as it is asynchronous).
> > 
> > what's the point of returning failure?
> > putting up an error dialog isn't helpful unless you're adding new UI to
handle
> > the error somehow
> 
> In the NTP there is a command to create the shortcut which shows a dialog. The
> gtk version of this dialog shows an error message if the command fails instead
> of silently failing.
> 
> Prior to this change it basically replicated the CreatePlatformShortcut code
in
> the dialog, with this change we can make it all common in web_app.cc

ok

[benwells, 2012-04-13 04:55:29]

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/9958006/diff/2015/chrome/browser/shell_integra...
chrome/browser/shell_integration.h:122: #if defined(USE_X11)
On 2012/04/13 02:16:41, sail wrote:
> On 2012/04/13 02:09:02, benwells wrote:
> > On 2012/04/13 01:59:14, sail wrote:
> > > On 2012/04/13 01:48:48, benwells wrote:
> > > > On 2012/04/12 15:06:29, sail wrote:
> > > > > could you move this to shell_integration_linux.h? Since these are just
> > > statics
> > > > > you probably don't need them to be a part of the ShortcutInfo struct.
> > > > 
> > > > They're part of the ShellIntegration class, not the ShortcutInfo struct.
> The
> > > > ShellIntegration class is more of a namespace than a class.
> > > > 
> > > > I guess we could convert it to a namespace and split out all the
platform
> > > > specific stuff into separate headers, but that's orthogonal.
> > > 
> > > Sorry, I mistook which struct this was in. Regardless. Adding more
#ifdefed
> > out
> > > code is bad. Putting it in the platform specific header is better.
> > 
> > Agreed. There are currently no platform specific headers. I could create
them
> > and move the platform specific stuff in there, in the process converting
from
> a
> > class to a namespace, but that feels orthogonal to this change.
> 
> You don't have to move everything. Just create a new namespace
> ShellIntegrationX11 and add the new function in that.

OK. I've moved just the new function. Let me know if you had something else in
mind.

It feels good like this but the rest of these X11 functions should also move out
(and probably the windows stuff too for consistency), but I'd like to do that as
a separate change to keep the changes manageable and reviewable.

While I'm cleaning it up it might make sense to change ShellIntegration from a
class to a namespace. Its all static except IsRunningInAppMode which doesn't
appear to be used anywhere. Is there any reason to leave it as a class?

[sail, 2012-04-13 05:03:00]

Cool, LGTM!

[commit-bot: I haz the power, 2012-04-13 05:24:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9958006/11004

[commit-bot: I haz the power, 2012-04-13 06:46:36]

Change committed as 132151