Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 452825 Update native / web app install banner triggering
Starred by 10 users Project Member Reported by benwells@chromium.org, Jan 28 2015 Back to list
Status: Fixed
Owner:
Closed: Feb 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 459839



Sign in to add a comment
- the banner should trigger iff:
  - the site has met some criteria of visits (e.g. on a previous day in the past two weeks)
  - the banner hasn't been shown recently (or maybe ever for a first cut)
  - the app isn't installed / added to homescreen (or some approximation of this.)

Also, for the first cut we'll probably prioritize the web app install banner over the native app install banner.
 
Blocking: chromium:452566
Cc: slightlyoff@chromium.org
Also, the banner should trigger only if some quality level is met. Code for this quality level is coming from someone else (is there a bug for that?).
Comment 3 by owe...@chromium.org, Jan 28 2015
I'm hoping/anticipating that Alex will coordinate that other code, perhaps with help from Tokyo, although I've not actually heard anyone commit yet.

Also hopefully Alex R's new doc clarifies these rules a little if they were a bit vague before. He'll probably suggest we don't punt allowing the developer to indicate priority in the manifest, so its something to bear in mind as you finish up the other triggering logic.
Cc: tedc...@chromium.org
+tedchoc

For reference, here is a doc outlining what we'll do: https://docs.google.com/document/u/0/d/1SsPZcfMba7txiRAOG_4qhA0YQs6uxma2293u9S7P0X8/edit?usp=sharing_eid (Google only).

Currently there are still a lot of things to figure out but I'll make assumptions.

Thinking about this overnight I am now planning to not use history at all, but to add extra information into app banner content setting dictionary we currently have.

This currently has information per android package, I'd like to treat web app start URLs as package names. That is, we'd have information for each origin about andriod packages and web app start URLs like:
- user has blocked this package / manifest
- separate instances when the package / manifest banner could have been shown [1]
- instances when the package / manifest banner was dismissed.

Let me know if this doesn't seem reasonable.
You'll probably want to loop in the people on that email I sent you RE:the ContentSetting dictionary.  I think one of the reasons they originally objected to adding things to the dictionary in the first place was because the whole thing resided in memory.
Comment 7 by owe...@chromium.org, Jan 29 2015
It seems that turned out not to be a problem. Quoting Bernhard: "Storing complex data in content settings is absolutely fine" (ref: https://codereview.chromium.org/884373002/).
Project Member Comment 8 by bugdroid1@chromium.org, Feb 3 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1fa5a3081a04c314c2dcdf8afc6b9d951be13493

commit 1fa5a3081a04c314c2dcdf8afc6b9d951be13493
Author: benwells <benwells@chromium.org>
Date: Tue Feb 03 04:13:14 2015

Update content setting for app banners to store more information.

This changes the app banner content setting from being a dictionary of
app key to bool, to being a dictionary of app key to dictionary.

The sub dictionary contains one bool for whether the banner has been
blocked, and also a list of dates. The list of dates will contain at
most 14 dates, and will be used to record distinct dates on which the
banner could have been shown.

This is to allow more complex triggering. The goal is to actually show
banners if they could otherwise have been shown on a previous date in
the last 14 days.

Note the complex triggering is not currently used.

BUG= 452825 

Review URL: https://codereview.chromium.org/884373002

Cr-Commit-Position: refs/heads/master@{#314266}

[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/browser/android/banners/app_banner_manager.cc
[delete] http://crrev.com/7286b9d038d6d9f95578ced6de7ebaa788970d1d/chrome/browser/android/banners/app_banner_settings_helper.cc
[delete] http://crrev.com/7286b9d038d6d9f95578ced6de7ebaa788970d1d/chrome/browser/android/banners/app_banner_settings_helper.h
[add] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/browser/banners/OWNERS
[add] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/browser/banners/app_banner_settings_helper.cc
[add] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/browser/banners/app_banner_settings_helper.h
[add] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/browser/banners/app_banner_settings_helper_unittest.cc
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/browser/ui/webui/options/content_settings_handler.cc
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/chrome_browser.gypi
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/chrome/chrome_tests_unit.gypi
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/components/content_settings/core/browser/content_settings_default_provider.cc
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/components/content_settings/core/browser/content_settings_policy_provider.cc
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/components/content_settings/core/browser/content_settings_utils.cc
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/components/content_settings/core/browser/host_content_settings_map.cc
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/components/content_settings/core/common/content_settings.cc
[modify] http://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493/components/content_settings/core/common/content_settings_types.h

Project Member Comment 11 by bugdroid1@chromium.org, Feb 7 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7020ca28ac9bc1960f824beeeabff85db333131

commit e7020ca28ac9bc1960f824beeeabff85db333131
Author: dfalcantara <dfalcantara@chromium.org>
Date: Sat Feb 07 12:30:38 2015

Move banner counting logic much later in the pipeline

The banner can fail to be shown for any number of reasons,
from a user navigation to a failed icon fetch.  Record that
the banner could have been shown only right before the
infobar is created.

This fixes situations where a user already had a native app
installed and the website requested a banner for it, which
permanently fills a slot in app dictionary for the site --
even if the Play Store tells the AppBannerManager that the
app is installed and no banner should be shown.

BUG= 453170 , 452825 

Review URL: https://codereview.chromium.org/905913002

Cr-Commit-Position: refs/heads/master@{#315193}

[modify] http://crrev.com/e7020ca28ac9bc1960f824beeeabff85db333131/chrome/browser/android/banners/app_banner_manager.cc

Status: Fixed
Marking this fixed - remaining tasks are tracked in separate bugs.
Blocking: -chromium:452566
Blocking: chromium:459839
Sign in to add a comment