Last_n: Add instrumentation test for the tab closure case.

Last_n: Add instrumentation test for the tab closure case.

The existing unit test for making sure that tabs being closed do not
generate last_n snapshots is somewhat artificial because the test itself
guarantees proper ordering. Having this instrumentation (integration)
level test is a more complete way of testing this code is actually doing
what's supposed and that its expectations of observer method calling
order are correct.

BUG=685791
Review-Url: https://codereview.chromium.org/2721253002
Cr-Commit-Position: refs/heads/master@{#454021}
Committed: https://chromium.googlesource.com/chromium/src/+/c8f0e4eb516b29f1e871f7fb80f5ffc7a541e81c

[carlosk, 2017-03-01 02:24:07]

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

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

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

[carlosk, 2017-03-01 02:26:13]

carlosk@chromium.org changed reviewers:
+ dewittj@chromium.org

[carlosk, 2017-03-01 02:26:14]

dewittj@: PTAL.

dimich@: FYI.

[carlosk, 2017-03-01 02:28:44]

carlosk@chromium.org changed reviewers:
+ dimich@chromium.org

[carlosk, 2017-03-01 02:28:44]

Adding dimich@ as reviewer too because of comment change in
recent_tab_helper.cc.

[commit-bot: I haz the power, 2017-03-01 02:58:00]

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

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

Dry run: This issue passed the CQ dry run.

[dewittj, 2017-03-01 19:21:05]

lgtm barring comment nits

https://codereview.chromium.org/2721253002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java
(right):

https://codereview.chromium.org/2721253002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:151:
// Wait a bit and checks that no page was created
nit: trailing "."

Also, can you document how 100ms was chosen?

https://codereview.chromium.org/2721253002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:168:
// Finally switch to a new tab and check that a snapshot is not created.
don't you m ean that a snapshot /is/ created?

also, please say a "snapshot for the first tab is created" so that it's clear
that you aren't waiting for a snapshot to be created for about:blank.

[carlosk, 2017-03-01 19:39:24]

Patchset #2 (id:20001) has been deleted

[carlosk, 2017-03-01 19:47:33]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2017-03-01 19:47:34]

The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2721253002/#ps60001
(title: "Comment changes.")

[carlosk, 2017-03-01 19:47:43]

https://codereview.chromium.org/2721253002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java
(right):

https://codereview.chromium.org/2721253002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:151:
// Wait a bit and checks that no page was created
On 2017/03/01 19:21:05, dewittj wrote:
> nit: trailing "."
> 
> Also, can you document how 100ms was chosen?

Done. Will add a flakiness warning comment to the test itself.

https://codereview.chromium.org/2721253002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:168:
// Finally switch to a new tab and check that a snapshot is not created.
On 2017/03/01 19:21:05, dewittj wrote:
> don't you m ean that a snapshot /is/ created?
> 
> also, please say a "snapshot for the first tab is created" so that it's clear
> that you aren't waiting for a snapshot to be created for about:blank.

Done.

[commit-bot: I haz the power, 2017-03-01 19:48:19]

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-01 20:30:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488397653537880,
"parent_rev": "a515dc389dc5e4a6ecf5583f35692c3e3e724406", "commit_rev":
"c8f0e4eb516b29f1e871f7fb80f5ffc7a541e81c"}

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

Description was changed from

==========
Last_n: Add instrumentation test for the tab closure case.

The existing unit test for making sure that tabs being closed do not
generate last_n snapshots is somewhat artificial because the test itself
guarantees proper ordering. Having this instrumentation (integration)
level test is a more complete way of testing this code is actually doing
what's supposed and that its expectations of observer method calling
order are correct.

BUG=685791
==========

to

==========
Last_n: Add instrumentation test for the tab closure case.

The existing unit test for making sure that tabs being closed do not
generate last_n snapshots is somewhat artificial because the test itself
guarantees proper ordering. Having this instrumentation (integration)
level test is a more complete way of testing this code is actually doing
what's supposed and that its expectations of observer method calling
order are correct.

BUG=685791

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

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

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c8f0e4eb516b29f1e871f7fb80f5...