Pass in extra parameters to WebApkBuilder#buildWebApkAsync()

Pass in extra parameters to WebApkBuilder#buildWebApkAsync()

This CL:
- Passes in the same parameters to WebApkBuilder#buildWebApkAsync() as those
  passed to ShortcutHelper#createWebappShortcutIntent(). This is necessary
  because WebAPKs and Web apps use the same splash screen rendering logic.
- Passes in the Web Manifest URL and the icon URL to
  WebApkBuilder#buildWebApkAsync(). If the Chrome server has previously
  generated a WebAPK for a (manifestUrl, iconURL) pair, it is possible to
  download a WebAPK for the (manifestUrl, iconURL) pair from the Chrome server
  without uploading the icon bitmap to the server.

BUG=619739
Committed: https://crrev.com/b8c25a180344260005e9ecd261dd594b84d9c903
Cr-Commit-Position: refs/heads/master@{#403523}

[pkotwicz, 2016-06-13 23:33:45]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2016-06-13 23:34:01]

pkotwicz@chromium.org changed reviewers:
+ hanxi@chromium.org

[pkotwicz, 2016-06-13 23:34:01]

Xi, can you please take a look?

[Xi Han, 2016-06-15 14:09:35]

Overall looks good to me, thanks for putting these together!

https://codereview.chromium.org/2064943002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2064943002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:158:
iconUrl, icon, displayMode, orientation, themeColor, backgroundColor,
Do we need to pass all these parameters to the server side? Server will parse
parameters like displayMode etc. from the manifest, right? Is these parameters
prepared for one-off APK?

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right):

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:74:
Java_AddToHomescreenDialogHelper_onIconAvailable(env, java_ref_.obj(),
Revert changes here?

[pkotwicz, 2016-06-15 14:19:13]

Xi can you please take another look?

https://codereview.chromium.org/2064943002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2064943002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:158:
iconUrl, icon, displayMode, orientation, themeColor, backgroundColor,
Yes parameters like displayMode are for OneOffWebAPks

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right):

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:74:
Java_AddToHomescreenDialogHelper_onIconAvailable(env, java_ref_.obj(),
I think that my change fixes the indentation in
Java_AddToHomescreenDialogHelper_onIconAvailable()

I know that I am a bit of a hypocrite here because I dislike unrelated changes
to the CL.

[Xi Han, 2016-06-15 14:32:46]

lgtm

[Xi Han, 2016-06-15 17:19:07]

On 2016/06/15 14:32:46, Xi Han wrote:
> lgtm

Please update the "Review-Url" in the CL description if needed. It doesn't seem
to be a correct one.

[pkotwicz, 2016-06-15 17:51:48]

Description was changed from

==========
Pass in extra parameters to WebApkBuilder#buildWebApkAsync()

This CL:
- Passes in the same parameters to WebApkBuilder#buildWebApkAsync() as those
  passed to ShortcutHelper#createWebappShortcutIntent(). This is necessary
  because WebAPKs and Web apps use the same splash screen rendering logic.
- Passes in the Web Manifest URL and the icon URL to
  WebApkBuilder#buildWebApkAsync(). If the Chrome server has previously
  generated a WebAPK for a (manifestUrl, iconURL) pair, it is possible to
  download a WebAPK for the (manifestUrl, iconURL) pair from the Chrome server
  without uploading the icon bitmap to the server.

BUG=619739

Review-Url: https://codereview.chromium.org/2048133004
Cr-Commit-Position: refs/heads/master@{#398925}
==========

to

==========
Pass in extra parameters to WebApkBuilder#buildWebApkAsync()

This CL:
- Passes in the same parameters to WebApkBuilder#buildWebApkAsync() as those
  passed to ShortcutHelper#createWebappShortcutIntent(). This is necessary
  because WebAPKs and Web apps use the same splash screen rendering logic.
- Passes in the Web Manifest URL and the icon URL to
  WebApkBuilder#buildWebApkAsync(). If the Chrome server has previously
  generated a WebAPK for a (manifestUrl, iconURL) pair, it is possible to
  download a WebAPK for the (manifestUrl, iconURL) pair from the Chrome server
  without uploading the icon bitmap to the server.

BUG=619739
==========

[pkotwicz, 2016-06-15 17:56:15]

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

[pkotwicz, 2016-06-15 17:56:15]

dfalcantara@ can you please take a look?

For context, I am in the process of writing the server calls to create a WebAPK.

[gone, 2016-06-15 17:57:53]

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

[gone, 2016-06-15 17:57:54]

This is mucking with ShortcutHelper.  Adding dom and mounir.

[dominickn, 2016-06-15 19:17:15]

Very quick pass through. Is there anywhere we can see
WebApkBuilder#buildWebApkAsync() for context?

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

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:23: if (manifest.url.is_valid())
Perhaps this should be checked in the manifest parser / manager? It's hard to
imagine that we could get all the way here with an invalid manifest URL.

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
File chrome/browser/android/shortcut_info.h (right):

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_info.h:53: bool is_icon_generated;
It looks like this variable isn't used any more. Can it be deleted?

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_data_fetcher.h (right):

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:168: virtual void
ShowBanner(const GURL& icon_url,
This needs to also fix up AppBannerDataFetcherDesktop::ShowBanner

[pkotwicz, 2016-06-15 20:30:57]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2016-06-15 20:34:24]

dominickn@ can you please take another look?

There is nowhere that you can see the implementation of
WebApkBuilder#buildWebApkAsync() for context.

Locally, I have made WebApkBuilder#buildWebApkAsync() send an intent to the
minter_apk test app which is checked into the Chrome internal repository.
I am in the process of making WebApkBuilder#buildWebApkAsync() do server calls.
I can send you an e-mail with the protobuf definition if you want

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

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:23: if (manifest.url.is_valid())
I removed the check. You are right that manifest.url should always be valid. (It
should not be possible to download the manifest from an invalid URL)

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
File chrome/browser/android/shortcut_info.h (right):

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_info.h:53: bool is_icon_generated;
On 2016/06/15 19:17:14, dominickn wrote:
> It looks like this variable isn't used any more. Can it be deleted?

Done.

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_data_fetcher.h (right):

https://codereview.chromium.org/2064943002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.h:168: virtual void
ShowBanner(const GURL& icon_url,
On 2016/06/15 19:17:14, dominickn wrote:
> This needs to also fix up AppBannerDataFetcherDesktop::ShowBanner

Done.

[dominickn, 2016-06-16 21:16:36]

https://codereview.chromium.org/2064943002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2064943002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:245: 
Instead of passing icon_url through OnManifestIconFetched() and
NotifyObserver(), can shortcut_icon_url_ be set in CreateLauncherIcon() and
OnManifestIconFetched() since this is all on the same object?

This also makes it clearer that icon_url is plumbed through properly from
FetchFavicon()

https://codereview.chromium.org/2064943002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):

https://codereview.chromium.org/2064943002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:121: GURL
shortcut_icon_url_;
Instead of this being a member variable on the fetcher, can it be added to
ShortcutInfo in place of is_icon_generated?

https://codereview.chromium.org/2064943002/diff/60001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_data_fetcher.cc (right):

https://codereview.chromium.org/2064943002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.cc:405: app_icon_url_ = icon_url;
Can this be done in FetchAppIcon(), so that it doesn't need to be passed through
Bind() and into this method's signature? I think either the code downloads the
icon successfully and reaches here, or or it fails and Cancel() gets called to
stop the pipeline anyway.

[pkotwicz, 2016-06-16 22:25:21]

Patchset #3 (id:80001) has been deleted

[pkotwicz, 2016-06-16 22:26:15]

dominickn@ can you please take another look? I think that I have addressed all
of your comments.

[pkotwicz, 2016-06-16 22:26:16]

dominickn@ can you please take another look? I think that I have addressed all
of your comments.

[dominickn, 2016-06-17 07:55:00]

lgtm

[pkotwicz, 2016-06-17 14:39:32]

mlamouri@ can you please take a look at the changes in content/

[pkotwicz, 2016-06-23 17:55:22]

mlamouri@ can you please take a look at the changes in content/

[pkotwicz, 2016-06-24 21:53:55]

mlamouri@ Ping!

[pkotwicz, 2016-06-24 21:53:57]

mlamouri@ Ping!

[mlamouri (slow - plz ping), 2016-06-27 12:52:33]

Sorry for the delay :(

Did you consider having the url added in the response instead of adding it to
content::Manifest? I would prefer if content::Manifest was a pure representation
of the Manifest format as spec'd.

It sounds like we could reach the same goals by having GetManifest() return an
URL with a content::Manifest object. It's actually what your code does on the
renderer side: the manifest is parsed and you attach the URL from the request to
the object later on.

WDYT?

https://codereview.chromium.org/2064943002/diff/100001/content/public/common/...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2064943002/diff/100001/content/public/common/...
content/public/common/manifest.h:76: // The URL of the manifest file. Null if
the parsing failed.
Null if the parsing failed?

[pkotwicz, 2016-06-27 18:40:53]

mlamouri@ can you please take another look?
I have removed the "manifest URL" from content::Manifest as per your request

[mlamouri (slow - plz ping), 2016-06-28 13:26:44]

pkotwicz@, that was meant as a suggestion. The question wasn't a rhetorical
question and I really wanted to engage the conversation. This said, the change
lgtm as is.

Thank you for working on this. This is super exciting! :)

[pkotwicz, 2016-06-28 14:10:46]

pkotwicz@chromium.org changed reviewers:
+ sievers@chromium.org

[pkotwicz, 2016-06-28 14:10:47]

sievers@ can you please take a look at the changes in content/ (except for
content/browser/manifest which mlamouri@ already looked at)

mlamouri@: I liked your suggestion hence I implemented it. I will try to write
longer replies to review comments in the future

[no sievers, 2016-06-28 22:47:48]

On 2016/06/28 14:10:47, pkotwicz wrote:
> sievers@ can you please take a look at the changes in content/ (except for
> content/browser/manifest which mlamouri@ already looked at)
> 
> mlamouri@: I liked your suggestion hence I implemented it. I will try to write
> longer replies to review comments in the future

lgtm

[gone, 2016-06-29 16:08:21]

OWNERs lgtm

[pkotwicz, 2016-06-29 21:54:08]

dfalcantara@ for OWNERS rubberstamp

[gone, 2016-06-29 21:54:34]

Already gave it to you, right above.

[pkotwicz, 2016-06-29 22:07:39]

dfalcantara@ just saw it :) Thanks!

[pkotwicz, 2016-06-29 22:12:58]

Patchset #5 (id:140001) has been deleted

[pkotwicz, 2016-06-29 22:15:37]

Patchset #5 (id:160001) has been deleted

[pkotwicz, 2016-06-29 22:28:57]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-06-29 22:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org,
dominickn@chromium.org, mlamouri@chromium.org, sievers@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2064943002/#ps180001
(title: "Merge branch 'master' into webapk_manifest000")

[commit-bot: I haz the power, 2016-06-29 22:30:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 22:50:55]

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

[commit-bot: I haz the power, 2016-06-29 22:50:57]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pkotwicz, 2016-06-30 00:10:06]

pkotwicz@chromium.org changed reviewers:
+ rsesek@chromium.org

[pkotwicz, 2016-06-30 00:10:07]

Robert can you please take a look at the changes in
	content/common/manifest_manager_messages.h

benwells@, can you please take a look at the changes in
	chrome/browser/extensions

[pkotwicz, 2016-06-30 00:10:56]

pkotwicz@chromium.org changed reviewers:
+ benwells@chromium.org

[pkotwicz, 2016-06-30 15:43:33]

pkotwicz@chromium.org changed reviewers:
+ sky@chromium.org

[pkotwicz, 2016-06-30 15:44:06]

Robert can you please take a look at the changes in
content/common/manifest_manager_messages.h

sky@ can you please take a look at the changes in chrome/browser/extensions
(benwells@ is OOO)

[Robert Sesek, 2016-06-30 16:56:35]

_message lgtm

[pkotwicz, 2016-06-30 18:08:24]

sky@ can you please take a look at the changes in chrome/browser/extensions
(benwells@ is OOO)

[sky, 2016-06-30 21:06:33]

LGTM

[pkotwicz, 2016-06-30 22:12:38]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-06-30 22:12:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, hanxi@chromium.org, mlamouri@chromium.org,
sievers@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2064943002/#ps200001
(title: "Merge branch 'master' into webapk_manifest000")

[commit-bot: I haz the power, 2016-06-30 22:13:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 22:34:03]

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

[commit-bot: I haz the power, 2016-06-30 22:34:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2016-07-01 20:00:03]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-07-01 20:00:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, sky@chromium.org, mlamouri@chromium.org,
sievers@chromium.org, rsesek@chromium.org, hanxi@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2064943002/#ps220001
(title: "Merge branch 'master' into webapk_manifest000")

[commit-bot: I haz the power, 2016-07-01 20:00:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 20:55:10]

Description was changed from

==========
Pass in extra parameters to WebApkBuilder#buildWebApkAsync()

This CL:
- Passes in the same parameters to WebApkBuilder#buildWebApkAsync() as those
  passed to ShortcutHelper#createWebappShortcutIntent(). This is necessary
  because WebAPKs and Web apps use the same splash screen rendering logic.
- Passes in the Web Manifest URL and the icon URL to
  WebApkBuilder#buildWebApkAsync(). If the Chrome server has previously
  generated a WebAPK for a (manifestUrl, iconURL) pair, it is possible to
  download a WebAPK for the (manifestUrl, iconURL) pair from the Chrome server
  without uploading the icon bitmap to the server.

BUG=619739
==========

to

==========
Pass in extra parameters to WebApkBuilder#buildWebApkAsync()

This CL:
- Passes in the same parameters to WebApkBuilder#buildWebApkAsync() as those
  passed to ShortcutHelper#createWebappShortcutIntent(). This is necessary
  because WebAPKs and Web apps use the same splash screen rendering logic.
- Passes in the Web Manifest URL and the icon URL to
  WebApkBuilder#buildWebApkAsync(). If the Chrome server has previously
  generated a WebAPK for a (manifestUrl, iconURL) pair, it is possible to
  download a WebAPK for the (manifestUrl, iconURL) pair from the Chrome server
  without uploading the icon bitmap to the server.

BUG=619739
==========

[commit-bot: I haz the power, 2016-07-01 20:55:12]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-07-01 20:57:00]

Description was changed from

==========
Pass in extra parameters to WebApkBuilder#buildWebApkAsync()

This CL:
- Passes in the same parameters to WebApkBuilder#buildWebApkAsync() as those
  passed to ShortcutHelper#createWebappShortcutIntent(). This is necessary
  because WebAPKs and Web apps use the same splash screen rendering logic.
- Passes in the Web Manifest URL and the icon URL to
  WebApkBuilder#buildWebApkAsync(). If the Chrome server has previously
  generated a WebAPK for a (manifestUrl, iconURL) pair, it is possible to
  download a WebAPK for the (manifestUrl, iconURL) pair from the Chrome server
  without uploading the icon bitmap to the server.

BUG=619739
==========

to

==========
Pass in extra parameters to WebApkBuilder#buildWebApkAsync()

This CL:
- Passes in the same parameters to WebApkBuilder#buildWebApkAsync() as those
  passed to ShortcutHelper#createWebappShortcutIntent(). This is necessary
  because WebAPKs and Web apps use the same splash screen rendering logic.
- Passes in the Web Manifest URL and the icon URL to
  WebApkBuilder#buildWebApkAsync(). If the Chrome server has previously
  generated a WebAPK for a (manifestUrl, iconURL) pair, it is possible to
  download a WebAPK for the (manifestUrl, iconURL) pair from the Chrome server
  without uploading the icon bitmap to the server.

BUG=619739

Committed: https://crrev.com/b8c25a180344260005e9ecd261dd594b84d9c903
Cr-Commit-Position: refs/heads/master@{#403523}
==========

[commit-bot: I haz the power, 2016-07-01 20:57:02]

Patchset 7 (id:??) landed as
https://crrev.com/b8c25a180344260005e9ecd261dd594b84d9c903
Cr-Commit-Position: refs/heads/master@{#403523}