Break out more manifest parsing logic from ShortcutHelper

Break out more manifest parsing logic from ShortcutHelper

* Pull out logic in ShortcutHelper for parsing shortcut properties
  into the ShortcutInfo struct.  This will be used by AppBannerManager.

* Passes ShortcutInfo structs around instead of 4 different parameters
  when creating shortcuts.

BUG=453170
Committed: https://crrev.com/16e84de00c5b15908530026a4c12b43e900d1578
Cr-Commit-Position: refs/heads/master@{#314415}

[gone, 2015-02-03 01:17:10]

dfalcantara@chromium.org changed reviewers:
+ mlamouri@chromium.org

[gone, 2015-02-03 01:17:11]

Pulling out more logic.

[mlamouri (slow - plz ping), 2015-02-03 17:40:24]

lgtm, thanks!

https://codereview.chromium.org/899543003/diff/20001/chrome/browser/android/b...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/899543003/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:278:
info.UpdateFromManifest(manifest);
You might want to set some default values like ShortcutHelper does? I understand
that some values like |name| MUST be present in order to trigger the app banner.
Could you add a TODO?

https://codereview.chromium.org/899543003/diff/20001/chrome/browser/android/s...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/899543003/diff/20001/chrome/browser/android/s...
chrome/browser/android/shortcut_info.cc:10: #include "url/gurl.h"
You probably don't need those #include, they are in the header already.

[gone, 2015-02-03 19:48:22]

dfalcantara@chromium.org changed reviewers:
+ newt@chromium.org

[gone, 2015-02-03 19:48:23]

newt@ for OWNERS again.

https://chromiumcodereview.appspot.com/899543003/diff/20001/chrome/browser/an...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/899543003/diff/20001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:278:
info.UpdateFromManifest(manifest);
On 2015/02/03 17:40:24, Mounir Lamouri wrote:
> You might want to set some default values like ShortcutHelper does? I
understand
> that some values like |name| MUST be present in order to trigger the app
banner.
> Could you add a TODO?

Changed a bit of the logic to prevent the infobar from appearing without a title
or URL.

https://chromiumcodereview.appspot.com/899543003/diff/20001/chrome/browser/an...
File chrome/browser/android/shortcut_info.cc (right):

https://chromiumcodereview.appspot.com/899543003/diff/20001/chrome/browser/an...
chrome/browser/android/shortcut_info.cc:10: #include "url/gurl.h"
On 2015/02/03 17:40:24, Mounir Lamouri wrote:
> You probably don't need those #include, they are in the header already.

Done.

[newt (away), 2015-02-03 19:56:47]

lgtm

[gone, 2015-02-03 20:01:17]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2015-02-03 20:02:05]

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

[commit-bot: I haz the power, 2015-02-03 20:48:49]

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

[commit-bot: I haz the power, 2015-02-03 20:48:50]

Failed to apply patch for chrome/browser/android/banners/app_banner_manager.cc:
While running git apply --index -3 -p1;
  error: patch failed: chrome/browser/android/banners/app_banner_manager.cc:9
  Falling back to three-way merge...
  Applied patch to 'chrome/browser/android/banners/app_banner_manager.cc' with
conflicts.
  U chrome/browser/android/banners/app_banner_manager.cc

Patch:       chrome/browser/android/banners/app_banner_manager.cc
Index: chrome/browser/android/banners/app_banner_manager.cc
diff --git a/chrome/browser/android/banners/app_banner_manager.cc
b/chrome/browser/android/banners/app_banner_manager.cc
index
316a036d4277724cd41664d716715645e759c14c..6b43fe062e7e294d0e827209012f2bc263a11087
100644
--- a/chrome/browser/android/banners/app_banner_manager.cc
+++ b/chrome/browser/android/banners/app_banner_manager.cc
@@ -9,9 +9,12 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
+#include "base/threading/worker_pool.h"
 #include "chrome/browser/android/banners/app_banner_infobar_delegate.h"
 #include "chrome/browser/android/banners/app_banner_metrics_ids.h"
 #include "chrome/browser/android/banners/app_banner_utilities.h"
+#include "chrome/browser/android/shortcut_helper.h"
+#include "chrome/browser/android/shortcut_info.h"
 #include "chrome/browser/banners/app_banner_settings_helper.h"
 #include "chrome/browser/bitmap_fetcher/bitmap_fetcher.h"
 #include "chrome/browser/infobars/infobar_service.h"
@@ -75,7 +78,7 @@ void AppBannerManager::Install() const {
     return;
 
   if (!manifest_.IsEmpty()) {
-    // TODO(dfalcantara): Trigger shortcut creation.
+    InstallManifestApp(manifest_, *app_icon_.get());
   }
 }
 
@@ -125,8 +128,10 @@ void AppBannerManager::OnDidGetManifest(const
content::Manifest& manifest) {
   if (web_contents()->IsBeingDestroyed())
     return;
 
-  if (manifest.IsEmpty()) {
-    // No manifest, see if there is a play store meta tag.
+  if (manifest.IsEmpty()
+      || !manifest.start_url.is_valid()
+      || (manifest.name.is_null() && manifest.short_name.is_null())) {
+    // No usable manifest, see if there is a play store meta tag.
     Send(new ChromeViewMsg_RetrieveMetaTagContent(routing_id(),
                                                   validated_url_,
                                                   kBannerTag));
@@ -268,6 +273,20 @@ int AppBannerManager::GetPreferredIconSize() {
   return Java_AppBannerManager_getPreferredIconSize(env, jobj.obj());
 }
 
+// static
+void AppBannerManager::InstallManifestApp(const content::Manifest& manifest,
+                                          const SkBitmap& icon) {
+  ShortcutInfo info;
+  info.UpdateFromManifest(manifest);
+
+  base::WorkerPool::PostTask(
+      FROM_HERE,
+      base::Bind(&ShortcutHelper::AddShortcutInBackgroundWithSkBitmap,
+                 info,
+                 icon),
+      true);
+}
+
 void RecordDismissEvent(JNIEnv* env, jclass clazz, jint metric) {
   banners::TrackDismissEvent(metric);
 }

[commit-bot: I haz the power, 2015-02-03 20:48:56]

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

[gone, 2015-02-03 21:02:17]

New patchsets have been uploaded after l-g-t-m from newt@chromium.org

[gone, 2015-02-03 21:02:24]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2015-02-03 21:03:38]

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

[commit-bot: I haz the power, 2015-02-03 22:08:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-03 22:09:11]

Patchset 4 (id:??) landed as
https://crrev.com/16e84de00c5b15908530026a4c12b43e900d1578
Cr-Commit-Position: refs/heads/master@{#314415}