[ios] Take snapshot for tab grid.

[ios] Take snapshot for tab grid.

Takes a snapshot of the visible tab when the tab grid 
is shown. This CL does not retrieve the corresponding
images in the tab grid.

BUG=686770
Review-Url: https://codereview.chromium.org/2971093002
Cr-Commit-Position: refs/heads/master@{#486021}
Committed: https://chromium.googlesource.com/chromium/src/+/69dbefe29d439e0de0f910290ad1b91adb2add08

[edchin, 2017-07-06 01:42:47]

Description was changed from

==========
[ios] Take snapshot for collection view.

BUG=
==========

to

==========
[ios] Take snapshot for collection view.

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

[edchin, 2017-07-06 02:04:27]

Patchset #1 (id:1) has been deleted

[edchin, 2017-07-06 02:08:34]

Description was changed from

==========
[ios] Take snapshot for collection view.

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

to

==========
[ios] Take snapshot for collection view.

Takes a snapshot of the visible tab when the tab grid 
is shown. This CL does not retrieve the corresponding
images in the tab grid.

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

[edchin, 2017-07-06 02:12:55]

Patchset #1 (id:20001) has been deleted

[edchin, 2017-07-06 02:13:31]

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

[commit-bot: I haz the power, 2017-07-06 02:13:47]

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 02:15:46]

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

[edchin, 2017-07-06 02:15:47]

PTAL. This CL was intentionally made small to
isolate the design choice of triggering the 
snapshot, then pushing an update to the tab grid.

[edchin, 2017-07-06 02:16:19]

Description was changed from

==========
[ios] Take snapshot for collection view.

Takes a snapshot of the visible tab when the tab grid 
is shown. This CL does not retrieve the corresponding
images in the tab grid.

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

to

==========
[ios] Take snapshot for tab grid.

Takes a snapshot of the visible tab when the tab grid 
is shown. This CL does not retrieve the corresponding
images in the tab grid.

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

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

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

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

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2971093002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right):

https://codereview.chromium.org/2971093002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:280: -
(void)takeSnapshot {
Since this talks to the view controller over the consumer protocol, and is in
general a model-ish update, maybe this belongs in the mediator? A
-takeSnapshotWithCache: method seems fine, passing in the snapshot cache
instance.

[edchin, 2017-07-07 20:25:26]

Patchset #2 (id:60001) has been deleted

[edchin, 2017-07-07 20:26:49]

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

[edchin, 2017-07-07 20:27:08]

PTAL. Comment addressed.

https://codereview.chromium.org/2971093002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right):

https://codereview.chromium.org/2971093002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:280: -
(void)takeSnapshot {
On 2017/07/06 12:48:30, marq (ping after 24h) wrote:
> Since this talks to the view controller over the consumer protocol, and is in
> general a model-ish update, maybe this belongs in the mediator? A
> -takeSnapshotWithCache: method seems fine, passing in the snapshot cache
> instance.

Done.

[commit-bot: I haz the power, 2017-07-07 20:27:21]

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-07 20:29:20]

The CQ bit was unchecked by edchin@chromium.org

[edchin, 2017-07-07 20:50:57]

Patchset #2 (id:80001) has been deleted

[edchin, 2017-07-07 20:51:14]

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

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

The CQ bit was unchecked by edchin@chromium.org

[edchin, 2017-07-07 20:52:14]

Patchset #2 (id:100001) has been deleted

[edchin, 2017-07-07 20:53:41]

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

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

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 22:21:13]

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

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

Dry run: This issue passed the CQ dry run.

[edchin, 2017-07-08 08:24:27]

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

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

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-08 08:24:42]

Patchset #2 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-07-08 09:34:39]

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

[commit-bot: I haz the power, 2017-07-08 09:34:40]

Dry run: This issue passed the CQ dry run.

[edchin, 2017-07-10 14:21:24]

Ping.

[jif, 2017-07-10 14:40:37]

lgtm

[marq (ping after 24h), 2017-07-10 17:09:33]

lgtm

[sczs, 2017-07-10 17:12:10]

lgtm
Are there any unittests that we could update?

[edchin, 2017-07-11 05:17:39]

The CQ bit was checked by edchin@chromium.org

[edchin, 2017-07-11 05:17:39]

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

[commit-bot: I haz the power, 2017-07-11 05:17:56]

CQ is trying da patch.

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

[edchin, 2017-07-11 05:19:19]

On 2017/07/10 17:12:10, sczs wrote:
> lgtm
> Are there any unittests that we could update?

Good point. I added a unit test where appropriate.

[commit-bot: I haz the power, 2017-07-11 05:29:22]

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

[commit-bot: I haz the power, 2017-07-11 05:29:23]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[edchin, 2017-07-11 18:34:46]

Patchset #3 (id:160001) has been deleted

[edchin, 2017-07-11 18:36:44]

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

[commit-bot: I haz the power, 2017-07-11 18:37:10]

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-11 18:39:46]

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

[edchin, 2017-07-11 18:40:57]

edchin@chromium.org changed required reviewers:
+ danakj@chromium.org

[edchin, 2017-07-11 18:40:58]

danakj@, this CL needs your approval because it adds a dependency onto +ui/gfx.

[danakj, 2017-07-11 19:14:15]

https://codereview.chromium.org/2971093002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm
(right):

https://codereview.chromium.org/2971093002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:19:
#include "ui/gfx/image/image.h"
I thought IOS cant depend on skia?

[edchin, 2017-07-11 21:56:59]

On 2017/07/11 19:14:15, danakj wrote:
>
https://codereview.chromium.org/2971093002/diff/180001/ios/clean/chrome/brows...
> File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm
> (right):
> 
>
https://codereview.chromium.org/2971093002/diff/180001/ios/clean/chrome/brows...
> ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:19:
> #include "ui/gfx/image/image.h"
> I thought IOS cant depend on skia?

iOS is already dependent on certain parts of gfx and skia. 
Here's an example:
https://cs.chromium.org/chromium/src/ios/chrome/DEPS?type=cs&q=ios/chrome/DEP...

[commit-bot: I haz the power, 2017-07-11 22:04:51]

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

[commit-bot: I haz the power, 2017-07-11 22:04:52]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-07-12 12:48:47]

On 2017/07/11 21:56:59, edchin wrote:
> On 2017/07/11 19:14:15, danakj wrote:
> >
>
https://codereview.chromium.org/2971093002/diff/180001/ios/clean/chrome/brows...
> > File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm
> > (right):
> > 
> >
>
https://codereview.chromium.org/2971093002/diff/180001/ios/clean/chrome/brows...
> > ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:19:
> > #include "ui/gfx/image/image.h"
> > I thought IOS cant depend on skia?
> 
> iOS is already dependent on certain parts of gfx and skia. 
> Here's an example:
>
https://cs.chromium.org/chromium/src/ios/chrome/DEPS?type=cs&q=ios/chrome/DEP...

Specifically, Chrome on iOS can use the following dependencies from skia:

  # Only parts of skia are compiled on iOS, so we explicitly list the
  # files that can be included to avoid bringing in more code.
  "+skia/ext/skia_utils_ios.h",
  "+third_party/skia/include/core/SkBitmap.h",
  "+third_party/skia/include/core/SkColor.h",
  "+third_party/skia/include/core/SkGraphics.h",

See ios/chrome/DEPS for details.

[danakj, 2017-07-12 16:22:53]

Ok thanks, LGTM

[edchin, 2017-07-12 17:55:14]

The CQ bit was checked by edchin@chromium.org

[edchin, 2017-07-12 17:55:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
sczs@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2971093002/#ps180001
(title: "Update unit test.")

[commit-bot: I haz the power, 2017-07-12 17:55:27]

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-12 18:00:17]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1499882114262620,
"parent_rev": "369cc5fa93c03c4ae78f1f87348afb4df857b4f0", "commit_rev":
"69dbefe29d439e0de0f910290ad1b91adb2add08"}

[commit-bot: I haz the power, 2017-07-12 18:00:32]

Description was changed from

==========
[ios] Take snapshot for tab grid.

Takes a snapshot of the visible tab when the tab grid 
is shown. This CL does not retrieve the corresponding
images in the tab grid.

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

to

==========
[ios] Take snapshot for tab grid.

Takes a snapshot of the visible tab when the tab grid 
is shown. This CL does not retrieve the corresponding
images in the tab grid.

BUG=686770

Review-Url: https://codereview.chromium.org/2971093002
Cr-Commit-Position: refs/heads/master@{#486021}
Committed:
https://chromium.googlesource.com/chromium/src/+/69dbefe29d439e0de0f910290ad1...
==========

[commit-bot: I haz the power, 2017-07-12 18:00:35]

Committed patchset #3 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/69dbefe29d439e0de0f910290ad1...