New issue
Advanced search Search tips

Issue 667073 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 671100

Blocking:
issue 616322



Sign in to add a comment

Remove app banner navigation heuristic

Project Member Reported by dominickn@chromium.org, Nov 19 2016

Issue description

App banner originally used a navigation based heuristic to determine when to trigger. This has been replaced with a more precise site engagement heuristic. Now we can remove the code supporting the navigation heuristic.
 
Blockedon: 671100
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/598df313ae463915342386163972df774453c570

commit 598df313ae463915342386163972df774453c570
Author: dominickn <dominickn@chromium.org>
Date: Wed Dec 14 07:26:00 2016

Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replaced with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG= 667073 

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

[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/browser/android/banners/app_banner_manager_android.cc
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/browser/banners/app_banner_manager.cc
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/browser/banners/app_banner_manager.h
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/browser/banners/app_banner_manager_browsertest.cc
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/browser/banners/app_banner_settings_helper.cc
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/browser/banners/app_banner_settings_helper.h
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/browser/banners/app_banner_settings_helper_unittest.cc
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/common/chrome_switches.cc
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/common/chrome_switches.h
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/TabLoadObserver.java
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/test/data/banners/prompt_in_handler_test_page.html
[modify] https://crrev.com/598df313ae463915342386163972df774453c570/chrome/test/data/banners/prompt_test_page.html

Status: Fixed (was: Assigned)
Blocking: 616322
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a8f5781bcfc7c13294895aa12205d81c3e102a82

commit a8f5781bcfc7c13294895aa12205d81c3e102a82
Author: dominickn <dominickn@chromium.org>
Date: Mon Jan 16 01:51:35 2017

Check if a PWA is installed before checking the service worker and icon.

This CL reorganises the app banner validity checking to avoid the
asynchronous service worker check and app icon download if the site
being checked should not be permitted to show a banner. This may happen
if the banner was shown and dismissed / ignored recently, or if the PWA
has already been installed.

This requires a slight reorganisation in the code as the validity check
can only be run after the manifest is downloaded, or after native app
data has been fetched if we are checking a native app banner.

BUG= 667073 

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

[modify] https://crrev.com/a8f5781bcfc7c13294895aa12205d81c3e102a82/chrome/browser/android/banners/app_banner_manager_android.cc
[modify] https://crrev.com/a8f5781bcfc7c13294895aa12205d81c3e102a82/chrome/browser/banners/app_banner_manager.cc
[modify] https://crrev.com/a8f5781bcfc7c13294895aa12205d81c3e102a82/chrome/browser/banners/app_banner_manager.h
[modify] https://crrev.com/a8f5781bcfc7c13294895aa12205d81c3e102a82/chrome/browser/banners/app_banner_settings_helper.cc

Sign in to add a comment