Instant App Banner logic.

Instant App Banner logic.

- Refactors app_banner_settings_helper to reuse for instant app banners
- Adds persistence for app banner behaviour. Record when an instant app
has been opened, dismissed, or ignored to decide whether the app should
be opened by default, or the banner should be hidden.
- Adds all the needed intent extras to the instant app intent.

BUG=639497
Committed: https://crrev.com/bb6f79430c79a30d9686f26f994bc62fb612044e
Cr-Commit-Position: refs/heads/master@{#422544}

[Maria, 2016-09-28 22:41:21]

mariakhomenko@chromium.org changed reviewers:
+ dfalcantara@chromium.org, dominickn@chromium.org, tedchoc@chromium.org

[Maria, 2016-09-28 22:41:21]

Dominick -- this is the refactoring we discussed, ptal. I didn't do any constant
renaming, I figured that could be done afterwards.

Dan, infobars require your stamp of approval.

Ted, I am just trying to help you reach 500 for the next quarter ;)

[Maria, 2016-09-28 22:43:42]

On 2016/09/28 22:41:21, Maria wrote:
> Dominick -- this is the refactoring we discussed, ptal. I didn't do any
constant
> renaming, I figured that could be done afterwards.
> 
> Dan, infobars require your stamp of approval.
> 
> Ted, I am just trying to help you reach 500 for the next quarter ;)

Oh, and tests will be coming in a separate CL (after branch :S)

[dominickn, 2016-09-29 07:22:05]

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
File chrome/browser/android/instantapps/instant_apps_settings.cc (right):

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:9: #include
"base/memory/ptr_util.h"
Nit: ptr_util.h isn't used?

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:28:
GURL(url).GetOrigin(),
Nit: the GetOrigin() call isn't necessary - it will happen implicitly in the
content settings layer.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:29:
std::string(kInstantAppsKey),
Nit: the std::string shouldn't be necessary - I think the compiler will
implicitly convert the const char argument and bind it appropriately.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:39:
GURL(url).GetOrigin(),
Nit: remove GetOrigin() and std::string

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:58:
GURL(url).GetOrigin(),
Nit: remove GetOrigin() and std::string.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:79:
std::string(InstantAppsSettings::kInstantAppsKey),
Nit: remove GetOrigin() and std::string

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:97:
GURL(url).GetOrigin(),
Nit: Nit: remove GetOrigin() and std::string

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:563: if
(!value->GetDouble(event_key, &internal_time))
Change this if statement to:

if (it.key() == kInstantAppsKey || !value->GetDouble....)

For now, I think it's safest to just say that sites which have recorded an
instant app launch shouldn't be counted as a recent launch. I think that should
check the key correctly.

[Maria, 2016-09-30 17:24:41]

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
File chrome/browser/android/instantapps/instant_apps_settings.cc (right):

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:9: #include
"base/memory/ptr_util.h"
On 2016/09/29 07:22:05, dominickn wrote:
> Nit: ptr_util.h isn't used?

Done.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:28:
GURL(url).GetOrigin(),
On 2016/09/29 07:22:05, dominickn wrote:
> Nit: the GetOrigin() call isn't necessary - it will happen implicitly in the
> content settings layer.

Done.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:29:
std::string(kInstantAppsKey),
On 2016/09/29 07:22:05, dominickn wrote:
> Nit: the std::string shouldn't be necessary - I think the compiler will
> implicitly convert the const char argument and bind it appropriately.

Done.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:39:
GURL(url).GetOrigin(),
On 2016/09/29 07:22:05, dominickn wrote:
> Nit: remove GetOrigin() and std::string

Done.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:58:
GURL(url).GetOrigin(),
On 2016/09/29 07:22:05, dominickn wrote:
> Nit: remove GetOrigin() and std::string.

Done.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:79:
std::string(InstantAppsSettings::kInstantAppsKey),
On 2016/09/29 07:22:05, dominickn wrote:
> Nit: remove GetOrigin() and std::string

Done.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/android/...
chrome/browser/android/instantapps/instant_apps_settings.cc:97:
GURL(url).GetOrigin(),
On 2016/09/29 07:22:05, dominickn wrote:
> Nit: Nit: remove GetOrigin() and std::string

Done.

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/2379783002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:563: if
(!value->GetDouble(event_key, &internal_time))
On 2016/09/29 07:22:05, dominickn wrote:
> Change this if statement to:
> 
> if (it.key() == kInstantAppsKey || !value->GetDouble....)
> 
> For now, I think it's safest to just say that sites which have recorded an
> instant app launch shouldn't be counted as a recent launch. I think that
should
> check the key correctly.

Nice catch!

[gone, 2016-09-30 17:29:29]

infobars lgtm

https://chromiumcodereview.appspot.com/2379783002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/instantapps/InstantAppsHandler.java
(right):

https://chromiumcodereview.appspot.com/2379783002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/instantapps/InstantAppsHandler.java:45:
// TODO(mariakhomenko): Depend directly on the constants once we roll to v8
libraries.
Alphabetical order?

[Maria, 2016-09-30 17:39:10]

https://chromiumcodereview.appspot.com/2379783002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/instantapps/InstantAppsHandler.java
(right):

https://chromiumcodereview.appspot.com/2379783002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/instantapps/InstantAppsHandler.java:45:
// TODO(mariakhomenko): Depend directly on the constants once we roll to v8
libraries.
On 2016/09/30 17:29:29, dfalcantara (slow) wrote:
> Alphabetical order?

().//8:DDOOTaaaabcccddeeeeeeeehhiiiikkllllmmnnnnnnoooooooprrrrrssstttttvwy

:)

[Ted C, 2016-09-30 18:01:34]

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
File chrome/browser/android/instantapps/instant_apps_settings.cc (right):

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
chrome/browser/android/instantapps/instant_apps_settings.cc:59:
AppBannerSettingsHelper::APP_BANNER_EVENT_DID_ADD_TO_HOMESCREEN,
should we have a TODO to change the enum name to
APP_BANNER_EVENT_SUCCESS_ACTION_TRIGGERED or something potentially less verbose.

It would be good to make that a bit clearer (for this and native app banners)

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
chrome/browser/android/instantapps/instant_apps_settings.cc:74: // Don't show if
it has been added to the homescreen.
comment doesn't seem applicable (or needs further explanation that "added to
homescreen" is actually a proxy for X)

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
File chrome/browser/android/instantapps/instant_apps_settings.h (right):

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
chrome/browser/android/instantapps/instant_apps_settings.h:16: class
InstantAppsSettings {
some description here would be good

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/b...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/b...
chrome/browser/banners/app_banner_settings_helper.cc:18: #include
"chrome/browser/android/instantapps/instant_apps_settings.h"
This class isn't android specific as of yet, so maybe we actually want to define
the instant app key in this file to avoid that?

Or we should ifdef this to android and only handle those actions for android
(also fine)

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/b...
chrome/browser/banners/app_banner_settings_helper.cc:427:
package_name_or_start_url == InstantAppsSettings::kInstantAppsKey)
when the condition is more than one line, I like to use {}s

[Maria, 2016-09-30 20:35:18]

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
File chrome/browser/android/instantapps/instant_apps_settings.cc (right):

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
chrome/browser/android/instantapps/instant_apps_settings.cc:59:
AppBannerSettingsHelper::APP_BANNER_EVENT_DID_ADD_TO_HOMESCREEN,
On 2016/09/30 18:01:34, Ted C wrote:
> should we have a TODO to change the enum name to
> APP_BANNER_EVENT_SUCCESS_ACTION_TRIGGERED or something potentially less
verbose.
> 
> It would be good to make that a bit clearer (for this and native app banners)

Done.

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
chrome/browser/android/instantapps/instant_apps_settings.cc:74: // Don't show if
it has been added to the homescreen.
On 2016/09/30 18:01:34, Ted C wrote:
> comment doesn't seem applicable (or needs further explanation that "added to
> homescreen" is actually a proxy for X)

Yeah, I probably copied it

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
File chrome/browser/android/instantapps/instant_apps_settings.h (right):

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/a...
chrome/browser/android/instantapps/instant_apps_settings.h:16: class
InstantAppsSettings {
On 2016/09/30 18:01:34, Ted C wrote:
> some description here would be good

Done.

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/b...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/b...
chrome/browser/banners/app_banner_settings_helper.cc:18: #include
"chrome/browser/android/instantapps/instant_apps_settings.h"
On 2016/09/30 18:01:34, Ted C wrote:
> This class isn't android specific as of yet, so maybe we actually want to
define
> the instant app key in this file to avoid that?
> 
> Or we should ifdef this to android and only handle those actions for android
> (also fine)

Done.

https://chromiumcodereview.appspot.com/2379783002/diff/60001/chrome/browser/b...
chrome/browser/banners/app_banner_settings_helper.cc:427:
package_name_or_start_url == InstantAppsSettings::kInstantAppsKey)
On 2016/09/30 18:01:34, Ted C wrote:
> when the condition is more than one line, I like to use {}s

Done.

[Ted C, 2016-09-30 20:38:52]

lgtm

[Maria, 2016-09-30 20:45:44]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2016-09-30 20:45:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2379783002/#ps80001
(title: "Move instant apps key + other comments.")

[commit-bot: I haz the power, 2016-09-30 20:46:02]

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-30 21:21:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 21:22:00]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dominickn, 2016-09-30 23:31:00]

https://codereview.chromium.org/2379783002/diff/80001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/2379783002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:43: const unsigned int
kMinimumDaysBetweenBannerShows = 14;
You're going to need to update the AppBannerSettingsHelperUnitTests with this
change. Sorry I didn't pick that up earlier. :(

[Maria, 2016-09-30 23:38:59]

The CQ bit was checked by mariakhomenko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 23:39:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maria, 2016-09-30 23:39:39]

https://codereview.chromium.org/2379783002/diff/80001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/2379783002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:43: const unsigned int
kMinimumDaysBetweenBannerShows = 14;
On 2016/09/30 23:31:00, dominickn wrote:
> You're going to need to update the AppBannerSettingsHelperUnitTests with this
> change. Sorry I didn't pick that up earlier. :(

Yep, I wasn't thinking -- just updated.

[commit-bot: I haz the power, 2016-10-01 00:51:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-01 00:51:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Maria, 2016-10-03 16:53:19]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2016-10-03 16:53:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2379783002/#ps100001
(title: "Fix tests.")

[commit-bot: I haz the power, 2016-10-03 16:53:42]

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-10-03 18:57:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 18:57:34]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Maria, 2016-10-03 20:33:19]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2016-10-03 20:33:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2379783002/#ps120001
(title: "Fixing tests.")

[commit-bot: I haz the power, 2016-10-03 20:34:05]

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-10-03 21:42:54]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-03 21:44:29]

Description was changed from

==========
Instant App Banner logic.

- Refactors app_banner_settings_helper to reuse for instant app banners
- Adds persistence for app banner behaviour. Record when an instant app
has been opened, dismissed, or ignored to decide whether the app should
be opened by default, or the banner should be hidden.
- Adds all the needed intent extras to the instant app intent.

BUG=639497
==========

to

==========
Instant App Banner logic.

- Refactors app_banner_settings_helper to reuse for instant app banners
- Adds persistence for app banner behaviour. Record when an instant app
has been opened, dismissed, or ignored to decide whether the app should
be opened by default, or the banner should be hidden.
- Adds all the needed intent extras to the instant app intent.

BUG=639497

Committed: https://crrev.com/bb6f79430c79a30d9686f26f994bc62fb612044e
Cr-Commit-Position: refs/heads/master@{#422544}
==========

[commit-bot: I haz the power, 2016-10-03 21:44:31]

Patchset 7 (id:??) landed as
https://crrev.com/bb6f79430c79a30d9686f26f994bc62fb612044e
Cr-Commit-Position: refs/heads/master@{#422544}

[dominickn, 2016-10-03 22:41:06]

(In after the bell) lgtm for the record. :)