Update app banner state machine to use more states.

Update app banner state machine to use more states.

This replaces one instance of the ACTIVE state with a new SENDING_EVENT
state, and removes a boolean and replaces it's true state with a new
SENDING_EVENT_GOT_EARLY_PROMPT state.

BUG=729919
Review-Url: https://codereview.chromium.org/2957063002
Cr-Commit-Position: refs/heads/master@{#484862}
Committed: https://chromium.googlesource.com/chromium/src/+/18cea352aa57b1bd0e57e7e60c04cba80728542d

[benwells, 2017-06-27 09:00:22]

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

[commit-bot: I haz the power, 2017-06-27 09:00:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[benwells, 2017-06-27 09:29:04]

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

[commit-bot: I haz the power, 2017-06-27 09:29:09]

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-06-27 10:42:25]

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

[commit-bot: I haz the power, 2017-06-27 10:42:26]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-06-27 22:55:58]

(sorry comments are on an old patch set...)

https://codereview.chromium.org/2957063002/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (left):

https://codereview.chromium.org/2957063002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:383: DCHECK(!is_pending_event() ||
has_sufficient_engagement_);
We could put this back in, but it seems like there are lots of invariants we
could check here. Not sure if this one is particularly fragile.

https://codereview.chromium.org/2957063002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:460: 
I think this is unnecessary due to the logic in the rest of the function.

https://codereview.chromium.org/2957063002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:562: UpdateState(State::ACTIVE);
I think going to ACTIVE here isn't needed, as we go straight to COMPLETE in a
few lines.

https://codereview.chromium.org/2957063002/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager_browsertest.cc (left):

https://codereview.chromium.org/2957063002/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:485:
EXPECT_FALSE(manager->is_active_or_pending());
not necessary as is_complete is checked two lines later.

[benwells, 2017-06-28 07:09:08]

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

[benwells, 2017-06-28 07:09:09]

oops forgot to add the reviewer...

[benwells, 2017-06-28 07:09:09]

oops forgot to add the reviewer...

[benwells, 2017-06-29 03:52:56]

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

[commit-bot: I haz the power, 2017-06-29 03:53:09]

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-06-29 04:33:33]

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

[commit-bot: I haz the power, 2017-06-29 04:33:34]

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

[benwells, 2017-06-29 05:27:23]

Arg that test is still failing. I'll look when I get back from my holiday...

[dominickn, 2017-06-30 02:50:39]

I think you might need to look at AppBannerManagerAndroid::IsActiveForTesting()
for working out that test crash. That method is used by AppBannerManageTest to
work out if the pipeline is fully complete (it waits for it to be false), and
right now it just calls is_active().

[benwells, 2017-07-07 03:57:46]

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

[commit-bot: I haz the power, 2017-07-07 03:58:07]

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-07-07 04:00:45]

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

[commit-bot: I haz the power, 2017-07-07 04:00:47]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[benwells, 2017-07-07 05:09:59]

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

[commit-bot: I haz the power, 2017-07-07 05:10:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[benwells, 2017-07-07 06:12:38]

Android tests seem happy now!

[dominickn, 2017-07-07 06:23:19]

Mostly nits, thanks.

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

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:227: Stop();
You need to add return here I think.

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

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:545: // requested in the
beforeinstallprompt event handle), we keep going and show
Nit: "handler"

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:554: DCHECK(state_ ==
State::SENDING_EVENT_GOT_EARLY_PROMPT);
DCHECK_EQ(State::SENDING_EVENT_GOT_EARLY_PROMPT, state_);

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:233: bool IsBlockedOnNetwork()
const;
Can we call this IsWaitingForData()? We could be blocked on "network" when
waiting for a service worker to be registered.

[benwells, 2017-07-07 06:29:27]

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

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:227: Stop();
On 2017/07/07 06:23:18, dominickn wrote:
> You need to add return here I think.

Done.

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

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:545: // requested in the
beforeinstallprompt event handle), we keep going and show
On 2017/07/07 06:23:18, dominickn wrote:
> Nit: "handler"

Done.

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:554: DCHECK(state_ ==
State::SENDING_EVENT_GOT_EARLY_PROMPT);
On 2017/07/07 06:23:18, dominickn wrote:
> DCHECK_EQ(State::SENDING_EVENT_GOT_EARLY_PROMPT, state_);

Done.

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2957063002/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:233: bool IsBlockedOnNetwork()
const;
On 2017/07/07 06:23:18, dominickn wrote:
> Can we call this IsWaitingForData()? We could be blocked on "network" when
> waiting for a service worker to be registered.

Done.

[dominickn, 2017-07-07 06:32:20]

lgtm, thanks!

[benwells, 2017-07-07 07:50:26]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2017-07-07 07:50:35]

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-07-07 08:54:10]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499413826976110,
"parent_rev": "85e5d2902937b668fad6a4f27efaa267bd2fd253", "commit_rev":
"18cea352aa57b1bd0e57e7e60c04cba80728542d"}

[commit-bot: I haz the power, 2017-07-07 08:54:22]

Description was changed from

==========
Update app banner state machine to use more states.

This replaces one instance of the ACTIVE state with a new SENDING_EVENT
state, and removes a boolean and replaces it's true state with a new
SENDING_EVENT_GOT_EARLY_PROMPT state.

BUG=729919
==========

to

==========
Update app banner state machine to use more states.

This replaces one instance of the ACTIVE state with a new SENDING_EVENT
state, and removes a boolean and replaces it's true state with a new
SENDING_EVENT_GOT_EARLY_PROMPT state.

BUG=729919

Review-Url: https://codereview.chromium.org/2957063002
Cr-Commit-Position: refs/heads/master@{#484862}
Committed:
https://chromium.googlesource.com/chromium/src/+/18cea352aa57b1bd0e57e7e60c04...
==========

[commit-bot: I haz the power, 2017-07-07 08:54:24]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/18cea352aa57b1bd0e57e7e60c04...