Merge into trunk : fix-application-name-setup : Code : webbrowser-app

Reviewer	Review Type	Date Requested	Status
system-apps-ci-bot	continuous-integration		Needs Fixing on 2016-05-10
Olivier Tilloy		2016-05-03	Approve on 2016-05-10
Review via email: mp+293658@code.launchpad.net

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-05-04:

#

The proposed solution only partially addresses the problem: when launching two instances of the webapp container on desktop (without an explicit app id), the second instance will open a new tab/overlay in the first instance (bug #1577806), because all webapps will have their app id set to the default value "webapp-container".

What we really need is to ensure that the app *always* has a reasonably unique app id, even when it was not explicitly set (typically, on a unity7 desktop). I propose using an encoded version of the initial URL when no app id can be inferred.

For example, if the webapp container was invoked like that on desktop:

webapp-container https://ubuntu.slack.com/

We could urlencode the URL and set the app id to "https%3A%2F%2Fubuntu.slack.com%2F". Or, because we need to be careful about the length of the path where the single instance socket lives (see https://bazaar.launchpad.net/~phablet-team/webbrowser-app/trunk/view/head:/src/app/single-instance-manager.cpp#L71), we might want to use a short hash of that value.

I assume that for click packages that ship several desktop files, we want to consider those as separate apps, and thus not have them share where they write data (cookies, cache, etc…). In that case we want to make sure that the dataLocation and cacheLocation are unique for each app within a package. Is that guaranteed by setting the application name to appIdParts.first() ? (on a related note, a comment to document the exact structure of APP_ID under unity8/UAL would be useful for future reference)

It looks to me like SingleInstanceManager::getProfilePath() should in fact be moved to BrowserApplication::initialize(), because this affects all the code that reads/writes data, not just the single instance manager.

And since all the logic to infer the app id is specific to the webapp container (the app id for the browser will always be "webbrowser-app"), it should be moved to src/app/webcontainer/webapp-container.cpp. The second parameter to BrowserApplication::initialize() would then become "appId" instead of "defaultAppId".

Finally, one style nit: given that the default app id for the browser is referred to in only one place, it doesn’t need to be declared as a const variable in the unnamed namespace.

The proposed solution only partially addresses the problem: when launching two instances of the webapp container on desktop (without an explicit app id), the second instance will open a new tab/overlay in the first instance (bug #1577806), because all webapps will have their app id set to the default value "webapp-container".

What we really need is to ensure that the app *always* has a reasonably unique app id, even when it was not explicitly set (typically, on a unity7 desktop). I propose using an encoded version of the initial URL when no app id can be inferred.

For example, if the webapp container was invoked like that on desktop:

webapp-container https://ubuntu.slack.com/

We could urlencode the URL and set the app id to "https%3A%2F%2Fubuntu.slack.com%2F". Or, because we need to be careful about the length of the path where the single instance socket lives (see https://bazaar.launchpad.net/~phablet-team/webbrowser-app/trunk/view/head:/src/app/single-instance-manager.cpp#L71), we might want to use a short hash of that value.

I assume that for click packages that ship several desktop files, we want to consider those as separate apps, and thus not have them share where they write data (cookies, cache, etc…). In that case we want to make sure that the dataLocation and cacheLocation are unique for each app within a package. Is that guaranteed by setting the application name to appIdParts.first() ? (on a related note, a comment to document the exact structure of APP_ID under unity8/UAL would be useful for future reference)

It looks to me like SingleInstanceManager::getProfilePath() should in fact be moved to BrowserApplication::initialize(), because this affects all the code that reads/writes data, not just the single instance manager.

And since all the logic to infer the app id is specific to the webapp container (the app id for the browser will always be "webbrowser-app"), it should be moved to src/app/webcontainer/webapp-container.cpp. The second parameter to BrowserApplication::initialize() would then become "appId" instead of "defaultAppId".

Finally, one style nit: given that the default app id for the browser is referred to in only one place, it doesn’t need to be declared as a const variable in the unnamed namespace.

review: Needs Fixing

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2016-05-04:

#

> The proposed solution only partially addresses the problem: when launching two
> instances of the webapp container on desktop (without an explicit app id), the
> second instance will open a new tab/overlay in the first instance (bug
> #1577806), because all webapps will have their app id set to the default value
> "webapp-container".
>
> What we really need is to ensure that the app *always* has a reasonably unique
> app id, even when it was not explicitly set (typically, on a unity7 desktop).
> I propose using an encoded version of the initial URL when no app id can be
> inferred.
>
> For example, if the webapp container was invoked like that on desktop:
>
> webapp-container https://ubuntu.slack.com/
>
> We could urlencode the URL and set the app id to
> "https%3A%2F%2Fubuntu.slack.com%2F". Or, because we need to be careful about
> the length of the path where the single instance socket lives (see
> https://bazaar.launchpad.net/~phablet-team/webbrowser-
> app/trunk/view/head:/src/app/single-instance-manager.cpp#L71), we might want
> to use a short hash of that value.

I thought about that problem too and chose to handle things that way for specific reasons
(that you might not agree on :) ),

- on desktop and until we have convergence the de facto way of doing things was to use
the --app-id command line parameter, this was both a way to ensure that the user did
acknowledge and control the exact app id. I am not sure we want to add another mechanism
for that,

- encoding the url is not a good way to auto determine the APP_ID since it also breaks
as soon 2 webapps share the same homepage. it also breaks if at runtime a webapp does not
define a homepage in the command line as it is the case with a few,

>
> I assume that for click packages that ship several desktop files, we want to
> consider those as separate apps, and thus not have them share where they write
> data (cookies, cache, etc…). In that case we want to make sure that the
> dataLocation and cacheLocation are unique for each app within a package. Is
> that guaranteed by setting the application name to appIdParts.first() ? (on a
> related note, a comment to document the exact structure of APP_ID under
> unity8/UAL would be useful for future reference)
>
> It looks to me like SingleInstanceManager::getProfilePath() should in fact be
> moved to BrowserApplication::initialize(), because this affects all the code
> that reads/writes data, not just the single instance manager.
>
> And since all the logic to infer the app id is specific to the webapp
> container (the app id for the browser will always be "webbrowser-app"), it
> should be moved to src/app/webcontainer/webapp-container.cpp. The second
> parameter to BrowserApplication::initialize() would then become "appId"
> instead of "defaultAppId".

well actually no, my view of things (and it is reflected by the way I did this)
is that when you have a single click, you want to implicitly share those
between the apps. It is based on some use cases that required that semantics,

> The proposed solution only partially addresses the problem: when launching two
> instances of the webapp container on desktop (without an explicit app id), the
> second instance will open a new tab/overlay in the first instance (bug
> #1577806), because all webapps will have their app id set to the default value
> "webapp-container".
> 
> What we really need is to ensure that the app *always* has a reasonably unique
> app id, even when it was not explicitly set (typically, on a unity7 desktop).
> I propose using an encoded version of the initial URL when no app id can be
> inferred.
> 
> For example, if the webapp container was invoked like that on desktop:
> 
>     webapp-container https://ubuntu.slack.com/
> 
> We could urlencode the URL and set the app id to
> "https%3A%2F%2Fubuntu.slack.com%2F". Or, because we need to be careful about
> the length of the path where the single instance socket lives (see
> https://bazaar.launchpad.net/~phablet-team/webbrowser-
> app/trunk/view/head:/src/app/single-instance-manager.cpp#L71), we might want
> to use a short hash of that value.

I thought about that problem too and chose to handle things that way for specific reasons
(that you might not agree on :) ),

- on desktop and until we have convergence the de facto way of doing things was to use
the --app-id command line parameter, this was both a way to ensure that the user did
acknowledge and control the exact app id. I am not sure we want to add another mechanism
for that,

- encoding the url is not a good way to auto determine the APP_ID since it also breaks
as soon 2 webapps share the same homepage. it also breaks if at runtime a webapp does not
define a homepage in the command line as it is the case with a few,

> 
> I assume that for click packages that ship several desktop files, we want to
> consider those as separate apps, and thus not have them share where they write
> data (cookies, cache, etc…). In that case we want to make sure that the
> dataLocation and cacheLocation are unique for each app within a package. Is
> that guaranteed by setting the application name to appIdParts.first() ? (on a
> related note, a comment to document the exact structure of APP_ID under
> unity8/UAL would be useful for future reference)
> 
> It looks to me like SingleInstanceManager::getProfilePath() should in fact be
> moved to BrowserApplication::initialize(), because this affects all the code
> that reads/writes data, not just the single instance manager.
> 
> And since all the logic to infer the app id is specific to the webapp
> container (the app id for the browser will always be "webbrowser-app"), it
> should be moved to src/app/webcontainer/webapp-container.cpp. The second
> parameter to BrowserApplication::initialize() would then become "appId"
> instead of "defaultAppId".

well actually no, my view of things (and it is reflected by the way I did this)
is that when you have a single click, you want to implicitly share those
between the apps. It is based on some use cases that required that semantics,

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-05-04:

#

> on desktop and until we have convergence the de facto way of doing
> things was to use the --app-id command line parameter, this was both a
> way to ensure that the user did acknowledge and control the exact app
> id. I am not sure we want to add another mechanism for that,

Fair enough. Can we add a big fat warning printed on the console when the app id is not set, to warn of the consequences, and explain how to set the app id (--app-id command-line parameter or APP_ID env var)?
I’m even thinking that we may want to bail out if no app id is set, to prevent various apps from sharing their data under the generic "webapp-container" profile. Not sure whether that’s very developer-friendly though.

> when you have a single click, you want to implicitly share those
> between the apps. It is based on some use cases that required that
> semantics,

OK. Can that be explicitly documented in the code with a comment?
And my previous suggestion of documenting the exact structure of APP_ID under unity8/UAL is still valid, I’d rather not have to do some investigation work next time I look at that code again.

> And since all the logic to infer the app id is specific to the webapp
> container (the app id for the browser will always be
> "webbrowser-app"), it should be moved to
> src/app/webcontainer/webapp-container.cpp. The second parameter to
> BrowserApplication::initialize() would then become "appId" instead of
> "defaultAppId".

This still holds true.

lp:~abreu-alexandre/webbrowser-app/fix-application-name-setup updated on 2016-05-05

1455. By Alexandre Abreu on 2016-05-05: Add comments; factor code for app_id retrieval

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2016-05-05:

#

updated

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-05-06:

#

Can you add a Q_ASSERT(!appId.isEmpty()) to the beginning of BrowserApplication::initialize()?
Passing an empty app id would lead to undefined behaviour I think.
That would also render the check for appIdParts.isEmpty() in SingleInstanceManager::getProfilePathFromAppId() useless.

In single-instance-manager.cpp, the additional include of <QDebug> is useless, it’s already included earlier on.
The additional include of <QtCore/QCoreApplication> also appears unused. Can you please remove it?

> https://bugs.launchpad.net/ubuntu/+source/ubuntu-ui-toolkit/+bug/1555542
Can you shorten that link to https://launchpad.net/bugs/1555542 ?

> appIdParts.mid(1, 1)[0]
Isn’t that equivalent to appIdParts[1] ?

> return profilePath
> + QDir::separator()
> + appDesktopName;
That could go on one single line.

It doesn’t look like getProfilePathFromAppId() needs to be a static member of SingleInstanceManager, it could simply be a function inside an unnamed namespace.

There’s a meaningless additional blank line in webbrowser-app.cpp, can it be reverted?

In tst_SingleInstanceManagerTests.cpp, the second parameter to run() should not be an empty string, this is unexpected. Can you use an non-empty string token?

Have you considered my suggestion of completely bailing out if no app id is explicitly set for the webapp container? I don’t think it makes sense to have apps that run in a "generic" container anyway, does it?

review: Needs Fixing

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2016-05-09:

#

> Can you add a Q_ASSERT(!appId.isEmpty()) to the beginning of
> BrowserApplication::initialize()?

I added an explicit check since it should be something that is guaranteed
at runtime whatever is the build type imo,

> Passing an empty app id would lead to undefined behaviour I think.
> That would also render the check for appIdParts.isEmpty() in
> SingleInstanceManager::getProfilePathFromAppId() useless.

done

> In single-instance-manager.cpp, the additional include of <QDebug> is useless,
> it’s already included earlier on.
> The additional include of <QtCore/QCoreApplication> also appears unused. Can
> you please remove it?

done

> > https://bugs.launchpad.net/ubuntu/+source/ubuntu-ui-toolkit/+bug/1555542
> Can you shorten that link to https://launchpad.net/bugs/1555542 ?

done

> > appIdParts.mid(1, 1)[0]
> Isn’t that equivalent to appIdParts[1] ?

totally :) done

> > return profilePath
> > + QDir::separator()
> > + appDesktopName;
> That could go on one single line.

done

> It doesn’t look like getProfilePathFromAppId() needs to be a static member of
> SingleInstanceManager, it could simply be a function inside an unnamed
> namespace.

totally done,

> There’s a meaningless additional blank line in webbrowser-app.cpp, can it be
> reverted?

done

> In tst_SingleInstanceManagerTests.cpp, the second parameter to run() should
> not be an empty string, this is unexpected. Can you use an non-empty string
> token?

agree, done

> Have you considered my suggestion of completely bailing out if no app id is
> explicitly set for the webapp container? I don’t think it makes sense to have
> apps that run in a "generic" container anyway, does it?

yes, I initially found it a bit drastic to bail out right away ... but after thinking
about it I think that it'll cause less confusion,

done

lp:~abreu-alexandre/webbrowser-app/fix-application-name-setup updated on 2016-05-09

1456. By Alexandre Abreu on 2016-05-09: Tweaks
1457. By Alexandre Abreu on 2016-05-09: Fail early when no app id is found

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-05-09:

#

In WebappContainer::initialize(), if the app id is empty, the error message is ambiguous:

« The application may not function properly and display some undesired behaviors.»

Considering that we are bailing out in that case, the application will not function at all :)

Can you update the message?

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-05-09:

#

Minor nit: in webbrowser-app.cpp, it would be better to pass the second argument to BrowserApplication::initialize() as QStringLiteral("webbrowser-app").

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2016-05-09:

#

> In WebappContainer::initialize(), if the app id is empty, the error message is
> ambiguous:
>
> « The application may not function properly and display some undesired
> behaviors.»
>
> Considering that we are bailing out in that case, the application will not
> function at all :)
>
> Can you update the message?

sorry, definitely, yes, done

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2016-05-09:

#

> Minor nit: in webbrowser-app.cpp, it would be better to pass the second
> argument to BrowserApplication::initialize() as QStringLiteral("webbrowser-
> app").

done

lp:~abreu-alexandre/webbrowser-app/fix-application-name-setup updated on 2016-05-09

1458. By Alexandre Abreu on 2016-05-09: Update bail out message when no app id is set; nit

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-05-10:

#

In the implementation of BrowserApplication::initialize(), there doesn’t seem to be a good reason for moving the calls to QCoreApplication::setApplicationName() and QCoreApplication::setOrganizationDomain(), can you revert that change? Because of that change a comment ("Ensure that application-specific data is written where it ought to.") is out of context.

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-05-10:

#

« // Take the app_name into account when creating the »

That comment appears to be incomplete.

review: Needs Fixing

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-05-10:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/486/rebuild

FAILED: Continuous integration, rev:1458
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/486/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build/488
    FAILURE: https://jenkins.canonical.com/system-apps/job/test-0-autopkgtest/label=phone-armhf,release=vivid+overlay,testname=default/42/console
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-0-fetch/488
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=vivid+overlay/487
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=xenial/487
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/494
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/494/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial/494
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial/494/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/494
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/494/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial/494
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial/494/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/494
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/494/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial/494
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial/494/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/486/rebuild

review: Needs Fixing (continuous-integration)

lp:~abreu-alexandre/webbrowser-app/fix-application-name-setup updated on 2016-05-10

1459. By Alexandre Abreu on 2016-05-10: fix comment; revert setappname call sites

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2016-05-10:

#

> In the implementation of BrowserApplication::initialize(), there doesn’t seem
> to be a good reason for moving the calls to
> QCoreApplication::setApplicationName() and
> QCoreApplication::setOrganizationDomain(), can you revert that change? Because
> of that change a comment ("Ensure that application-specific data is written
> where it ought to.") is out of context.

yes, a leftover, thank you done,

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2016-05-10:

#

> « // Take the app_name into account when creating the »
>
> That comment appears to be incomplete.

done

Revision history for this message

Olivier Tilloy (osomon) wrote on 2016-05-10:

#

LGTM now, thanks. I have tested on desktop but not on touch devices, so this will need to be tested from a silo to validate that the behaviour is as expected.

review: Approve

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-05-10:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/495/rebuild

FAILED: Continuous integration, rev:1459
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/495/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build/497
    FAILURE: https://jenkins.canonical.com/system-apps/job/test-0-autopkgtest/label=phone-armhf,release=vivid+overlay,testname=default/44/console
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-0-fetch/497
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=vivid+overlay/489
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=xenial/489
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/496
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/496/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial/496
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial/496/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/496
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/496/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial/496
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial/496/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/496
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/496/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial/496
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial/496/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-webbrowser-app-ci/495/rebuild

review: Needs Fixing (continuous-integration)

webbrowser-app

Merge lp:~abreu-alexandre/webbrowser-app/fix-application-name-setup into lp:webbrowser-app

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'src/app/browserapplication.cpp'
 --- src/app/browserapplication.cpp	2016-04-12 19:59:00 +0000
 +++ src/app/browserapplication.cpp	2016-05-10 14:22:12 +0000
@@ -95,16 +95,6 @@
      return host;
+ }
--QString BrowserApplication::appId() const
--{
--    Q_FOREACH(const QString& argument, m_arguments) {
--        if (argument.startsWith("--app-id=")) {
--            return argument.split("--app-id=")[1];
--        }
--    }
--    return QString();
--}
--
  #define MAKE_SINGLETON_FACTORY(type) \
      static QObject* type##_singleton_factory(QQmlEngine* engine, QJSEngine* scriptEngine) { \
          Q_UNUSED(engine); \
@@ -115,36 +105,54 @@
  MAKE_SINGLETON_FACTORY(MemInfo)
  MAKE_SINGLETON_FACTORY(MimeDatabase)
--
--bool BrowserApplication::initialize(const QString& qmlFileSubPath)
++bool BrowserApplication::initialize(const QString& qmlFileSubPath
++                                    , const QString& appId)
+ {
      Q_ASSERT(m_window == 0);
++    if (appId.isEmpty()) {
++        qCritical() << "Cannot initialize the runtime environment: "
++                       "no application id detected.";
++        return false;
++    }
++
      if (m_arguments.contains("--help") || m_arguments.contains("-h")) {
          printUsage();
          return false;
+     }
--    // Handle legacy platforms (i.e. current desktop versions, where
--    // applications are not started by the Ubuntu ApplicationManager).
--    if (qgetenv("APP_ID").isEmpty()) {
--        QString id = appId();
--        if (id.isEmpty()) {
--            id = QStringLiteral(APP_ID);
--        }
--        qputenv("APP_ID", id.toUtf8());
--    }
      // Ensure that application-specific data is written where it ought to.
--    QStringList appIdParts =
--        QString::fromUtf8(qgetenv("APP_ID")).split('_');
++    QStringList appIdParts = appId.split('_');
++
      QCoreApplication::setApplicationName(appIdParts.first());
      QCoreApplication::setOrganizationDomain(QCoreApplication::applicationName());
++
      // Get also the the first two components of the app ID: <package>_<app>,
      // which is needed by Online Accounts.
      QString unversionedAppId = QStringList(appIdParts.mid(0, 2)).join('_');
      // Ensure only one instance of the app is running.
--    if (m_singleton.run(m_arguments)) {
++    // For webapps using the container as a launcher, the predicate that
++    // is used to determine if this running instance is a duplicate of
++    // a running one, is based on the current APP_ID.
++    // The app id is formed as: <package name>_<app name>_<version>
++
++    // Where the <package name> is specified in the the manifest.json as
++    // "appName" and is specific for the whole click package.
++
++    // The <app name> portion is based on the desktop file name and is a short
++    // app name. This name is meaningful when more than one desktop file is
++    // found in a given click package.
++
++    // IMPORTANT:
++    // 1. When a click application contains more than one desktop file
++    // the bundle is considered a single app from the point of view of the
++    // cache and resource file locations. THOSE FILES ARE THEN SHARED between
++    // the instances.
++    // 2. To make sure that if more than one desktop file is found in a click package,
++    // those apps are not considered the same instance, the instance existance predicate
++    // is based on the <package name> AND the <app name> detailed above.
++    if (m_singleton.run(m_arguments, appId)) {
          connect(&m_singleton, SIGNAL(newInstanceLaunched(const QStringList&)),
                  SLOT(onNewInstanceLaunched(const QStringList&)));
      } else {
 === modified file 'src/app/browserapplication.h'
 --- src/app/browserapplication.h	2016-01-15 09:29:22 +0000
 +++ src/app/browserapplication.h	2016-05-10 14:22:12 +0000
@@ -45,7 +45,7 @@
      BrowserApplication(int& argc, char** argv);
      ~BrowserApplication();
--    bool initialize(const QString& qmlFileSubPath);
++    bool initialize(const QString& qmlFileSubPath, const QString& appId);
      int run();
  protected:
@@ -63,7 +63,6 @@
      void onNewInstanceLaunched(const QStringList& arguments) const;
  private:
--    QString appId() const;
      QString inspectorPort() const;
      QString inspectorHost() const;
 === modified file 'src/app/config.h.in'
 --- src/app/config.h.in	2015-05-14 05:57:01 +0000
 +++ src/app/config.h.in	2016-05-10 14:22:12 +0000
@@ -23,7 +23,6 @@
  #include <QtCore/QDir>
  #include <QtCore/QString>
--#define APP_ID "webbrowser-app"
  #define REMOTE_INSPECTOR_PORT 9221
  inline bool isRunningInstalled()
 === modified file 'src/app/single-instance-manager.cpp'
 --- src/app/single-instance-manager.cpp	2016-04-08 13:49:25 +0000
 +++ src/app/single-instance-manager.cpp	2016-05-10 14:22:12 +0000
@@ -33,12 +33,48 @@
  #include "single-instance-manager.h"
  namespace {
++
  const int kWaitForRunningInstanceToRespondMs = 1000;
  const int kWaitForRunningInstanceToAckMs = 1000;
  const int kDataStreamVersion = QDataStream::Qt_5_0;
  const QString kHeaderToken = QStringLiteral("MESSAGE");
  const QString kAckToken = QStringLiteral("ACK");
--}
++
++QString getProfilePathFromAppId(const QString& appId)
++{
++    QString profilePath =
++            QStandardPaths::writableLocation(QStandardPaths::DataLocation);
++
++    QStringList appIdParts = appId.split('_', QString::SkipEmptyParts);
++
++    QString appDesktopName;
++
++    // We try to get the "short app name" to try to uniquely identify
++    // the single instance profile path.
++
++    // In cases where you have a single click with multiple apps in it,
++    // the "app name" as defined in the click manifest.json file will be
++    // a proper way to distinguish a unique instance, it needs to take
++    // the desktop name into account.
++
++    // At the moment there is no clean way to get those click app name
++    // paths, see:
++    //  https://launchpad.net/bugs/1555542
++
++    if (appIdParts.size() >= 3) {
++        // Assume that we have a APP_ID that corresponds to:
++        // <manifest app name>_<desktop app name>_<version>
++        appDesktopName = appIdParts[1];
++    } else {
++        // We either run on desktop or as the webbrowser
++        appDesktopName = appIdParts.first();
++    }
++
++    return profilePath + QDir::separator() + appDesktopName;
++}
++
++}
++
  SingleInstanceManager::SingleInstanceManager(QObject* parent)
      : QObject(parent)
@@ -54,13 +90,13 @@
      return false;
+ }
--bool SingleInstanceManager::run(const QStringList& arguments)
++bool SingleInstanceManager::run(const QStringList& arguments, const QString& appId)
+ {
      if (m_server.isListening()) {
          return false;
+     }
--    QDir profile(QStandardPaths::writableLocation(QStandardPaths::DataLocation));
++    QDir profile(getProfilePathFromAppId(appId));
      if (!profile.exists()) {
          if (!QDir::root().mkpath(profile.absolutePath())) {
              qCritical() << "Failed to create profile directory,"
 === modified file 'src/app/single-instance-manager.h'
 --- src/app/single-instance-manager.h	2016-01-15 10:06:31 +0000
 +++ src/app/single-instance-manager.h	2016-05-10 14:22:12 +0000
@@ -33,7 +33,7 @@
  public:
      SingleInstanceManager(QObject* parent=nullptr);
--    bool run(const QStringList& arguments);
++    bool run(const QStringList& arguments, const QString& appId);
  Q_SIGNALS:
      void newInstanceLaunched(const QStringList& arguments) const;
 === modified file 'src/app/webbrowser/webbrowser-app.cpp'
 --- src/app/webbrowser/webbrowser-app.cpp	2016-02-05 15:20:10 +0000
 +++ src/app/webbrowser/webbrowser-app.cpp	2016-05-10 14:22:12 +0000
@@ -76,7 +76,7 @@
      qmlRegisterSingletonType<DownloadsModel>(uri, 0, 1, "DownloadsModel", DownloadsModel_singleton_factory);
      qmlRegisterType<TextSearchFilterModel>(uri, 0, 1, "TextSearchFilterModel");
--    if (BrowserApplication::initialize("webbrowser/webbrowser-app.qml")) {
++    if (BrowserApplication::initialize("webbrowser/webbrowser-app.qml", QStringLiteral("webbrowser-app"))) {
          QStringList searchEnginesSearchPaths;
          searchEnginesSearchPaths << QStandardPaths::writableLocation(QStandardPaths::DataLocation) + "/searchengines";
          searchEnginesSearchPaths << UbuntuBrowserDirectory() + "/webbrowser/searchengines";
 === modified file 'src/app/webcontainer/webapp-container.cpp'
 --- src/app/webcontainer/webapp-container.cpp	2016-04-25 17:35:53 +0000
 +++ src/app/webcontainer/webapp-container.cpp	2016-05-10 14:22:12 +0000
@@ -92,12 +92,38 @@
+ {
+ }
++QString WebappContainer::appId() const
++{
++    Q_FOREACH(const QString& argument, m_arguments) {
++        if (argument.startsWith("--app-id=")) {
++            return argument.split("--app-id=")[1];
++        }
++    }
++    return QString();
++}
  bool WebappContainer::initialize()
+ {
      earlyEnvironment();
--    if (BrowserApplication::initialize("webcontainer/webapp-container.qml")) {
++    if (qgetenv("APP_ID").isEmpty()) {
++        QString id = appId();
++        if (id.isEmpty()) {
++            qCritical() << "The application has been launched with no "
++                          "explicit or system provided app id. "
++                          "An application id can be set by using the --app-id "
++                          "command line parameter and setting it to a unique "
++                          "application specific value or using the APP_ID environment "
++                          "variable.";
++            return false;
++        }
++        qputenv("APP_ID", id.toUtf8());
++    }
++
++    if (BrowserApplication::initialize(
++                "webcontainer/webapp-container.qml",
++                QString::fromUtf8(qgetenv("APP_ID")))) {
++
          parseCommandLine();
          parseExtraConfiguration();
 === modified file 'src/app/webcontainer/webapp-container.h'
 --- src/app/webcontainer/webapp-container.h	2016-02-27 21:45:57 +0000
 +++ src/app/webcontainer/webapp-container.h	2016-05-10 14:22:12 +0000
@@ -54,6 +54,7 @@
      bool shouldNotValidateCommandLineUrls() const;
      bool isValidLocalIntentFilterFile(const QString& filename) const;
      void setupLocalSchemeFilterIfAny(QQmlContext* context, const QString& webappSearchPath);
++    QString appId() const;
  private:
      QString m_webappName;
 === modified file 'tests/autopilot/webapp_container/tests/__init__.py'
 --- tests/autopilot/webapp_container/tests/__init__.py	2016-01-22 10:19:34 +0000
 +++ tests/autopilot/webapp_container/tests/__init__.py	2016-05-10 14:22:12 +0000
@@ -57,6 +57,10 @@
              args.append(
                  '--desktop_file_hint=/usr/share/applications/'
                  'webbrowser-app.desktop')
++
++        if next(filter(lambda e: e.startswith('--appid'), args), None) is None:
++            args.append('--app-id=running.test')
++
          if envvars:
              for envvar_key in envvars:
                  self.useFixture(fixtures.EnvironmentVariable(
 === modified file 'tests/unittests/single-instance-manager/tst_SingleInstanceManagerTests.cpp'
 --- tests/unittests/single-instance-manager/tst_SingleInstanceManagerTests.cpp	2016-01-18 14:45:12 +0000
 +++ tests/unittests/single-instance-manager/tst_SingleInstanceManagerTests.cpp	2016-05-10 14:22:12 +0000
@@ -50,18 +50,18 @@
      void test_cannot_run_twice_same_instance()
+     {
--        QVERIFY(singleton->run(QStringList()));
--        QVERIFY(!singleton->run(QStringList()));
++        QVERIFY(singleton->run(QStringList(), "appid"));
++        QVERIFY(!singleton->run(QStringList(), "appid"));
          QVERIFY(newInstanceSpy->isEmpty());
+     }
      void test_arguments_passed_to_already_running_instance()
+     {
--        QVERIFY(singleton->run(QStringList()));
++        QVERIFY(singleton->run(QStringList(), "appid"));
          SingleInstanceManager other;
          QStringList args;
          args << QStringLiteral("foo") << QStringLiteral("bar") << QStringLiteral("baz");
--        QVERIFY(!other.run(args));
++        QVERIFY(!other.run(args, "appid"));
          newInstanceSpy->wait();
          QCOMPARE(newInstanceSpy->first().at(0).toStringList(), args);
+     }