Split the NOT_STARTED and STOPPED_BEFORE_COMPLETION installability metrics.

Split the NOT_STARTED and STOPPED_BEFORE_COMPLETION installability metrics.

This CL ensures that the NOT_STARTED state is recorded separately from
the STOPPED_BEFORE_COMPLETION state in the installability menu-open
check metrics.

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

[dominickn, 2017-04-28 03:03:41]

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

[dominickn, 2017-04-28 03:04:03]

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

[dominickn, 2017-04-28 03:04:04]

PTAL, thanks!

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

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-04-28 04:04:38]

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

[commit-bot: I haz the power, 2017-04-28 04:04:39]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-04-28 05:08:15]

lgtm

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

After some thought, I realised that |page_status_| was never being set away from
NOT_STARTED when the first installability check was run. Fixed in this version,
PTAL!

[benwells, 2017-05-01 22:55:10]

Are there any tests for this metric?

[dominickn, 2017-05-01 23:38:11]

On 2017/05/01 22:55:10, benwells wrote:
> Are there any tests for this metric?

There are for the cases where it should be recorded as finished
(AppBannerManagerTest on Android). The not-finished cases are really tricky to
test reliably because we have to simulate the menu being triggered during the
async bits of the check, then choose to terminate / not terminate the test. It
would need a bunch of mocking / plumbing back and forth across the JNI just for
the test - enough that it possibly won't be representative. :(

WDYT?

[benwells, 2017-05-01 23:39:57]

On 2017/05/01 23:38:11, dominickn wrote:
> On 2017/05/01 22:55:10, benwells wrote:
> > Are there any tests for this metric?
> 
> There are for the cases where it should be recorded as finished
> (AppBannerManagerTest on Android). The not-finished cases are really tricky to
> test reliably because we have to simulate the menu being triggered during the
> async bits of the check, then choose to terminate / not terminate the test. It
> would need a bunch of mocking / plumbing back and forth across the JNI just
for
> the test - enough that it possibly won't be representative. :(
> 
> WDYT?

lgtm, fair enough. I was wondering if testing  might help us understand the
weird results we're seeing.

[dominickn, 2017-05-01 23:41:48]

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

[commit-bot: I haz the power, 2017-05-01 23:42:36]

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-05-02 00:31:49]

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

[commit-bot: I haz the power, 2017-05-02 00:31:49]

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_...)

[dominickn, 2017-05-02 03:35:11]

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

[commit-bot: I haz the power, 2017-05-02 03:35:26]

Dry run: CQ is trying da patch.

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

[dominickn, 2017-05-02 03:37:09]

PTAL one more time. Now we fail early in RecordQueuedMetricsOnTaskCompletion()
if the check failed and it was e.g. just for the manifest (i.e. swap out of the
in progress state and to a COMPLETE_NON_PWA state).

Browser tests are updated for this.

[commit-bot: I haz the power, 2017-05-02 04:25:02]

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

[commit-bot: I haz the power, 2017-05-02 04:25:02]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-05-04 05:05:14]

lgtm with the comment clarified

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

https://codereview.chromium.org/2844383004/diff/40001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:174: // the full params, we
don't yet know if the site is installable. However, any
This comment is confusing - it's basically saying in some cases we know that we
don't yet know. Could you rephrase it to be less confusing, e.g. "In the latter
case, (i.e. the check passed but we weren't checking everything), we don't yet
know if the site is installable. However if the check didn't pass we know for
sure the site isn't installable, regardless of how much we checked"

[dominickn, 2017-05-04 05:18:57]

Thanks!

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

https://codereview.chromium.org/2844383004/diff/40001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:174: // the full params, we
don't yet know if the site is installable. However, any
On 2017/05/04 05:05:13, benwells wrote:
> This comment is confusing - it's basically saying in some cases we know that
we
> don't yet know. Could you rephrase it to be less confusing, e.g. "In the
latter
> case, (i.e. the check passed but we weren't checking everything), we don't yet
> know if the site is installable. However if the check didn't pass we know for
> sure the site isn't installable, regardless of how much we checked"

Done.

[dominickn, 2017-05-04 05:19:08]

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

[commit-bot: I haz the power, 2017-05-04 05:19:15]

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-05-04 06:03:50]

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

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

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-05-04 06:05:09]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-05-04 06:05:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2844383004/#ps60001
(title: "Fix comment")

[commit-bot: I haz the power, 2017-05-04 06:05:18]

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-05-04 06:08:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493877909510050,
"parent_rev": "b0f43286afa116cc6ac6160cbf9cec199ba56ee3", "commit_rev":
"07f715aee7d56fd113ac152d5e0b34dd1ece3ff6"}

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

Description was changed from

==========
Split the NOT_STARTED and STOPPED_BEFORE_COMPLETION installability metrics.

This CL ensures that the NOT_STARTED state is recorded separately from
the STOPPED_BEFORE_COMPLETION state in the installability menu-open
check metrics.

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

to

==========
Split the NOT_STARTED and STOPPED_BEFORE_COMPLETION installability metrics.

This CL ensures that the NOT_STARTED state is recorded separately from
the STOPPED_BEFORE_COMPLETION state in the installability menu-open
check metrics.

BUG=704369

Review-Url: https://codereview.chromium.org/2844383004
Cr-Commit-Position: refs/heads/master@{#469283}
Committed:
https://chromium.googlesource.com/chromium/src/+/07f715aee7d56fd113ac152d5e0b...
==========

[commit-bot: I haz the power, 2017-05-04 06:09:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/07f715aee7d56fd113ac152d5e0b...