Allow the app banner installability check to run on page load.

Allow the app banner installability check to run on page load.

This CL implements a feature to control when the app banner
installability check is run - when the site accumulates sufficient
engagement (current default), or on every page load. Significant
refactoring of the AppBannerManager's internal state and new tests are
added to support this functionality.

New metrics are introduced in the installable manager to measure the
state of the installability check when the Android menu / add to
homescreen menu item are used. These metrics will be used to measure the
impact of running app banner checks as they are now / on page load in
determining how often we know that a site is a PWA when the user
interacts with the menu.

BUG=704369
Review-Url: https://codereview.chromium.org/2778983005
Cr-Commit-Position: refs/heads/master@{#461967}
Committed: https://chromium.googlesource.com/chromium/src/+/c2d4ff6157a4d06bd06089b872e0bc8a81068853

[dominickn, 2017-03-29 04:19:59]

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

[commit-bot: I haz the power, 2017-03-29 04:20:37]

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

[dominickn, 2017-03-29 05:41:36]

dominickn@chromium.org changed reviewers:
+ benwells@chromium.org

[dominickn, 2017-03-29 05:41:36]

WDYT? This looks big, but fully half the change is JNI plumbing and tests.

[commit-bot: I haz the power, 2017-03-29 06:54:16]

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

[commit-bot: I haz the power, 2017-03-29 06:54:16]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-03-29 19:22:25]

Description was changed from

==========
Allow the app banner installability check to run on page load.

This CL implements a feature to control when the app banner
installability check is run - when the site accumulates sufficient
engagement (current default), or on every page load. Significant
refactoring of the AppBannerManager's internal state and new tests are
added to support this functionality.

New metrics are introduced in the installable manager to measure the
state of the installability check when the Android menu / add to
homescreen menu item are used. These metrics will be used to measure the
impact of running app banner checks as they are now / on page load in
determining how often we know that a site is a PWA when the user
interacts with the menu.

BUG=704369
==========

to

==========
Allow the app banner installability check to run on page load.

This CL implements a feature to control when the app banner
installability check is run - when the site accumulates sufficient
engagement (current default), or on every page load. Significant
refactoring of the AppBannerManager's internal state and new tests are
added to support this functionality.

New metrics are introduced in the installable manager to measure the
state of the installability check when the Android menu / add to
homescreen menu item are used. These metrics will be used to measure the
impact of running app banner checks as they are now / on page load in
determining how often we know that a site is a PWA when the user
interacts with the menu.

BUG=704369
==========

[pkotwicz, 2017-03-29 19:22:25]

pkotwicz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[benwells, 2017-03-30 08:22:37]

Looks really nice, I'm feeling about this. Hopefully moving to onload checking
won't be too expensive as I think it will make everything simpler - once the
dust settles :)

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:74: if (IsActive()) {
My initial comment: Is this right? Should it be !IsInactive?

Then I realised that we rerun the banner checks sometimes to get more stuff. Is
that still right? Will it still needed now if we do all the checks on page load?

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:342: has_sufficient_engagement_ =
false;
Why do you set this false? and why COMPLETE?

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:361: state_ = state;
Is it worth setting more state here (e.g. if inactive, set
has_sufficient_engageemnt false, etc.)?

I think adding DCHECKS (e.g. if PENDING_EVENT, has_sufficient_engagement should
be true) would be worthwhile as the logic is quite subtle, and there might be
some invariants we can check here.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:517:
UpdateState(State::PENDING_EVENT);
How come you don't have to check page_requested_prompt anymore?

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:529: UpdateState(State::ACTIVE);
This is weird - why go back to ACTIVE?

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.h:217: bool IsActive() const { return
state_ == State::ACTIVE; }
nit: I think the style is unix_hacker_style for simple inline functions

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.h:30:
FORWARD_DECLARE_TEST(AppBannerManagerBrowserTest,
Is this required becaues the test is in a namespace?

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:329: // Use the
NOT_COMPLETE_PWA state to represent the general case of the check
You mean IN_PROGRESS_PWA?

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:342: // Compute what the
status would have been and record metrics.
Nit: 'would have been' -> 'would have been for any RecordMenu* calls received
while the check was running'. Or something. It took me a while to realise that.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:352: menu_opened_count_ = 0;
Do you need to reset these two variables? They should just just been reset to
zero by the while loops.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.h (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.h:232:
InstallableMetrics::InstallabilityCheckStatus status_;
This variable is kinda subtle (e.g. you need to comment). Can you rename it to
something making the subtelty obviuos? E.g. first_run_status_, page_status_, or
something like that.

[benwells, 2017-03-30 08:23:13]

lol .. missed an important word ..

I'm feeling *good* about this...

[pkotwicz, 2017-03-30 20:48:25]

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:345: :
InstallableMetrics::IN_PROGRESS_NON_PWA;
What is the value of having both IN_PROGRESS_PWA and IN_PROGRESS_NON_PWA?

If InstallableManager::RecordMenuOpenHistogram() is called while the check is in
progress, InstallableMetrics::IN_PROGRESS_PWA will always be recorded

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:356: RunCallback(task, code);
I think that it would be clearer if the metrics recording code was in GetData()
and RunCallback()

Having the logic in this function is confusing to me. I keep on wondering: "Do
we record metrics when WorkOnTask() is called but the callback is not called"

[dominickn, 2017-03-30 23:40:55]

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

[commit-bot: I haz the power, 2017-03-30 23:41:39]

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

[dominickn, 2017-03-30 23:42:53]

Thanks!

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:74: if (IsActive()) {
On 2017/03/30 08:22:37, benwells wrote:
> My initial comment: Is this right? Should it be !IsInactive?
> 
> Then I realised that we rerun the banner checks sometimes to get more stuff.
Is
> that still right? Will it still needed now if we do all the checks on page
load?

The main case this is catching is if we've triggered on page load and then
engagement ticks over whilst we're still checking (but haven't yet gotten to
waiting for engagement).

If that's the case, we'll eventually get to the pending engagement check, which
will pass. So just bailing out if we're currently active is correct I think.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:342: has_sufficient_engagement_ =
false;
On 2017/03/30 08:22:37, benwells wrote:
> Why do you set this false? and why COMPLETE?

COMPLETE means that we're not going to do anything more for this page load.
Hence, having sufficient engagement is now irrelevant.

We want to clear this out early (i.e. as soon as Stop() is called) to ensure
that we're not storing banner-triggering state for too long.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:361: state_ = state;
On 2017/03/30 08:22:36, benwells wrote:
> Is it worth setting more state here (e.g. if inactive, set
> has_sufficient_engageemnt false, etc.)?
> 
> I think adding DCHECKS (e.g. if PENDING_EVENT, has_sufficient_engagement
should
> be true) would be worthwhile as the logic is quite subtle, and there might be
> some invariants we can check here.

I added the pending event DCHECK, but I can't think of any more that are
sensible to have here (we pay the cost of evaluating more conditionals for
single cases).

The main reason I added this method was to let it be overridden in tests for run
loop catching.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:517:
UpdateState(State::PENDING_EVENT);
On 2017/03/30 08:22:36, benwells wrote:
> How come you don't have to check page_requested_prompt anymore?

We still do to return (see line 519). The key thing here is that the metric is
recorded when the reply is a CANCEL (this was a bug).

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:529: UpdateState(State::ACTIVE);
On 2017/03/30 08:22:36, benwells wrote:
> This is weird - why go back to ACTIVE?

This is analogous to the case where we didn't have the site call
preventDefault(). In that case, it stays in ACTIVE all the way till the banner
is shown. So we move back to ACTIVE when we rejoin that code path.

Does that make sense?

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.h:217: bool IsActive() const { return
state_ == State::ACTIVE; }
On 2017/03/30 08:22:37, benwells wrote:
> nit: I think the style is unix_hacker_style for simple inline functions

Done.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.h:30:
FORWARD_DECLARE_TEST(AppBannerManagerBrowserTest,
On 2017/03/30 08:22:37, benwells wrote:
> Is this required becaues the test is in a namespace?

Yes, unfortunately.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:329: // Use the
NOT_COMPLETE_PWA state to represent the general case of the check
On 2017/03/30 08:22:37, benwells wrote:
> You mean IN_PROGRESS_PWA?

Done.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:342: // Compute what the
status would have been and record metrics.
On 2017/03/30 08:22:37, benwells wrote:
> Nit: 'would have been' -> 'would have been for any RecordMenu* calls received
> while the check was running'. Or something. It took me a while to realise
that.

Done.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:345: :
InstallableMetrics::IN_PROGRESS_NON_PWA;
On 2017/03/30 20:48:25, pkotwicz wrote:
> What is the value of having both IN_PROGRESS_PWA and IN_PROGRESS_NON_PWA?
> 
> If InstallableManager::RecordMenuOpenHistogram() is called while the check is
in
> progress, InstallableMetrics::IN_PROGRESS_PWA will always be recorded

This is not correct. RecordMenuOpenHistogram() only records something if the
state is COMPLETE_PWA or COMPLETE_NON_PWA. If the check is in progress (i.e. the
state is IN_PROGRESS_PWA), then nothing is recorded until the check is done. See
the definition of that method.

We need to distinguish these cases because the case we most care about is when a
site IS a PWA and the check HAS / HASN'T completed. We don't want that to be
obscured in what is likely to be the much more frequent cases of NOT PWA / (NOT)
COMPLETED.

It will not be useful if all we get from this metric is that 90% of the time we
have completed the PWA check when the menu is opened. We'll have no idea whether
PWAs are in or out of the 90%, and that's what we care about (i.e. when the menu
is opened, is the check complete AND the site is a PWA). So we have to have the
two separate cases of COMPLETE/NOT COMPLETE for PWAs and NON PWAs.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:352: menu_opened_count_ = 0;
On 2017/03/30 08:22:37, benwells wrote:
> Do you need to reset these two variables? They should just just been reset to
> zero by the while loops.

The while loops may have set them to -1 if they started at 0. So yes, they need
to be set explicitly to 0 unless I want to add extra conditionals to not loop if
they're at 0 already. This seemed slightly clearer.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:356: RunCallback(task, code);
On 2017/03/30 20:48:25, pkotwicz wrote:
> I think that it would be clearer if the metrics recording code was in
GetData()
> and RunCallback()
> 
> Having the logic in this function is confusing to me. I keep on wondering: "Do
> we record metrics when WorkOnTask() is called but the callback is not called"

RunCallback() already has to do a bunch of fetching, and we would need to pass
the params to it. I added a new method instead. This is the appropriate time to
call it because WorkOnTask() is what computes whether or not a task is completed
since it's the main run loop in InstallableManager.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.h (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.h:232:
InstallableMetrics::InstallabilityCheckStatus status_;
On 2017/03/30 08:22:37, benwells wrote:
> This variable is kinda subtle (e.g. you need to comment). Can you rename it to
> something making the subtelty obviuos? E.g. first_run_status_, page_status_,
or
> something like that.

Done.

[commit-bot: I haz the power, 2017-03-30 23:47:01]

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

[commit-bot: I haz the power, 2017-03-30 23:47:02]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dominickn, 2017-03-31 00:11:22]

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

[commit-bot: I haz the power, 2017-03-31 00:12:25]

Dry run: 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, 2017-03-31 01:35:23]

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

[commit-bot: I haz the power, 2017-03-31 01:35:24]

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

[benwells, 2017-03-31 04:00:35]

lgtm

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:74: if (IsActive()) {
On 2017/03/30 23:42:52, dominickn wrote:
> On 2017/03/30 08:22:37, benwells wrote:
> > My initial comment: Is this right? Should it be !IsInactive?
> > 
> > Then I realised that we rerun the banner checks sometimes to get more stuff.
> Is
> > that still right? Will it still needed now if we do all the checks on page
> load?
> 
> The main case this is catching is if we've triggered on page load and then
> engagement ticks over whilst we're still checking (but haven't yet gotten to
> waiting for engagement).
> 
> If that's the case, we'll eventually get to the pending engagement check,
which
> will pass. So just bailing out if we're currently active is correct I think.

OK. This logic is really complicated, but right now I think it has to be. What I
was wondering really was, if we get to a time when we are doing a check on page
load, and getting all the things then, can we remove the need to be able to
retrigger the flow for more things and just have the one run each page load.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:529: UpdateState(State::ACTIVE);
On 2017/03/30 23:42:52, dominickn wrote:
> On 2017/03/30 08:22:36, benwells wrote:
> > This is weird - why go back to ACTIVE?
> 
> This is analogous to the case where we didn't have the site call
> preventDefault(). In that case, it stays in ACTIVE all the way till the banner
> is shown. So we move back to ACTIVE when we rejoin that code path.
> 
> Does that make sense?

Ah ... I think so.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:352: menu_opened_count_ = 0;
On 2017/03/30 23:42:52, dominickn wrote:
> On 2017/03/30 08:22:37, benwells wrote:
> > Do you need to reset these two variables? They should just just been reset
to
> > zero by the while loops.
> 
> The while loops may have set them to -1 if they started at 0. So yes, they
need
> to be set explicitly to 0 unless I want to add extra conditionals to not loop
if
> they're at 0 already. This seemed slightly clearer.

Ah ... I see. You could use a for loop instead:

for (; menu_ > 0; menu_--) {
}

Or just:
while (menu_ > 0) {
  \\blah
  menu_--;
}

Up to you...

[pkotwicz, 2017-03-31 04:12:37]

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:345: :
InstallableMetrics::IN_PROGRESS_NON_PWA;
I understand.

In this case the code on lines 327 - 331 is useless

|status_| can be replaced by:
bool pwa_check_completed_;
InstallabilityCheckStatus pwa_check_result_;

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:356: RunCallback(task, code);
I still think that RunCallback() is the correct location for this code.
WorkOnTask() also does a whole bunch of work

RunCallback() is passed a InstallableManager::Task which is:
std::pair<InstallableParams, InstallableCallback>

[dominickn, 2017-03-31 04:48:45]

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

[commit-bot: I haz the power, 2017-03-31 04:48:58]

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

[dominickn, 2017-03-31 04:58:54]

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

[commit-bot: I haz the power, 2017-03-31 04:59:10]

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

[dominickn, 2017-03-31 04:59:16]

dominickn@chromium.org changed reviewers:
+ dfalcantara@chromium.org, isherman@chromium.org

[dominickn, 2017-03-31 04:59:18]

Thanks!

benwells: can you take one more look? There were some subtle changes to metrics
logging thanks to Peter's suggestions

+dfalcantara: PTAL at Java/Android
+isherman: PTAL at histograms

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:74: if (IsActive()) {
On 2017/03/31 04:00:35, benwells wrote:
> On 2017/03/30 23:42:52, dominickn wrote:
> > On 2017/03/30 08:22:37, benwells wrote:
> > > My initial comment: Is this right? Should it be !IsInactive?
> > > 
> > > Then I realised that we rerun the banner checks sometimes to get more
stuff.
> > Is
> > > that still right? Will it still needed now if we do all the checks on page
> > load?
> > 
> > The main case this is catching is if we've triggered on page load and then
> > engagement ticks over whilst we're still checking (but haven't yet gotten to
> > waiting for engagement).
> > 
> > If that's the case, we'll eventually get to the pending engagement check,
> which
> > will pass. So just bailing out if we're currently active is correct I think.
> 
> OK. This logic is really complicated, but right now I think it has to be. What
I
> was wondering really was, if we get to a time when we are doing a check on
page
> load, and getting all the things then, can we remove the need to be able to
> retrigger the flow for more things and just have the one run each page load.

Yep, I think we can remove the retrigger stuff here if we know we're running on
page load. :)

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:345: :
InstallableMetrics::IN_PROGRESS_NON_PWA;
On 2017/03/31 04:12:37, pkotwicz wrote:
> I understand.
> 
> In this case the code on lines 327 - 331 is useless
> 
> |status_| can be replaced by:
> bool pwa_check_completed_;
> InstallabilityCheckStatus pwa_check_result_;

Done, thanks!

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:352: menu_opened_count_ = 0;
On 2017/03/31 04:00:35, benwells wrote:
> On 2017/03/30 23:42:52, dominickn wrote:
> > On 2017/03/30 08:22:37, benwells wrote:
> > > Do you need to reset these two variables? They should just just been reset
> to
> > > zero by the while loops.
> > 
> > The while loops may have set them to -1 if they started at 0. So yes, they
> need
> > to be set explicitly to 0 unless I want to add extra conditionals to not
loop
> if
> > they're at 0 already. This seemed slightly clearer.
> 
> Ah ... I see. You could use a for loop instead:
> 
> for (; menu_ > 0; menu_--) {
> }
> 
> Or just:
> while (menu_ > 0) {
>   \\blah
>   menu_--;
> }
> 
> Up to you...

Changed to the for loop, that's a bit nicer.

https://codereview.chromium.org/2778983005/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:356: RunCallback(task, code);
On 2017/03/31 04:12:37, pkotwicz wrote:
> I still think that RunCallback() is the correct location for this code.
> WorkOnTask() also does a whole bunch of work
> 
> RunCallback() is passed a InstallableManager::Task which is:
> std::pair<InstallableParams, InstallableCallback>

Acknowledged. I prefer it here as WorkOnTask is the main eval-dispatch loop.

[pkotwicz, 2017-03-31 05:07:44]

LGTM with nits

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:191: DCHECK_EQ(0,
menu_item_count_);
Personally, I would remove the DCHECKs. In my experience, DCHECKS are only
useful in catching bugs if they make tests on the trybots fail

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:286: // We may have reset
prior to completion, in which case menu_open_count_ or
Nit: |menu_open_count_| and |menu_item_count_| with '|'

[benwells, 2017-03-31 05:35:28]

still lgtm. tbh i preferred the previous, longer variable names. menu_item_count
implies something totally different.

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:191: DCHECK_EQ(0,
menu_item_count_);
On 2017/03/31 05:07:44, pkotwicz wrote:
> Personally, I would remove the DCHECKs. In my experience, DCHECKS are only
> useful in catching bugs if they make tests on the trybots fail

+1, these DCHECKS don't add much value.

[commit-bot: I haz the power, 2017-03-31 06:01:06]

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

[commit-bot: I haz the power, 2017-03-31 06:01:07]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-03-31 17:03:16]

Metrics LGTM % nits:

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_metrics.cc (right):

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_metrics.cc:13:
"Webapp.InstallabilityCheckStatus.MenuItemAddToHomescreen";
Optional nit: I would go ahead and inline these names, but that's just a
personal preference, so just putting it out there as a suggestion.

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_metrics.h (right):

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_metrics.h:11: enum
InstallabilityCheckStatus {
Optional nit: You can now use "enum class" without casts for recording UMA
histograms, so you might want to go ahead and do that here.

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_metrics.h:14: IN_PROGRESS_NON_PWA,
nit: "PWA" is not an obvious acronym, at least for me.  I'd encourage you to
spell out something more complete here.

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_metrics.h:18: MAX,
Optional nit: I would name this "COUNT" -- "MAX" is usually used to semantically
refer to a value that's equal to the last "real" value in the enum.

https://codereview.chromium.org/2778983005/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2778983005/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:98289: +  <int value="5" label="Check
complete for a PWA"/>
nit: Here too, "PWA" is not entirely obvious.  You could mention the expanded
text in the histogram descriptions, or add hover text for these labels like so:

<int value="5" label="Check complete for a PWA">Check complete for a Progressive
Web App</int>

[dominickn, 2017-04-03 00:06:12]

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

[commit-bot: I haz the power, 2017-04-03 00:06:30]

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

[dominickn, 2017-04-03 00:06:46]

Thanks!

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:191: DCHECK_EQ(0,
menu_item_count_);
On 2017/03/31 05:35:28, benwells wrote:
> On 2017/03/31 05:07:44, pkotwicz wrote:
> > Personally, I would remove the DCHECKs. In my experience, DCHECKS are only
> > useful in catching bugs if they make tests on the trybots fail
> 
> +1, these DCHECKS don't add much value.

Done.

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:286: // We may have reset
prior to completion, in which case menu_open_count_ or
On 2017/03/31 05:07:44, pkotwicz wrote:
> Nit: |menu_open_count_| and |menu_item_count_| with '|'

Done.

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_metrics.cc (right):

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_metrics.cc:13:
"Webapp.InstallabilityCheckStatus.MenuItemAddToHomescreen";
On 2017/03/31 17:03:15, Ilya Sherman wrote:
> Optional nit: I would go ahead and inline these names, but that's just a
> personal preference, so just putting it out there as a suggestion.

Done.

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_metrics.h (right):

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_metrics.h:11: enum
InstallabilityCheckStatus {
On 2017/03/31 17:03:15, Ilya Sherman wrote:
> Optional nit: You can now use "enum class" without casts for recording UMA
> histograms, so you might want to go ahead and do that here.

Done. zomg no more static casting!!!!!

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_metrics.h:14: IN_PROGRESS_NON_PWA,
On 2017/03/31 17:03:15, Ilya Sherman wrote:
> nit: "PWA" is not an obvious acronym, at least for me.  I'd encourage you to
> spell out something more complete here.

Done.

https://codereview.chromium.org/2778983005/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_metrics.h:18: MAX,
On 2017/03/31 17:03:16, Ilya Sherman wrote:
> Optional nit: I would name this "COUNT" -- "MAX" is usually used to
semantically
> refer to a value that's equal to the last "real" value in the enum.

Done.

https://codereview.chromium.org/2778983005/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2778983005/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:98289: +  <int value="5" label="Check
complete for a PWA"/>
On 2017/03/31 17:03:16, Ilya Sherman wrote:
> nit: Here too, "PWA" is not entirely obvious.  You could mention the expanded
> text in the histogram descriptions, or add hover text for these labels like
so:
> 
> <int value="5" label="Check complete for a PWA">Check complete for a
Progressive
> Web App</int>

Done, mentioned it in the histogram description.

[commit-bot: I haz the power, 2017-04-03 01:09:17]

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

[commit-bot: I haz the power, 2017-04-03 01:09:18]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-04 21:28:15]

Java stuff lgtm

[dominickn, 2017-04-04 23:55:24]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-04-04 23:55:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, benwells@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2778983005/#ps100001
(title: "Address comments")

[commit-bot: I haz the power, 2017-04-04 23:56:16]

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, 2017-04-05 03:01:11]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491350124931970,
"parent_rev": "56564d4099ebbaecf1cfb804cc09a6d198101fbb", "commit_rev":
"c743a012b21d3c99ddf903660c7c8a9c37090942"}

[commit-bot: I haz the power, 2017-04-05 03:14:09]

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

[commit-bot: I haz the power, 2017-04-05 03:14:14]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[dominickn, 2017-04-05 03:20:25]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2017-04-05 03:20:51]

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, 2017-04-05 03:27:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491362425069960,
"parent_rev": "570a5854119d55591e3a02ac8a471a07039bf467", "commit_rev":
"c2d4ff6157a4d06bd06089b872e0bc8a81068853"}

[commit-bot: I haz the power, 2017-04-05 03:29:07]

Description was changed from

==========
Allow the app banner installability check to run on page load.

This CL implements a feature to control when the app banner
installability check is run - when the site accumulates sufficient
engagement (current default), or on every page load. Significant
refactoring of the AppBannerManager's internal state and new tests are
added to support this functionality.

New metrics are introduced in the installable manager to measure the
state of the installability check when the Android menu / add to
homescreen menu item are used. These metrics will be used to measure the
impact of running app banner checks as they are now / on page load in
determining how often we know that a site is a PWA when the user
interacts with the menu.

BUG=704369
==========

to

==========
Allow the app banner installability check to run on page load.

This CL implements a feature to control when the app banner
installability check is run - when the site accumulates sufficient
engagement (current default), or on every page load. Significant
refactoring of the AppBannerManager's internal state and new tests are
added to support this functionality.

New metrics are introduced in the installable manager to measure the
state of the installability check when the Android menu / add to
homescreen menu item are used. These metrics will be used to measure the
impact of running app banner checks as they are now / on page load in
determining how often we know that a site is a PWA when the user
interacts with the menu.

BUG=704369

Review-Url: https://codereview.chromium.org/2778983005
Cr-Commit-Position: refs/heads/master@{#461967}
Committed:
https://chromium.googlesource.com/chromium/src/+/c2d4ff6157a4d06bd06089b872e0...
==========

[commit-bot: I haz the power, 2017-04-05 03:29:09]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c2d4ff6157a4d06bd06089b872e0...