Allow only responsive websites to install as a web app on mobile.

Allow only responsive websites to install as a web app on mobile.

'Responsiveness' is detected by querying the renderer for the meta
viewport tag, and verifying that its contents contain either
"width=device-width", or "initial-scale=1". Different tag cases,
orderings, and length restrictions are detected and accounted for.

If a page is not responsive on mobile, the app install banner display
pipeline is halted, and engagement checks are not run. Additional
debug logging to the javascript console is produced to inform developers
of any issues detected with the meta viewport tag.

BUG=463403

[dominickn (DO NOT USE), 2015-05-21 07:27:23]

dominickn@google.com changed reviewers:
+ benwells@chromium.org, dfalcantara@chromium.org

[dominickn (DO NOT USE), 2015-05-26 23:52:29]

This is waiting on advice regarding whether websites offering native apps should
be responsive as well. However, it would be good to get advice on the
implementation.

[benwells, 2015-05-27 05:58:55]

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

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher.cc:74: is_responsive_(true),
I think this can be made cleaner. In particular we don't need the is_responsive_
field (which is kind of falsely set to true on desktop) and to have the Start
method overridden / called later.

Given we may want to check responsiveness differently depending native /
webbiness, maybe we should do it after that has been determined. That is, there
would be a new function called something like
'CheckResponsiveness(base::Callback... then)'. In the base fetcher this would
just call then, but in the android one in would do the meta tag checks etc.

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_debug_log.cc (right):

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_debug_log.cc:46: const char
kMetaViewportTagNotResponsive[] =
Why are there two strings when they are appended together?

[gone, 2015-05-27 22:53:35]

https://codereview.chromium.org/1148163003/diff/120001/chrome/android/javates...
File
chrome/android/javatests_shell/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://codereview.chromium.org/1148163003/diff/120001/chrome/android/javates...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:446:
// Visit a site that can have a banner, then wait until the service worker is
activated.
This should just be refactored instead of copied over.  The comment about why
the AppBannerManager is disabled in the test was lost and it doesn't make much
sense if you don't realize it's copied from above.

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:18: const char
kMetaViewportTagName[] = "viewport";
Should ask Paul Kinlan about formatting.  Curious if there are variants that
aren't being considered here.

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:115: 
nit: remove newline

[dominickn (DO NOT USE), 2015-06-05 07:42:59]

Thanks for the feedback. I've tweaked the implementation, and reduced the
duplication in the tests.

I won't land this until we have a decision on whether it should apply only to
native apps or to both native and web apps.

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:18: const char
kMetaViewportTagName[] = "viewport";
On 2015/05/27 22:53:35, dfalcantara wrote:
> Should ask Paul Kinlan about formatting.  Curious if there are variants that
> aren't being considered here.

Ben found some code in the compositor which seems to do the checks we want
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...),
but it's buried pretty deeply in there, and I couldn't work out a simple way of
getting to it from here.

I agree that this implementation is not a great way of checking responsiveness.
I've tried to make it as flexible as possible, but it feels a bit like a hack.
I'd like to find something more robust - preferably a synchronous call into
blink.

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

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_data_fetcher.cc:74: is_responsive_(true),
On 2015/05/27 05:58:55, benwells wrote:
> I think this can be made cleaner. In particular we don't need the
is_responsive_
> field (which is kind of falsely set to true on desktop) and to have the Start
> method overridden / called later.
> 
> Given we may want to check responsiveness differently depending native /
> webbiness, maybe we should do it after that has been determined. That is,
there
> would be a new function called something like
> 'CheckResponsiveness(base::Callback... then)'. In the base fetcher this would
> just call then, but in the android one in would do the meta tag checks etc.

Done, but slightly differently to your suggestion.

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_debug_log.cc (right):

https://codereview.chromium.org/1148163003/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_debug_log.cc:46: const char
kMetaViewportTagNotResponsive[] =
On 2015/05/27 05:58:55, benwells wrote:
> Why are there two strings when they are appended together?

Done.

[benwells, 2015-06-11 01:54:24]

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:57: bool
handled = true;
should you call the inherited OnMessageReceived?

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:129: for (auto
property : content_tokens) {
nit: const auto&

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:140: if
(is_responsive) {
Nit: this would be cleaner as

if (!responsive) {
  // give warning
  return;
}

if (!checkfetcher) {
  // give warning
  return;
}

AppBannerDataFetcher::HandleWebApp(..)

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.h (right):

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:29: using
AppBannerDataFetcher::OnMessageReceived;
Why do you have this?

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:32: bool
OnMessageReceived(const IPC::Message& message) override;
Do these overrides need to be public? Can they be protected or private instead?

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:52: if
(!ValidateFrameHostAndOrigin(render_frame_host, validated_url))
What is this change for?

[dominickn (DO NOT USE), 2015-06-11 03:32:12]

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:57: bool
handled = true;
On 2015/06/11 01:54:23, benwells wrote:
> should you call the inherited OnMessageReceived?

It's not inherited from AppBannerDataFetcher (different signature).

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:140: if
(is_responsive) {
On 2015/06/11 01:54:24, benwells wrote:
> Nit: this would be cleaner as
> 
> if (!responsive) {
>   // give warning
>   return;
> }
> 
> if (!checkfetcher) {
>   // give warning
>   return;
> }
> 
> AppBannerDataFetcher::HandleWebApp(..)

Done.

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.h (right):

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:29: using
AppBannerDataFetcher::OnMessageReceived;
On 2015/06/11 01:54:24, benwells wrote:
> Why do you have this?

The parent class OnMessageReceived() has a different signature to the one in
this class. This using statement is needed so that the child class method
doesn't hide the parent class method (even though they take different
arguments). It crashes without this statement.

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:32: bool
OnMessageReceived(const IPC::Message& message) override;
On 2015/06/11 01:54:24, benwells wrote:
> Do these overrides need to be public? Can they be protected or private
instead?

HandleWebApp should be protected (it's protected in the parent class). Thanks!

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:52: if
(!ValidateFrameHostAndOrigin(render_frame_host, validated_url))
On 2015/06/11 01:54:24, benwells wrote:
> What is this change for?

It was due to the former implementation of this feature. I'll unwind this
change.

[gone, 2015-06-11 17:37:25]

https://codereview.chromium.org/1148163003/diff/260001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/1148163003/diff/260001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:101:
InfoBarService::FromWebContents(web_contents)
Shouldn't this be checking if the infobar is null?

[dominickn (DO NOT USE), 2015-06-12 00:33:47]

On 2015/06/11 17:37:25, dfalcantara wrote:
>
https://codereview.chromium.org/1148163003/diff/260001/chrome/browser/android...
> File chrome/browser/android/banners/app_banner_data_fetcher_android.cc
(right):
> 
>
https://codereview.chromium.org/1148163003/diff/260001/chrome/browser/android...
> chrome/browser/android/banners/app_banner_data_fetcher_android.cc:101:
> InfoBarService::FromWebContents(web_contents)
> Shouldn't this be checking if the infobar is null?

Yes, good catch. This should have been fixed up in
https://codereview.chromium.org/1161233005. Thanks!

[benwells, 2015-06-15 04:14:59]

lgtm

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_data_fetcher_android.h (right):

https://codereview.chromium.org/1148163003/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_data_fetcher_android.h:32: bool
OnMessageReceived(const IPC::Message& message) override;
On 2015/06/11 03:32:12, dominickn wrote:
> On 2015/06/11 01:54:24, benwells wrote:
> > Do these overrides need to be public? Can they be protected or private
> instead?
> 
> HandleWebApp should be protected (it's protected in the parent class). Thanks!

OnMessageReceived should also be protected or private. Nothing that knows of
this as an AppbannerDataFetcherAndroid should be calling this function, only
bits of the code that know of it as a WebContentsObserver.

[gone, 2015-06-15 22:42:19]

lgtm % ben's comments

[gone, 2016-04-25 17:29:45]

dfalcantara@chromium.org changed reviewers:
- dfalcantara@chromium.org