Implement syncing of bookmark apps.

Implement syncing of bookmark apps.

This CL adds syncing of bookmark apps. This involves syncing two extra fields,
which are the bookmark app url and description. These are not populated for
non-bookmark apps. The bookmark app name is obtained from the extension sync
data's name field.

Bookmark apps that are installled from sync will not have any icon. Updating
the icon and possibly fetching it on sync will be implemented in a future CL.
Bookmark apps that are updated with a new name will keep their icons.

This CL also adds a GetWebApplicationInfoFromBookmarkApp() function that
returns a ready-to-install WebApplicationInfo that is populated with the app's
details and icons.

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

[benwells, 2014-04-11 23:15:56]

c/b/e lgtm

https://codereview.chromium.org/229553003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/229553003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:376:
app_sync_data.extension_sync_data().name() &&
Was this git cl formatted?

[calamity, 2014-04-14 01:12:02]

+zea for */sync/* OWNERS.

https://codereview.chromium.org/229553003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/229553003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:376:
app_sync_data.extension_sync_data().name() &&
On 2014/04/11 23:15:56, benwells wrote:
> Was this git cl formatted?

Yep... That _is_ pretty crazy-lookin' though.

[benwells, 2014-04-14 06:19:27]

https://codereview.chromium.org/229553003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/229553003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:376:
app_sync_data.extension_sync_data().name() &&
On 2014/04/14 01:12:03, calamity wrote:
> On 2014/04/11 23:15:56, benwells wrote:
> > Was this git cl formatted?
> 
> Yep... That _is_ pretty crazy-lookin' though.

I think it is 4 spaces indented from the thing being compared against. Wowzers.

[Nicolas Zea, 2014-04-14 21:46:11]

https://codereview.chromium.org/229553003/diff/20001/sync/protocol/app_specif...
File sync/protocol/app_specifics.proto (right):

https://codereview.chromium.org/229553003/diff/20001/sync/protocol/app_specif...
sync/protocol/app_specifics.proto:77: optional string bookmark_app_description =
7;
Is this something that will change with different versions of the app? Should it
just be fetched out of band with the extension itself?

[calamity, 2014-04-15 01:06:04]

https://codereview.chromium.org/229553003/diff/20001/sync/protocol/app_specif...
File sync/protocol/app_specifics.proto (right):

https://codereview.chromium.org/229553003/diff/20001/sync/protocol/app_specif...
sync/protocol/app_specifics.proto:77: optional string bookmark_app_description =
7;
On 2014/04/14 21:46:11, Nicolas Zea wrote:
> Is this something that will change with different versions of the app? Should
it
> just be fetched out of band with the extension itself?

Hmm. That's a good point. I was also thinking of syncing icon URLs and creating
a WebContents to fetch the icons on down-sync. I suppose I could fetch the whole
page and then get the icons and descriptions out of that.

I've sent an email to meacer@ asking if there would be security concerns there.

[calamity, 2014-04-16 01:41:50]

On 2014/04/15 01:06:04, calamity wrote:
>
https://codereview.chromium.org/229553003/diff/20001/sync/protocol/app_specif...
> File sync/protocol/app_specifics.proto (right):
> 
>
https://codereview.chromium.org/229553003/diff/20001/sync/protocol/app_specif...
> sync/protocol/app_specifics.proto:77: optional string bookmark_app_description
=
> 7;
> On 2014/04/14 21:46:11, Nicolas Zea wrote:
> > Is this something that will change with different versions of the app?
Should
> it
> > just be fetched out of band with the extension itself?
> 
> Hmm. That's a good point. I was also thinking of syncing icon URLs and
creating
> a WebContents to fetch the icons on down-sync. I suppose I could fetch the
whole
> page and then get the icons and descriptions out of that.
> 
> I've sent an email to meacer@ asking if there would be security concerns
there.

meacer@ seems to be happy with this approach. The description would be updated
when
the app is updated which I plan to be every time the app is launched. If that
description
has changed, it would be synced.

WDYT? Any thoughts on the plan to sync favicon URLs and then fetch them at
down-sync time?

[Nicolas Zea, 2014-04-16 19:06:04]

The problem with this approach is that these kind of automatic updates triggered
without user intent can lead to broken conflict resolution.

Is it possible for the app to launch automatically? If so, that means that if
the user deletes the app on one machine, attempts to sync that deletion, and
then launches chrome on another machine, the launch of the app on that second
machine will trigger an undeletion of the app (if the description had changed).
We've run into this with extensions due to syncing the extension name/version,
which can update on its own without user involvement.

Does this sound like it would be an issue for you?

[calamity, 2014-04-22 01:13:26]

On 2014/04/16 19:06:04, Nicolas Zea wrote:
> The problem with this approach is that these kind of automatic updates
triggered
> without user intent can lead to broken conflict resolution.
> 
> Is it possible for the app to launch automatically? If so, that means that if
> the user deletes the app on one machine, attempts to sync that deletion, and
> then launches chrome on another machine, the launch of the app on that second
> machine will trigger an undeletion of the app (if the description had
changed).
> We've run into this with extensions due to syncing the extension name/version,
> which can update on its own without user involvement.
> 
> Does this sound like it would be an issue for you?

The apps aren't supposed to launch automatically so I don't think this is an
issue. If this becomes a problem, we can check whether the app is still 
installed at the end of the update flow in order to prevent an update from
reinstalling an uninstalled app.

[Nicolas Zea, 2014-04-22 17:43:31]

lgtm

[calamity, 2014-04-23 05:19:33]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-04-23 05:20:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/229553003/20001

[commit-bot: I haz the power, 2014-04-23 05:20:19]

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

[commit-bot: I haz the power, 2014-04-23 05:20:19]

Failed to apply patch for chrome/browser/extensions/extension_sync_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_sync_service.cc
  Hunk #1 FAILED at 7.
  Hunk #2 succeeded at 17 (offset -1 lines).
  Hunk #3 succeeded at 341 (offset -1 lines).
  Hunk #4 succeeded at 354 (offset -1 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_sync_service.cc.rej

Patch:       chrome/browser/extensions/extension_sync_service.cc
Index: chrome/browser/extensions/extension_sync_service.cc
diff --git a/chrome/browser/extensions/extension_sync_service.cc
b/chrome/browser/extensions/extension_sync_service.cc
index
472c5d5e751b8683a43fd91ddeffc78ae8a47e07..22801d1dad20fce03c578faa9023baef1ca04d16
100644
--- a/chrome/browser/extensions/extension_sync_service.cc
+++ b/chrome/browser/extensions/extension_sync_service.cc
@@ -7,9 +7,12 @@
 #include <iterator>
 
 #include "base/basictypes.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/extensions/app_sync_data.h"
+#include "chrome/browser/extensions/bookmark_app_helper.h"
+#include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_error_ui.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_sync_data.h"
@@ -18,22 +21,46 @@
 #include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/glue/sync_start_util.h"
+#include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
+#include "chrome/common/extensions/manifest_handlers/icons_handler.h"
 #include "chrome/common/extensions/sync_helper.h"
+#include "chrome/common/web_application_info.h"
 #include "components/sync_driver/sync_prefs.h"
 #include "extensions/browser/app_sorting.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/extension_icon_set.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest_constants.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory.h"
+#include "ui/gfx/image/image_family.h"
 
 using extensions::Extension;
 using extensions::ExtensionPrefs;
 using extensions::ExtensionRegistry;
 using extensions::FeatureSwitch;
 
+namespace {
+
+void OnWebApplicationInfoLoaded(
+    WebApplicationInfo synced_info,
+    base::WeakPtr<ExtensionService> extension_service,
+    const WebApplicationInfo& loaded_info) {
+  DCHECK_EQ(synced_info.app_url, loaded_info.app_url);
+
+  if (!extension_service)
+    return;
+
+  // Use the old icons if they exist.
+  synced_info.icons = loaded_info.icons;
+  CreateOrUpdateBookmarkApp(extension_service.get(), synced_info);
+}
+
+}  // namespace
+
 ExtensionSyncService::ExtensionSyncService(Profile* profile,
                                            ExtensionPrefs* extension_prefs,
                                            ExtensionService* extension_service)
@@ -318,6 +345,9 @@ bool ExtensionSyncService::ProcessAppSyncData(
                               app_sync_data.launch_type());
   }
 
+  if (!app_sync_data.bookmark_app_url().empty())
+    ProcessBookmarkAppSyncData(app_sync_data);
+
   if (!ProcessExtensionSyncDataHelper(app_sync_data.extension_sync_data(),
                                       syncer::APPS)) {
     app_sync_bundle_.AddPendingApp(id, app_sync_data);
@@ -328,6 +358,46 @@ bool ExtensionSyncService::ProcessAppSyncData(
   return true;
 }
 
+void ExtensionSyncService::ProcessBookmarkAppSyncData(
+    const extensions::AppSyncData& app_sync_data) {
+  // Process bookmark app sync if necessary.
+  GURL bookmark_app_url(app_sync_data.bookmark_app_url());
+  if (!bookmark_app_url.is_valid() ||
+      app_sync_data.extension_sync_data().uninstalled()) {
+    return;
+  }
+
+  const extensions::Extension* extension =
+      extension_service_->GetInstalledExtension(
+          app_sync_data.extension_sync_data().id());
+
+  // Return if there are no bookmark app details that need updating.
+  if (extension && extension->non_localized_name() ==
+                       app_sync_data.extension_sync_data().name() &&
+      extension->description() == app_sync_data.bookmark_app_description()) {
+    return;
+  }
+
+  WebApplicationInfo web_app_info;
+  web_app_info.app_url = bookmark_app_url;
+  web_app_info.title =
+      base::UTF8ToUTF16(app_sync_data.extension_sync_data().name());
+  web_app_info.description =
+      base::UTF8ToUTF16(app_sync_data.bookmark_app_description());
+
+  // If the bookmark app already exists, keep the old icons.
+  if (!extension) {
+    CreateOrUpdateBookmarkApp(extension_service_, web_app_info);
+  } else {
+    app_sync_data.extension_sync_data().name();
+    GetWebApplicationInfoFromApp(profile_,
+                                 extension,
+                                 base::Bind(&OnWebApplicationInfoLoaded,
+                                            web_app_info,
+                                            extension_service_->AsWeakPtr()));
+  }
+}
+
 void ExtensionSyncService::SyncOrderingChange(const std::string& extension_id)
{
   const extensions::Extension* ext = extension_service_->GetInstalledExtension(
       extension_id);

[calamity, 2014-04-23 14:00:33]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-04-23 14:00:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/229553003/40001

[commit-bot: I haz the power, 2014-04-23 14:42:38]

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

[commit-bot: I haz the power, 2014-04-23 14:42:38]

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

[calamity, 2014-04-24 03:57:13]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-04-24 03:57:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/229553003/60001

[commit-bot: I haz the power, 2014-04-24 05:15:17]

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

[commit-bot: I haz the power, 2014-04-24 05:15:18]

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

[calamity, 2014-04-24 05:46:25]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-04-24 05:47:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/229553003/60001

[commit-bot: I haz the power, 2014-04-24 08:42:47]

Change committed as 265887