Make it clear that AddToHomescreenDataFetcher ignores WebApplicationInfo for WebAPK compatible pages

Make it clear that AddToHomescreenDataFetcher ignores WebApplicationInfo for WebAPK compatible pages

This CL:
- Changes AddToHomescreenDataFetcher to make it clearer that WebApplicationInfo
  is ignored for WebAPK compatible pages.
- Adds tests checking that WebApplicationInfo is ignored

BUG=638468
Committed: https://crrev.com/0749dfd34ea2aba53f29bb83c0d19d2ea825b6f9
Cr-Commit-Position: refs/heads/master@{#421531}

[pkotwicz, 2016-09-23 15:36:09]

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

[pkotwicz, 2016-09-23 15:36:10]

Xi, can you please take a look?

This CL depends on https://codereview.chromium.org/2354363007/

[Xi Han, 2016-09-26 14:56:51]

One silly question: why we don't let WebAPK support application-info? Is there
any other reason except simplicity?

[pkotwicz, 2016-09-26 17:05:38]

We don't let WebAPKs support application-info because the WAM server does not
know about the document URL. The number of unique "document URL <-> Web Manifest
URL mappings" is much greater than the number of "Web Manifest URLs"

[Xi Han, 2016-09-26 17:48:12]

Thanks for the explanation, yes, it makes sense to me now:)

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (left):

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:117:
web_app_info.description.substr(0, chrome::kMaxMetaTagAttributeLength);
Doesn't regular web app still need this?

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
(right):

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:289:
fetcher->OnDidGetWebApplicationInfo(web_application_info);
Once when a fetcher is created by BuildFetcher(), it starts fetching and will
call OnDidGetWebApplicationInfo() when an IPC is received. My question is: is
that true that you never get the IPC which got OnDidGetWebApplicationInfo()
called automatically? If that is the case, it is safe to call
fetcher->OnDidGetWebApplicationInfo() explicitly.

[pkotwicz, 2016-09-26 20:13:47]

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (left):

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:117:
web_app_info.description.substr(0, chrome::kMaxMetaTagAttributeLength);
|web_app_info| is a variable which is local to the OnDidGetWebApplicationInfo()
function. I can't find where the description is used.

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
(right):

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:289:
fetcher->OnDidGetWebApplicationInfo(web_application_info);
Tests with use TestWebContents do not create renderers at all.

My understanding is that renderers are only created as a result of a browser
navigation. TestWebContents stubs out all of the navigation methods. (e.g.
TestWebContents::NavigateAndCommit())

[Xi Han, 2016-09-26 20:15:41]

lgtm

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (left):

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:117:
web_app_info.description.substr(0, chrome::kMaxMetaTagAttributeLength);
On 2016/09/26 20:13:47, pkotwicz wrote:
> |web_app_info| is a variable which is local to the
OnDidGetWebApplicationInfo()
> function. I can't find where the description is used.
> 

Acknowledged.

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
(right):

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:289:
fetcher->OnDidGetWebApplicationInfo(web_application_info);
On 2016/09/26 20:13:47, pkotwicz wrote:
> Tests with use TestWebContents do not create renderers at all.
> 
> My understanding is that renderers are only created as a result of a browser
> navigation. TestWebContents stubs out all of the navigation methods. (e.g.
> TestWebContents::NavigateAndCommit())

Acknowledged.

[pkotwicz, 2016-09-27 01:19:02]

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

[pkotwicz, 2016-09-27 01:19:03]

Dominick for OWNERS

[dominickn, 2016-09-27 01:51:28]

lgtm % nit.

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2362813003/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:215:
shortcut_info_ = ShortcutInfo(GURL());
Nit: remove braces. Also, move this check into the preceding if statement?
webapk_compatible should never be true if check_webapk_compatibility_ is false.

[pkotwicz, 2016-09-28 15:16:16]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-09-28 15:19:35]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-09-28 15:19:35]

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

[commit-bot: I haz the power, 2016-09-28 15:19:51]

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-09-28 15:57:27]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-28 16:03:09]

Description was changed from

==========
Make it clear that AddToHomescreenDataFetcher ignores WebApplicationInfo for
WebAPK compatible pages

This CL:
- Changes AddToHomescreenDataFetcher to make it clearer that WebApplicationInfo
  is ignored for WebAPK compatible pages.
- Adds tests checking that WebApplicationInfo is ignored

BUG=638468
==========

to

==========
Make it clear that AddToHomescreenDataFetcher ignores WebApplicationInfo for
WebAPK compatible pages

This CL:
- Changes AddToHomescreenDataFetcher to make it clearer that WebApplicationInfo
  is ignored for WebAPK compatible pages.
- Adds tests checking that WebApplicationInfo is ignored

BUG=638468

Committed: https://crrev.com/0749dfd34ea2aba53f29bb83c0d19d2ea825b6f9
Cr-Commit-Position: refs/heads/master@{#421531}
==========

[commit-bot: I haz the power, 2016-09-28 16:03:11]

Patchset 2 (id:??) landed as
https://crrev.com/0749dfd34ea2aba53f29bb83c0d19d2ea825b6f9
Cr-Commit-Position: refs/heads/master@{#421531}

[jaydasika, 2016-09-30 18:20:29]

A revert of this CL (patchset #2 id:40001) has been created in
https://codereview.chromium.org/2385843002/ by jaydasika@chromium.org.

The reason for reverting is:
"CheckWebApkCompatibility/AddToHomescreenDataFetcherTestCommon.ManifestShortNameClobbersWebApplicationName/0"
is flaky on android_n5x_swarming_rel
(https://bugs.chromium.org/p/chromium/issues/detail?id=651289).