Merge Android RetrieveWebappInformation and Extensions GetApplicationInfo.

Merge Android RetrieveWebappInformation and Extensions GetApplicationInfo.

This is refactoring some code and also allows add to homescreen to use
<meta name='application-name'>.

BUG=366145
Committed: https://crrev.com/89ccc63d3e5ad5893006d0e445750ecdfcb2c5ac
Cr-Commit-Position: refs/heads/master@{#295125}

[mlamouri (slow - plz ping), 2014-09-15 12:57:59]

mlamouri@chromium.org changed reviewers:
+ dcheng@chromium.org, jackhou@chromium.org, sky@chromium.org

[mlamouri (slow - plz ping), 2014-09-15 12:58:00]

jackhou@chromium.org: could you have a look at the changes wrt to web_apps/ and
extensions/. It should have no behaviour change but let me know if there is
anything that doesn't look good.

dcheng@chromium.org: could you review the IPC changes:
 chrome/common/extensions/chrome_extension_messages.h
 chrome/common/render_messages.h

sky@chromium.org: could you review everything else in:
 chrome/

[sky, 2014-09-15 16:11:02]

https://codereview.chromium.org/568823003/diff/100001/chrome/common/web_appli...
File chrome/common/web_application_info.h (right):

https://codereview.chromium.org/568823003/diff/100001/chrome/common/web_appli...
chrome/common/web_application_info.h:29: Unspecified,
See style guide for proper names. And I prefer prefixing with the name of the
enum, eg MOBILE_CAPABLE_UNSPECIFIED...

https://codereview.chromium.org/568823003/diff/100001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/568823003/diff/100001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_view_observer.cc:262: --i;
If i ==0, this overflows.

[mlamouri (slow - plz ping), 2014-09-15 16:24:17]

Comments applied. PTAL.

https://codereview.chromium.org/568823003/diff/100001/chrome/common/web_appli...
File chrome/common/web_application_info.h (right):

https://codereview.chromium.org/568823003/diff/100001/chrome/common/web_appli...
chrome/common/web_application_info.h:29: Unspecified,
On 2014/09/15 16:11:02, sky wrote:
> See style guide for proper names. And I prefer prefixing with the name of the
> enum, eg MOBILE_CAPABLE_UNSPECIFIED...

Done.

https://codereview.chromium.org/568823003/diff/100001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/568823003/diff/100001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_view_observer.cc:262: --i;
On 2014/09/15 16:11:02, sky wrote:
> If i ==0, this overflows.

Eh. I have to admit that I copy-pasted this from the previous location to the
new. It's now using iterators.

[mlamouri (slow - plz ping), 2014-09-15 16:27:23]

mlamouri@chromium.org changed reviewers:
+ dfalcantara@chromium.org, skylined@google.com
- sky@chromium.org

[mlamouri (slow - plz ping), 2014-09-15 16:27:24]

Dan, can you have a look at the changes in chrome/browser/android/

A side effect of this change is that "Add to Homescreen" on Android will now be
using <meta name="application-name"> which sounds desirable given that it is a
<meta> tag specified in the WHATWG HTML specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#me...

[mlamouri (slow - plz ping), 2014-09-15 16:28:03]

mlamouri@chromium.org changed reviewers:
+ sky@chromium.org
- skylined@google.com

[mlamouri (slow - plz ping), 2014-09-15 16:28:04]

-skylined1, +sky@

(don't know what happened...)

[gone, 2014-09-15 16:55:18]

https://codereview.chromium.org/568823003/diff/120001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java
(right):

https://codereview.chromium.org/568823003/diff/120001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java:169:
@FlakyTest
Seems weird to immediately add a FlakyTest notation.

https://codereview.chromium.org/568823003/diff/120001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/568823003/diff/120001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:61: mobile_capable_ =
web_app_info.mobile_capable;
mobile_capable_ is a really strange name for this.  should be
web_app_mode_capable_, if anything.

https://codereview.chromium.org/568823003/diff/120001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:63: title_ =
!web_app_info.title.empty() ? web_app_info.title
nit: ! seems redundant when you can just flip the condition:

web_app_info.empty() ? web_contents()->GetTitle()
                     : web_app_info.title;

https://codereview.chromium.org/568823003/diff/120001/chrome/common/chrome_co...
File chrome/common/chrome_constants.cc (left):

https://codereview.chromium.org/568823003/diff/120001/chrome/common/chrome_co...
chrome/common/chrome_constants.cc:260: const size_t kMaxMetaTagAttributeLength =
2000;
This number was decided with security, IIRC.  Can you ask palmer@ if it's
alright to change this?  Not sure why anyone needs a meta tag that's 4K long.

https://codereview.chromium.org/568823003/diff/120001/chrome/renderer/web_app...
File chrome/renderer/web_apps.cc (right):

https://codereview.chromium.org/568823003/diff/120001/chrome/renderer/web_app...
chrome/renderer/web_apps.cc:175: WebApplicationInfo::MOBILE_CAPABLE_UNSPECIFIED)
{
indentation is weird here; seems like it's indented by only 3.

[sky, 2014-09-15 17:13:19]

https://codereview.chromium.org/568823003/diff/120001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/568823003/diff/120001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_view_observer.cc:262: it =
web_app_info.icons.erase(it);
This now skips an element when you erase.

[mlamouri (slow - plz ping), 2014-09-15 20:30:52]

mlamouri@chromium.org changed reviewers:
+ palmer@chromium.org
- dcheng@chromium.org

[mlamouri (slow - plz ping), 2014-09-15 20:30:54]

PTAL, also -dcheng, +palmer

https://codereview.chromium.org/568823003/diff/120001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java
(right):

https://codereview.chromium.org/568823003/diff/120001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java:169:
@FlakyTest
On 2014/09/15 16:55:17, dfalcantara wrote:
> Seems weird to immediately add a FlakyTest notation.

I don't think there is a reason to expect that one to be less flaky than the
others. Though, when the other are fixed, that should be be fixed too.

https://codereview.chromium.org/568823003/diff/120001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/568823003/diff/120001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:61: mobile_capable_ =
web_app_info.mobile_capable;
On 2014/09/15 16:55:17, dfalcantara wrote:
> mobile_capable_ is a really strange name for this.  should be
> web_app_mode_capable_, if anything.

Renamed web_app_capable_.

https://codereview.chromium.org/568823003/diff/120001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:63: title_ =
!web_app_info.title.empty() ? web_app_info.title
On 2014/09/15 16:55:17, dfalcantara wrote:
> nit: ! seems redundant when you can just flip the condition:
> 
> web_app_info.empty() ? web_contents()->GetTitle()
>                      : web_app_info.title;
> 

Done.

https://codereview.chromium.org/568823003/diff/120001/chrome/common/chrome_co...
File chrome/common/chrome_constants.cc (left):

https://codereview.chromium.org/568823003/diff/120001/chrome/common/chrome_co...
chrome/common/chrome_constants.cc:260: const size_t kMaxMetaTagAttributeLength =
2000;
On 2014/09/15 16:55:17, dfalcantara wrote:
> This number was decided with security, IIRC.  Can you ask palmer@ if it's
> alright to change this?  Not sure why anyone needs a meta tag that's 4K long.

Reverted.

https://codereview.chromium.org/568823003/diff/120001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/568823003/diff/120001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_view_observer.cc:262: it =
web_app_info.icons.erase(it);
On 2014/09/15 17:13:18, sky wrote:
> This now skips an element when you erase.

Oups. Fixed.

https://codereview.chromium.org/568823003/diff/120001/chrome/renderer/web_app...
File chrome/renderer/web_apps.cc (right):

https://codereview.chromium.org/568823003/diff/120001/chrome/renderer/web_app...
chrome/renderer/web_apps.cc:175: WebApplicationInfo::MOBILE_CAPABLE_UNSPECIFIED)
{
On 2014/09/15 16:55:17, dfalcantara wrote:
> indentation is weird here; seems like it's indented by only 3.

Good catch.

[palmer, 2014-09-15 21:02:45]

https://codereview.chromium.org/568823003/diff/160001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/568823003/diff/160001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:61: const WebApplicationInfo&
received_web_app_info) {
You should also check the sanity of the GURL (length? valid scheme?), and the
size and number of icons.

https://codereview.chromium.org/568823003/diff/160001/chrome/common/render_me...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/568823003/diff/160001/chrome/common/render_me...
chrome/common/render_messages.h:191:
IPC_ENUM_TRAITS(WebApplicationInfo::MobileCapable)
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

"""IPC_ENUM_TRAITS() is deprecated (it generates unchecked enums).
IPC_ENUM_TRAITS_MAX_VALUE and friends in ipc/param_traits_macros.h can help you
make a constrained enum."""

[jackhou1, 2014-09-16 01:03:27]

I'm not too familiar with the renderer stuff, but LGTM for:
chrome/browser/extensions/
chrome/browser/web_applications/

[mlamouri (slow - plz ping), 2014-09-16 14:38:22]

https://codereview.chromium.org/568823003/diff/160001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/568823003/diff/160001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:61: const WebApplicationInfo&
received_web_app_info) {
On 2014/09/15 21:02:44, Chromium Palmer wrote:
> You should also check the sanity of the GURL (length? valid scheme?), and the
> size and number of icons.

Hmm, I do not use those. I'm not sure how useful it would be to sanitize things
that are not being used. Not because I do not want to add those two lines but
mostly because it would be assuming that this code will be kept up to date -
which would be a pious wish.

Wouldn't that be better to only sanitize things that are used so if someone uses
new data from |retrieve_web_app_info|, that person will be more likely to
sanitize it properly?

https://codereview.chromium.org/568823003/diff/160001/chrome/common/render_me...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/568823003/diff/160001/chrome/common/render_me...
chrome/common/render_messages.h:191:
IPC_ENUM_TRAITS(WebApplicationInfo::MobileCapable)
On 2014/09/15 21:02:44, Chromium Palmer wrote:
> http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
> 
> """IPC_ENUM_TRAITS() is deprecated (it generates unchecked enums).
> IPC_ENUM_TRAITS_MAX_VALUE and friends in ipc/param_traits_macros.h can help
you
> make a constrained enum."""

Done.

[mlamouri (slow - plz ping), 2014-09-16 14:38:35]

PTAL

[gone, 2014-09-16 17:51:02]

lgtm on my end.

[palmer, 2014-09-16 18:00:20]

> > You should also check the sanity of the GURL (length? valid scheme?), and
the
> > size and number of icons.
> 
> Hmm, I do not use those. I'm not sure how useful it would be to sanitize
things
> that are not being used. Not because I do not want to add those two lines but
> mostly because it would be assuming that this code will be kept up to date -
> which would be a pious wish.

Why send them if we don't use them?

[mlamouri (slow - plz ping), 2014-09-16 18:01:10]

On 2014/09/16 18:00:20, Chromium Palmer wrote:
> > > You should also check the sanity of the GURL (length? valid scheme?), and
> the
> > > size and number of icons.
> > 
> > Hmm, I do not use those. I'm not sure how useful it would be to sanitize
> things
> > that are not being used. Not because I do not want to add those two lines
but
> > mostly because it would be assuming that this code will be kept up to date -
> > which would be a pious wish.
> 
> Why send them if we don't use them?

It's not used by the Android code. Extension code is using that somewhere.

[palmer, 2014-09-16 18:02:00]

OK. LGTM.

[sky, 2014-09-16 18:07:30]

LGTM with the following change

https://codereview.chromium.org/568823003/diff/180001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/568823003/diff/180001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_view_observer.cc:261:
it->url.SchemeIs(url::kDataScheme) ? it = web_app_info.icons.erase(it)
Ternary operator is good for assignments and parameters, but for standalone code
an if statement is cleaner. Please convert to if.

[mlamouri (slow - plz ping), 2014-09-16 18:28:49]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-09-16 18:29:00]

https://codereview.chromium.org/568823003/diff/180001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/568823003/diff/180001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_view_observer.cc:261:
it->url.SchemeIs(url::kDataScheme) ? it = web_app_info.icons.erase(it)
On 2014/09/16 18:07:30, sky wrote:
> Ternary operator is good for assignments and parameters, but for standalone
code
> an if statement is cleaner. Please convert to if.

Done.

[commit-bot: I haz the power, 2014-09-16 18:29:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568823003/200001

[commit-bot: I haz the power, 2014-09-16 19:30:46]

Committed patchset #11 (id:200001) as 49d9e7594aa614f5012df69df0a3ca6841eb3a8b

[commit-bot: I haz the power, 2014-09-16 19:32:14]

Patchset 11 (id:??) landed as
https://crrev.com/89ccc63d3e5ad5893006d0e445750ecdfcb2c5ac
Cr-Commit-Position: refs/heads/master@{#295125}