[ios] TabCollectionTabCell to use SnapshotCache.

[ios] TabCollectionTabCell to use SnapshotCache.

TabCollectionTabCell lazily retrieves snapshot images from SnapshotCache.
This CL adds this capability and unit tests. This CL does not plumb the cache
into the tab cell. This CL does not modify SnapshotCache to handle multiple
image sizes.

BUG=686770
Review-Url: https://codereview.chromium.org/2967083002
Cr-Commit-Position: refs/heads/master@{#485098}
Committed: https://chromium.googlesource.com/chromium/src/+/4f216c72a3c1ab9cb60d747eae7bdeafbdd73af1

[edchin, 2017-07-04 20:24:54]

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

[commit-bot: I haz the power, 2017-07-04 20:25:06]

Dry run: CQ is trying da patch.

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

[edchin, 2017-07-04 20:32:03]

Description was changed from

==========
[ios] TabCollectionTabCell to use SnapshotCache.

TabCollectionTabCell lazily retrieves snapshot images from SnapshotCache.
This CL adds this capability and unit tests. This CL does not plumb the cache
into the tab cell.

BUG=686770
==========

to

==========
[ios] TabCollectionTabCell to use SnapshotCache.

TabCollectionTabCell lazily retrieves snapshot images from SnapshotCache.
This CL adds this capability and unit tests. This CL does not plumb the cache
into the tab cell. This CL does not modify SnapshotCache to handle multiple
image sizes.

BUG=686770
==========

[edchin, 2017-07-04 20:32:38]

edchin@chromium.org changed reviewers:
+ jif@chromium.org, marq@chromium.org, rohitrao@chromium.org

[edchin, 2017-07-04 20:32:39]

PTAL.

[edchin, 2017-07-04 20:36:15]

Patchset #1 (id:1) has been deleted

[edchin, 2017-07-04 20:36:54]

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

[commit-bot: I haz the power, 2017-07-04 20:37:09]

Dry run: CQ is trying da patch.

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

[edchin, 2017-07-04 20:56:33]

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

[commit-bot: I haz the power, 2017-07-04 20:56:37]

Dry run: CQ is trying da patch.

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

[edchin, 2017-07-04 20:56:40]

Patchset #1 (id:20001) has been deleted

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

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

[commit-bot: I haz the power, 2017-07-04 22:09:48]

Dry run: This issue passed the CQ dry run.

[edchin, 2017-07-06 01:40:45]

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

[edchin, 2017-07-06 01:40:52]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-07-06 01:41:02]

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

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

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

Dry run: This issue passed the CQ dry run.

[edchin, 2017-07-06 04:28:32]

On 2017/07/04 20:32:39, edchin wrote:
> PTAL.

Ping.

[marq (ping after 24h), 2017-07-06 12:41:18]

I'm not wild about the UI layer becoming responsible for managing asynchronous
requests into services; that feels like the start of a slippery slope to me. 

That said, if the general consensus is that this is an approach we'll take, then
this LGTM modulo one nit.

(and thanks for keeping the CL small).

https://codereview.chromium.org/2967083002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab_collection/tab_collection_tab_cell.mm
(right):

https://codereview.chromium.org/2967083002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab_collection/tab_collection_tab_cell.mm:69:
DCHECK([NSThread isMainThread]);
This check shouldn't be needed; if the contract is that the snapshot cache calls
back on the same thread, then the cache should be doing the DCHECK.

[edchin, 2017-07-06 14:43:17]

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

[commit-bot: I haz the power, 2017-07-06 14:43:32]

Dry run: CQ is trying da patch.

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

[edchin, 2017-07-06 14:47:19]

On 2017/07/06 12:41:18, marq (ping after 24h) wrote:
> I'm not wild about the UI layer becoming responsible for managing asynchronous
> requests into services; that feels like the start of a slippery slope to me. 

This is the proposed design in the design doc. I'll ping others to chime in
here.

https://codereview.chromium.org/2967083002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab_collection/tab_collection_tab_cell.mm
(right):

https://codereview.chromium.org/2967083002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab_collection/tab_collection_tab_cell.mm:69:
DCHECK([NSThread isMainThread]);
On 2017/07/06 12:41:17, marq (ping after 24h) wrote:
> This check shouldn't be needed; if the contract is that the snapshot cache
calls
> back on the same thread, then the cache should be doing the DCHECK.

Done.

[commit-bot: I haz the power, 2017-07-06 15:54:45]

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

[commit-bot: I haz the power, 2017-07-06 15:54:46]

Dry run: This issue passed the CQ dry run.

[edchin, 2017-07-06 16:52:14]

edchin@chromium.org changed reviewers:
+ sczs@chromium.org

[jif, 2017-07-07 16:19:40]

lgtm

[sczs, 2017-07-07 19:57:42]

lgtm

As Mark said this could be a slippery slope and we should keep a close eye to
these kind of requests. 
On the other hand I think it should be fairly easy to refactor if we feel it
becomes a problem, and this would allow Ed to continue working on the rest of
the feature.

[edchin, 2017-07-07 22:07:08]

The CQ bit was checked by edchin@chromium.org

[edchin, 2017-07-07 22:07:08]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org,
jif@chromium.org, sczs@chromium.org
Link to the patchset: https://codereview.chromium.org/2967083002/#ps100001
(title: "Clean up")

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

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 23:27:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499465228036280,
"parent_rev": "56efe642b5d1fc400ac7f34451d9a540f9d30fbb", "commit_rev":
"4f216c72a3c1ab9cb60d747eae7bdeafbdd73af1"}

[commit-bot: I haz the power, 2017-07-07 23:27:26]

Description was changed from

==========
[ios] TabCollectionTabCell to use SnapshotCache.

TabCollectionTabCell lazily retrieves snapshot images from SnapshotCache.
This CL adds this capability and unit tests. This CL does not plumb the cache
into the tab cell. This CL does not modify SnapshotCache to handle multiple
image sizes.

BUG=686770
==========

to

==========
[ios] TabCollectionTabCell to use SnapshotCache.

TabCollectionTabCell lazily retrieves snapshot images from SnapshotCache.
This CL adds this capability and unit tests. This CL does not plumb the cache
into the tab cell. This CL does not modify SnapshotCache to handle multiple
image sizes.

BUG=686770

Review-Url: https://codereview.chromium.org/2967083002
Cr-Commit-Position: refs/heads/master@{#485098}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f216c72a3c1ab9cb60d747eae7b...
==========

[commit-bot: I haz the power, 2017-07-07 23:27:28]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4f216c72a3c1ab9cb60d747eae7b...

[edchin, 2017-07-07 23:53:51]

Patchset #4 (id:120001) has been deleted