[blimp] Add SkPicture caching support.

[blimp] Add SkPicture caching support.

This CL adds support for de-duplication of SkPicture blobs across
consecutive frames.

Previously all DrawingDisplayItems and their respective SkPictures were
transferred from the engine to the client whenever the frames that
contained them was sent to the client. This lead to the same SkPictures
being sent over and over.

This CL changes this behavior so the engine can assume that the client
always keeps around the contents of the interest rect since last commit
and only sends down the new SkPictures that occur in the interest rect.

Both the engine and the client agrees on what is currently part of the
interest rect, and as such they can separately control their own caches
accordingly.

This has the benefit of sending less data from the engine to the client
whenever two consecutive frames have a lot of the same content.

This CL also creates //cc/blimp which is to be used for blimp-related
code living in //cc.

BUG=597811
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/fbaee11e729589960ece321f39dee33e01503368
Cr-Commit-Position: refs/heads/master@{#402006}

[nyquist, 2016-05-17 01:14:41]

Description was changed from

==========
[blimp] Add SkPicture caching support.

This CL adds support for de-duplication of SkPicture blobs across
consecutive frames.

Previously all DrawingDisplayItems and their respective SkPictures were
transferred from the engine to the client whenever the frames that
contained them was sent to the client. This lead to the same SkPictures
being sent over and over.

This CL changes this behavior so the engine can assume that the client
always keeps around the contents of the interest rect since last commit
and only sends down the new SkPictures that occur in the interest rect.

Both the engine and the client agrees on what is currently part of the
interest rect, and as such they can separately control their own caches
accordingly.

This has the benefit of sending less data from the engine to the client
whenever two consecutive frames have a lot of the same content.

BUG=597811
==========

to

==========
[blimp] Add SkPicture caching support.

This CL adds support for de-duplication of SkPicture blobs across
consecutive frames.

Previously all DrawingDisplayItems and their respective SkPictures were
transferred from the engine to the client whenever the frames that
contained them was sent to the client. This lead to the same SkPictures
being sent over and over.

This CL changes this behavior so the engine can assume that the client
always keeps around the contents of the interest rect since last commit
and only sends down the new SkPictures that occur in the interest rect.

Both the engine and the client agrees on what is currently part of the
interest rect, and as such they can separately control their own caches
accordingly.

This has the benefit of sending less data from the engine to the client
whenever two consecutive frames have a lot of the same content.

BUG=597811
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[nyquist, 2016-05-17 01:16:05]

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

[commit-bot: I haz the power, 2016-05-17 01:16:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1982893002/1

[commit-bot: I haz the power, 2016-05-17 01:22:24]

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

[commit-bot: I haz the power, 2016-05-17 01:22:25]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[nyquist, 2016-05-17 18:43:43]

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

[commit-bot: I haz the power, 2016-05-17 18:44:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1982893002/20001

[nyquist, 2016-05-17 18:45:57]

nyquist@chromium.org changed reviewers:
+ kmarshall@chromium.org, vmpstr@chromium.org

[nyquist, 2016-05-17 18:45:58]

vmpstr, kmarshall: PTAL

[commit-bot: I haz the power, 2016-05-17 18:54:47]

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

[commit-bot: I haz the power, 2016-05-17 18:54:48]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[nyquist, 2016-05-17 22:46:30]

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

[commit-bot: I haz the power, 2016-05-17 22:47:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1982893002/60001

[commit-bot: I haz the power, 2016-05-18 00:20:33]

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

[commit-bot: I haz the power, 2016-05-18 00:20:34]

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

[vmpstr, 2016-05-25 01:54:06]

Just some initial comments

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:30: NOTREACHED()
<< "Failed to find picture with id = " << engine_picture_id;
Remind me what the policy on error handling is? In normal chromium code, this
would be a DCHECK(pictures_.find(engine_picture_id) != pictures_.end()); Why
isn't it that here?

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:43:
std::make_pair(picture_data.unique_id, deserialized_picture));
How does unique_id related to SkPicture::uniqueID()?

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:75:
registry_.Unstage(engine_picture_id);
Technically this is stage for removal, not unstage... Unstage would imply to
undo a staging done by Mark function below. I'm not too picky on the names
though.

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_client_picture_cache.h:42:
sk_sp<SkPicture> DeserializePicture(const cc::PictureData& picture_data);
This should be in .cc anonymous block (and take pixel_deserializer_ as a
parameter).

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
File blimp/common/compositor/blimp_picture_cache_registry.cc (right):

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:20: auto it =
staging_.find(item);
This whole function is equivalent to:

++staging_[item];

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:30: if (it ==
staging_.end()) {
If you skip this check (and please let me know why you have it instead of a
DCHECK), then this is all --staging_[item];

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:36:
DCHECK_GE(it->second, 0U);
unsigned value is always >= 0

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:39: void
BlimpPictureCacheRegistry::Commit(std::set<uint32_t>* added_entries,
Because these are generated from an std::map, this is guaranteed to be unique
even if it's a vector. Do you need this to be a set? It has different complexity
behavior (depending on what you need it for)

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:47: if (it.second > 0) {
Maybe:

auto& which = it.second > 0 ? used : unused;
which.insert(it.first);

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:54: // Remove unused
entries from |committed_| and update |removed_entries|.
Below here, you iterate over unused (without using used) and then iterate over
used (without using unused). Why not just do this in the loop above?

something along the lines of

for (auto it = staging_.begin(); it != staging_.end();) {
  uint32_t key = it->first;
  uint32_t ref_count = it->second;
  if (ref_count == 0) {
    // This is unused.
    if (committed_.find(key) != committed_.end()) {
      committed_.erase(key);
      removed_entries->insert(key);
    }
    it = staging_.erase(it);
  } else {
    // This is used.
    if (committed_.find(key) == committed_.end()) {
      committed_.insert(key);
      added_entries->insert(key);
    }
    ++it;
  }
}

Also, if using the typical terminology after a "commit" step, you shouldn't have
any staged changes.

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:55: for (const auto it :
unused) {
don't name it "it" if it's not an iterator. Also, put the actual type instead of
auto.

[nyquist, 2016-05-26 01:08:01]

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

[nyquist, 2016-05-26 01:08:09]

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:30: NOTREACHED()
<< "Failed to find picture with id = " << engine_picture_id;
On 2016/05/25 01:54:05, vmpstr wrote:
> Remind me what the policy on error handling is? In normal chromium code, this
> would be a DCHECK(pictures_.find(engine_picture_id) != pictures_.end()); Why
> isn't it that here?

Done.

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:43:
std::make_pair(picture_data.unique_id, deserialized_picture));
On 2016/05/25 01:54:05, vmpstr wrote:
> How does unique_id related to SkPicture::uniqueID()?

It's the SkPicture::uniqueID() that the engine used, so we can't use the
deserialized_picture->uniqueID(). Do you think we should name the field
engine_unique_id or something like that instead?

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:75:
registry_.Unstage(engine_picture_id);
On 2016/05/25 01:54:05, vmpstr wrote:
> Technically this is stage for removal, not unstage... Unstage would imply to
> undo a staging done by Mark function below. I'm not too picky on the names
> though.

Done.

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_client_picture_cache.h:42:
sk_sp<SkPicture> DeserializePicture(const cc::PictureData& picture_data);
On 2016/05/25 01:54:06, vmpstr wrote:
> This should be in .cc anonymous block (and take pixel_deserializer_ as a
> parameter).

Done.

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
File blimp/common/compositor/blimp_picture_cache_registry.cc (right):

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:20: auto it =
staging_.find(item);
On 2016/05/25 01:54:06, vmpstr wrote:
> This whole function is equivalent to:
> 
> ++staging_[item];

Done.

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:30: if (it ==
staging_.end()) {
On 2016/05/25 01:54:06, vmpstr wrote:
> If you skip this check (and please let me know why you have it instead of a
> DCHECK), then this is all --staging_[item];

Done.

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:36:
DCHECK_GE(it->second, 0U);
On 2016/05/25 01:54:06, vmpstr wrote:
> unsigned value is always >= 0

... Done.

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:39: void
BlimpPictureCacheRegistry::Commit(std::set<uint32_t>* added_entries,
On 2016/05/25 01:54:06, vmpstr wrote:
> Because these are generated from an std::map, this is guaranteed to be unique
> even if it's a vector. Do you need this to be a set? It has different
complexity
> behavior (depending on what you need it for)

Done.

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:47: if (it.second > 0) {
On 2016/05/25 01:54:06, vmpstr wrote:
> Maybe:
> 
> auto& which = it.second > 0 ? used : unused;
> which.insert(it.first);

Irrelevant after the rest of the change.

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:54: // Remove unused
entries from |committed_| and update |removed_entries|.
On 2016/05/25 01:54:06, vmpstr wrote:
> Below here, you iterate over unused (without using used) and then iterate over
> used (without using unused). Why not just do this in the loop above?
> 
> something along the lines of
> 
> for (auto it = staging_.begin(); it != staging_.end();) {
>   uint32_t key = it->first;
>   uint32_t ref_count = it->second;
>   if (ref_count == 0) {
>     // This is unused.
>     if (committed_.find(key) != committed_.end()) {
>       committed_.erase(key);
>       removed_entries->insert(key);
>     }
>     it = staging_.erase(it);
>   } else {
>     // This is used.
>     if (committed_.find(key) == committed_.end()) {
>       committed_.insert(key);
>       added_entries->insert(key);
>     }
>     ++it;
>   }
> }
> 
> Also, if using the typical terminology after a "commit" step, you shouldn't
have
> any staged changes. 

Done.

https://codereview.chromium.org/1982893002/diff/60001/blimp/common/compositor...
blimp/common/compositor/blimp_picture_cache_registry.cc:55: for (const auto it :
unused) {
On 2016/05/25 01:54:06, vmpstr wrote:
> don't name it "it" if it's not an iterator. Also, put the actual type instead
of
> auto. 

Done.

[commit-bot: I haz the power, 2016-05-26 01:09:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1982893002/80001

[commit-bot: I haz the power, 2016-05-26 01:12:58]

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

[commit-bot: I haz the power, 2016-05-26 01:13:00]

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

[nyquist, 2016-05-26 21:25:47]

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

[commit-bot: I haz the power, 2016-05-26 21:26:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1982893002/100001

[commit-bot: I haz the power, 2016-05-26 22:06:18]

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

[commit-bot: I haz the power, 2016-05-26 22:06:18]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[vmpstr, 2016-06-01 00:10:57]

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:38:
BlimpClientPictureCache::~BlimpClientPictureCache() {}
nit: = default

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:43: return
pictures_.find(engine_picture_id)->second;
return pictures_[engine_picture_id];

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:49:
sk_sp<SkPicture> deserialized_picture =
I think we need to be a bit more consistent with constness here and throughout.
If GetPicture returns sk_sp<const SkPicture>, then we probably want to propagate
that in the rest of the functions/structures.

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:53:
std::make_pair(picture_data.unique_id, deserialized_picture));
I think pictures_.emplace(picture_data.unique_id,
std::move(deserialized_picture)); reads better, but it's up to you.

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:75: NOTREACHED()
<< "PictureMap does not contain entry " << it;
Can this if be a DCHECK instead? If not, can you add a comment why not.

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:47:
std::map<uint32_t, sk_sp<SkPicture>> pictures_;
Should the value be sk_sp<const SkPicture>?

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
File blimp/client/feature/compositor/client_image_serialization_processor.cc
(right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/client_image_serialization_processor.cc:29:
return base::WrapUnique(new BlimpClientPictureCache(&BlimpImageDecoder));
BlimpImageDecoder is a function? If so, it should be named DecodeBlimpImage or
something? It's a bit confusing if a function is named with a noun.

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
File blimp/common/compositor/blimp_picture_cache_registry.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.cc:25:
DCHECK_GT(used_item_ref_counts_[item], 0U);
nit: lowercase u

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.cc:35: bool is_committed =
committed_.find(key) != committed_.end();
committed_.count(key) > 0;

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.cc:36: if (ref_count > 0U)
{
nit: same here.

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
File blimp/common/compositor/blimp_picture_cache_registry.h (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:47: std::map<uint32_t,
uint32_t> used_item_ref_counts_;
std::unordered_map

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:50: std::set<uint32_t>
committed_;
std::unordered_set

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:26:
BlimpEnginePictureCache::~BlimpEnginePictureCache() {}
= default

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:39: if
(pictures_.find(picture->uniqueID()) != pictures_.end()) {
nit: braces optional

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:46: if
(stream.bytesWritten() <= 0) {
Can you add a comment on when this can fail?

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:56: picture_data.data =
data;
the last two lines could be one line?

picture_data.data = stream.copyToData()?

or .reset if sk_sp has that.

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:39: using PictureMap =
std::map<uint32_t, cc::PictureData>;
You don't need the alias, just use the type directly below.

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:74: if (old_host == host)
When does this happen? Should this check be even higher?

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:80: recording_source_->MarkForUnregistration();
This feels wrong.

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:89: recording_source_.reset(new RecordingSource);
This makes me believe that we can just do the Unmarking in the recording source
dtor.

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:93: recording_source_->set_engine_picture_cache(
likewise, we can mark things in the set function? Ie unmark the old cache (if it
was there) and mark the new cache (if it exists)

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
File cc/playback/recording_source.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
cc/playback/recording_source.cc:95: if (engine_picture_cache_)
Can you explain in a few sentences why both server and client need mark/unmark
stuff?

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
File cc/playback/recording_source.h (right):

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
cc/playback/recording_source.h:55: void MarkForRegistration();
I think these should be private

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
cc/playback/recording_source.h:72: void
set_engine_picture_cache(EnginePictureCache* engine_picture_cache) {
These setters can do more than just set a pointer (see my comments in
picture_layer). If you agree, please rename them to something like
SetEnginePictureCache

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
cc/playback/recording_source.h:100: EnginePictureCache* engine_picture_cache_;
I think this at least needs a comment explaining why there's both egine and
client picture cache and it's uses.

[nyquist, 2016-06-04 00:24:58]

PTAL

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:38:
BlimpClientPictureCache::~BlimpClientPictureCache() {}
On 2016/06/01 00:10:56, vmpstr wrote:
> nit: = default

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:43: return
pictures_.find(engine_picture_id)->second;
On 2016/06/01 00:10:56, vmpstr wrote:
> return pictures_[engine_picture_id];
> 

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:49:
sk_sp<SkPicture> deserialized_picture =
On 2016/06/01 00:10:56, vmpstr wrote:
> I think we need to be a bit more consistent with constness here and
throughout.
> If GetPicture returns sk_sp<const SkPicture>, then we probably want to
propagate
> that in the rest of the functions/structures. 

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:53:
std::make_pair(picture_data.unique_id, deserialized_picture));
On 2016/06/01 00:10:56, vmpstr wrote:
> I think pictures_.emplace(picture_data.unique_id,
> std::move(deserialized_picture)); reads better, but it's up to you. 

Does this look OK?

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:75: NOTREACHED()
<< "PictureMap does not contain entry " << it;
On 2016/06/01 00:10:56, vmpstr wrote:
> Can this if be a DCHECK instead? If not, can you add a comment why not.

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:47:
std::map<uint32_t, sk_sp<SkPicture>> pictures_;
On 2016/06/01 00:10:56, vmpstr wrote:
> Should the value be sk_sp<const SkPicture>?

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
File blimp/client/feature/compositor/client_image_serialization_processor.cc
(right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/client/feature/c...
blimp/client/feature/compositor/client_image_serialization_processor.cc:29:
return base::WrapUnique(new BlimpClientPictureCache(&BlimpImageDecoder));
On 2016/06/01 00:10:56, vmpstr wrote:
> BlimpImageDecoder is a function? If so, it should be named DecodeBlimpImage or
> something? It's a bit confusing if a function is named with a noun. 

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
File blimp/common/compositor/blimp_picture_cache_registry.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.cc:25:
DCHECK_GT(used_item_ref_counts_[item], 0U);
On 2016/06/01 00:10:56, vmpstr wrote:
> nit: lowercase u

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.cc:35: bool is_committed =
committed_.find(key) != committed_.end();
On 2016/06/01 00:10:56, vmpstr wrote:
> committed_.count(key) > 0; 

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.cc:36: if (ref_count > 0U)
{
On 2016/06/01 00:10:56, vmpstr wrote:
> nit: same here. 

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
File blimp/common/compositor/blimp_picture_cache_registry.h (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:47: std::map<uint32_t,
uint32_t> used_item_ref_counts_;
On 2016/06/01 00:10:56, vmpstr wrote:
> std::unordered_map

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:50: std::set<uint32_t>
committed_;
On 2016/06/01 00:10:56, vmpstr wrote:
> std::unordered_set

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:26:
BlimpEnginePictureCache::~BlimpEnginePictureCache() {}
On 2016/06/01 00:10:56, vmpstr wrote:
> = default

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:39: if
(pictures_.find(picture->uniqueID()) != pictures_.end()) {
On 2016/06/01 00:10:56, vmpstr wrote:
> nit: braces optional 

Yes, but we use them in //blimp, which is why my CL might seem very inconsistent
to you :-/

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:46: if
(stream.bytesWritten() <= 0) {
On 2016/06/01 00:10:56, vmpstr wrote:
> Can you add a comment on when this can fail?

From reading the serialization code, it seems that we'll always at least write a
byte, so I don't think it can. Changed to DCHECK.

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:56: picture_data.data =
data;
On 2016/06/01 00:10:56, vmpstr wrote:
> the last two lines could be one line?
> 
> picture_data.data = stream.copyToData()?
> 
> or .reset if sk_sp has that. 

Done.

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/120001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:39: using PictureMap =
std::map<uint32_t, cc::PictureData>;
On 2016/06/01 00:10:56, vmpstr wrote:
> You don't need the alias, just use the type directly below. 

Done.

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:74: if (old_host == host)
On 2016/06/01 00:10:56, vmpstr wrote:
> When does this happen? Should this check be even higher?

Removed now.

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:80: recording_source_->MarkForUnregistration();
On 2016/06/01 00:10:56, vmpstr wrote:
> This feels wrong. 
> 

Removed this block.

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:89: recording_source_.reset(new RecordingSource);
On 2016/06/01 00:10:56, vmpstr wrote:
> This makes me believe that we can just do the Unmarking in the recording
source
> dtor. 

Done.

https://codereview.chromium.org/1982893002/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:93: recording_source_->set_engine_picture_cache(
On 2016/06/01 00:10:57, vmpstr wrote:
> likewise, we can mark things in the set function? Ie unmark the old cache (if
it
> was there) and mark the new cache (if it exists)

Done.

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
File cc/playback/recording_source.cc (right):

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
cc/playback/recording_source.cc:95: if (engine_picture_cache_)
On 2016/06/01 00:10:57, vmpstr wrote:
> Can you explain in a few sentences why both server and client need mark/unmark
> stuff?

Hmm... I split these out to separate methods, so now the code looks cleaner.

However, were you suggesting that I'd explain why the
[Engine|Client]PictureCache exists and how it should be used here as well?

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
File cc/playback/recording_source.h (right):

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
cc/playback/recording_source.h:55: void MarkForRegistration();
On 2016/06/01 00:10:57, vmpstr wrote:
> I think these should be private

Done.

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
cc/playback/recording_source.h:72: void
set_engine_picture_cache(EnginePictureCache* engine_picture_cache) {
On 2016/06/01 00:10:57, vmpstr wrote:
> These setters can do more than just set a pointer (see my comments in
> picture_layer). If you agree, please rename them to something like
> SetEnginePictureCache

Done.

https://codereview.chromium.org/1982893002/diff/120001/cc/playback/recording_...
cc/playback/recording_source.h:100: EnginePictureCache* engine_picture_cache_;
On 2016/06/01 00:10:57, vmpstr wrote:
> I think this at least needs a comment explaining why there's both egine and
> client picture cache and it's uses. 

Done.

[Kevin M, 2016-06-06 23:33:59]

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:9: #include
<unordered_map>
already included in .h, ditto for stdint, memory

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:52:
DCHECK(pictures_.find(picture_data.unique_id) == pictures_.end());
DCHECK_EQ(expected, actual)

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:77: DCHECK(entry
!= pictures_.end());
Is it possible that this could happen at release time, or is this just a sanity
check?

If it's the former, then can we handle this error?

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:77: DCHECK(entry
!= pictures_.end());
DCHECK_NE

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:35: sk_sp<const
SkPicture> GetPicture(uint32_t engine_picture_id) override;
Add comments for translating Skia-ese to our use case? Or, create private
methods with more descriptive names which these impl methods can dispatch to.
Same for engine picture cache plz.

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:50: // A
ref-counted registry of SkPictures that are currently in use.
Implies that this is a refptr to a BlimpPictureCacheRegistry?

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc
(right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:31:
BlimpClientPictureCache cache_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
File blimp/common/compositor/blimp_picture_cache_registry.h (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:20: // of items with a
positive reference count since last call to Commit().
...of uncommitted items

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:24: // been committed.
Only changes to the reference count need to be informed.
It's not the ref count change that matters, it's the noncommitted <=> committed
state transition?

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:41: void
Commit(std::vector<uint32_t>* added_items,
"Commit"() seems like it's mixing in concepts from the caller - it's called by
commits, but that purpose is somewhat orthogonal to this class. Can we give this
class some nomenclature that's a bit more general-purpose?

"that have a positive reference count" - this isn't true for items whose
reference count was positive before the previous commit and then incremented
even more afterward.

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
File blimp/common/compositor/blimp_picture_cache_registry_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry_unittest.cc:17: bool
Contains(std::vector<uint32_t> haystack, uint32_t needle) {
Your tests might benefit from EXPECT_THAT(added_, UnorderedElementsAre(z, y,
x))...

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry_unittest.cc:26: };
DISALLOW yada

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:43: // Use skia's
serialize() method for now.
For now... until what? Got more detail or a bug?

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:62: for (const auto it :
added) {
"it" - good name for an iterator, not so good for a value

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:65: NOTREACHED() << "Picture
map does not contain entry " << it;
This is more like a DLOG(ERROR). NOTREACHED() means it's an impossible code
path, but this is reachable by -a bug or a data error.

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:27: //
MarkPictureForRegistration and MarkPictureForUnregistration respectively.
What's the lifetime of an engine picture cache?

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:43: // The current cache of
pictures. Cleared after every flush.
What is a "flush" here?

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:15: 
remove newline on 15, add one after 16

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:38: // Order is
unknown, but pictures should be the same.
UnorderedElementsAre FTW! Just make a MATCHER_P for SamePicture.

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:61: 
remove newline

[nyquist, 2016-06-10 01:37:21]

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

[commit-bot: I haz the power, 2016-06-10 01:38:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/160001

[commit-bot: I haz the power, 2016-06-10 01:42:46]

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

[commit-bot: I haz the power, 2016-06-10 01:42:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nyquist, 2016-06-10 22:02:25]

kmarshall: PTAL //blimp
vmpstr: PTAL

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:9: #include
<unordered_map>
On 2016/06/06 23:33:58, Kevin M wrote:
> already included in .h, ditto for stdint, memory

Done.

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:52:
DCHECK(pictures_.find(picture_data.unique_id) == pictures_.end());
On 2016/06/06 23:33:58, Kevin M wrote:
> DCHECK_EQ(expected, actual)

I'm not sure I like that for iterators. Are you certain you want this?

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:77: DCHECK(entry
!= pictures_.end());
On 2016/06/06 23:33:58, Kevin M wrote:
> Is it possible that this could happen at release time, or is this just a
sanity
> check?
> 
> If it's the former, then can we handle this error?

It's just a sanity check. If it happens it's a bug.

I'm not sure I like using the DCHECK_NE here, since the error message isn't
really helpful compared to just using the DCHECK

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:35: sk_sp<const
SkPicture> GetPicture(uint32_t engine_picture_id) override;
On 2016/06/06 23:33:58, Kevin M wrote:
> Add comments for translating Skia-ese to our use case? Or, create private
> methods with more descriptive names which these impl methods can dispatch to.
> Same for engine picture cache plz.

I tried... :-) Please see what you think.

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:50: // A
ref-counted registry of SkPictures that are currently in use.
On 2016/06/06 23:33:58, Kevin M wrote:
> Implies that this is a refptr to a BlimpPictureCacheRegistry?

Clarified comment.

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc
(right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:31:
BlimpClientPictureCache cache_;
On 2016/06/06 23:33:58, Kevin M wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
File blimp/common/compositor/blimp_picture_cache_registry.h (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:20: // of items with a
positive reference count since last call to Commit().
On 2016/06/06 23:33:58, Kevin M wrote:
> ...of uncommitted items

But it also provides the delta for items that were previously committed, but
does not have a positive reference count anymore, so I don't think it's correct
to insert 'uncommitted' here.

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:24: // been committed.
Only changes to the reference count need to be informed.
On 2016/06/06 23:33:58, Kevin M wrote:
> It's not the ref count change that matters, it's the noncommitted <=>
committed
> state transition?

Tried to clarify.

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry.h:41: void
Commit(std::vector<uint32_t>* added_items,
On 2016/06/06 23:33:58, Kevin M wrote:
> "Commit"() seems like it's mixing in concepts from the caller - it's called by
> commits, but that purpose is somewhat orthogonal to this class. Can we give
this
> class some nomenclature that's a bit more general-purpose?
> 
> "that have a positive reference count" - this isn't true for items whose
> reference count was positive before the previous commit and then incremented
> even more afterward.

You're right, it's technically calculating the delta in items that still have a
positive refcount since last call to commit. Tried to clarify the comments,
class name and this method name. Let me know what you think.

Also, what about this method name: GetPositiveRefCountDelta() or something along
those lines?

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
File blimp/common/compositor/blimp_picture_cache_registry_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry_unittest.cc:17: bool
Contains(std::vector<uint32_t> haystack, uint32_t needle) {
On 2016/06/06 23:33:58, Kevin M wrote:
> Your tests might benefit from EXPECT_THAT(added_, UnorderedElementsAre(z, y,
> x))...

Wow, that and EXPECT_TRUE(foo.empty()) was awesome! thanks!

https://codereview.chromium.org/1982893002/diff/160001/blimp/common/composito...
blimp/common/compositor/blimp_picture_cache_registry_unittest.cc:26: };
On 2016/06/06 23:33:58, Kevin M wrote:
> DISALLOW yada

Done.

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:43: // Use skia's
serialize() method for now.
On 2016/06/06 23:33:59, Kevin M wrote:
> For now... until what? Got more detail or a bug?

Well, we have in fact kind of settled for this until we change it, so this
comment seems unnecessary now. Removed.

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:62: for (const auto it :
added) {
On 2016/06/06 23:33:59, Kevin M wrote:
> "it" - good name for an iterator, not so good for a value

Done.

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:65: NOTREACHED() << "Picture
map does not contain entry " << it;
On 2016/06/06 23:33:59, Kevin M wrote:
> This is more like a DLOG(ERROR). NOTREACHED() means it's an impossible code
> path, but this is reachable by -a bug or a data error.

Done. Thanks for the clarification!

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:27: //
MarkPictureForRegistration and MarkPictureForUnregistration respectively.
On 2016/06/06 23:33:59, Kevin M wrote:
> What's the lifetime of an engine picture cache?

Done.

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:43: // The current cache of
pictures. Cleared after every flush.
On 2016/06/06 23:33:59, Kevin M wrote:
> What is a "flush" here?

Clarified comment.

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:15: 
On 2016/06/06 23:33:59, Kevin M wrote:
> remove newline on 15, add one after 16

Done.

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:38: // Order is
unknown, but pictures should be the same.
On 2016/06/06 23:33:59, Kevin M wrote:
> UnorderedElementsAre FTW! Just make a MATCHER_P for SamePicture.

Wow.

https://codereview.chromium.org/1982893002/diff/160001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:61: 
On 2016/06/06 23:33:59, Kevin M wrote:
> remove newline

Done.

[Kevin M, 2016-06-11 00:29:33]

Getting pretty close! Mostly nits but I'm very curious about the threadsafety of
the engine cache.

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:15: namespace {
newline after "namespace", but remove the one beforehand

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc
(right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:19: 
remove newline here

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:44: 
remove newline

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:66: 
remove newline

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
blimp/common/compositor/reference_tracker.cc:30: std::vector<uint32_t>*
removed_entries) {
DCHECK added_entries and removed_entries

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.h (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
blimp/common/compositor/reference_tracker.h:19: // a given item, and a way of
retrieving the delta in the number of items with a
nit: item => int32? Using a general term for "item" makes me think that it
should be templatized.

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
blimp/common/compositor/reference_tracker.h:32: // Increment the reference count
for an |item|. Must be called before
The second sentence might be better on Decrement, because it's a constraint on
how that's called.

And perhaps it could be rephrased as "negative reference counts are not
allowed"?

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
File blimp/common/compositor/reference_tracker_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
blimp/common/compositor/reference_tracker_unittest.cc:164: }  // namespace
nice tests

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:17: SkPixelSerializer*
pixel_serializer)
DCHECK(pixel_serializer)

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:23: const SkPicture*
picture) {
DCHECK(picture)

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:28: const SkPicture*
picture) {
DCHECK(picture)

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:50: DLOG(ERROR) << "Picture
map does not contain item " << item;
How messed up is it if this happens? If the answer is "very", how about
DCHECKing?

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:56: // All new items will be
sent to client, so clear everything.
the client

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:29: // The lifetime of a
cache matches the lifetime of a specific compositor.
What's the threading situation like for this class? Does this need to do
anything tricky to handle concurrent modifications by multiple raster threads?

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:45: bool
CacheContainsPicture(const SkPicture* picture);
nit: make this const?

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:23:
BlimpEnginePictureCache cache_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1982893002/diff/180001/blimp/test/support/com...
File blimp/test/support/compositor/picture_cache_test_support.h (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:9: #include
"cc/proto/picture_cache.h"
A lot of these can be turned into prototypes

https://codereview.chromium.org/1982893002/diff/180001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:24: const BlobId
GetBlobId(sk_sp<const SkPicture> picture);
Why return a const value type...?

[nyquist, 2016-06-14 01:37:38]

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

[nyquist, 2016-06-14 01:37:58]

kmarshall: PTAL

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:15: namespace {
On 2016/06/11 00:29:32, Kevin M wrote:
> newline after "namespace", but remove the one beforehand

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc
(right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:19: 
On 2016/06/11 00:29:32, Kevin M wrote:
> remove newline here

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:44: 
On 2016/06/11 00:29:32, Kevin M wrote:
> remove newline

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:66: 
On 2016/06/11 00:29:32, Kevin M wrote:
> remove newline

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
blimp/common/compositor/reference_tracker.cc:30: std::vector<uint32_t>*
removed_entries) {
On 2016/06/11 00:29:32, Kevin M wrote:
> DCHECK added_entries and removed_entries

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.h (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
blimp/common/compositor/reference_tracker.h:19: // a given item, and a way of
retrieving the delta in the number of items with a
On 2016/06/11 00:29:32, Kevin M wrote:
> nit: item => int32? Using a general term for "item" makes me think that it
> should be templatized.

I guess it technically could be templatized, but I'll change it to uint32_t for
now.

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
blimp/common/compositor/reference_tracker.h:32: // Increment the reference count
for an |item|. Must be called before
On 2016/06/11 00:29:32, Kevin M wrote:
> The second sentence might be better on Decrement, because it's a constraint on
> how that's called.
> 
> And perhaps it could be rephrased as "negative reference counts are not
> allowed"?

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
File blimp/common/compositor/reference_tracker_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/common/composito...
blimp/common/compositor/reference_tracker_unittest.cc:164: }  // namespace
On 2016/06/11 00:29:32, Kevin M wrote:
> nice tests

Well, thank you sir!

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:17: SkPixelSerializer*
pixel_serializer)
On 2016/06/11 00:29:33, Kevin M wrote:
> DCHECK(pixel_serializer)

A nullptr pixel serializer ends up using the default one in Skia, so a nullptr
is in fact valid. So keeping as is unless you feel strongly.

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:23: const SkPicture*
picture) {
On 2016/06/11 00:29:32, Kevin M wrote:
> DCHECK(picture)

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:28: const SkPicture*
picture) {
On 2016/06/11 00:29:33, Kevin M wrote:
> DCHECK(picture)

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:50: DLOG(ERROR) << "Picture
map does not contain item " << item;
On 2016/06/11 00:29:33, Kevin M wrote:
> How messed up is it if this happens? If the answer is "very", how about
> DCHECKing?

Pretty messed up. 
Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:56: // All new items will be
sent to client, so clear everything.
On 2016/06/11 00:29:32, Kevin M wrote:
> the client

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:29: // The lifetime of a
cache matches the lifetime of a specific compositor.
On 2016/06/11 00:29:33, Kevin M wrote:
> What's the threading situation like for this class? Does this need to do
> anything tricky to handle concurrent modifications by multiple raster threads?

Only ever on the main thread. Added a comment to clarify that.

Do you think I should add a DCHECK() for that in all the public methods?

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:45: bool
CacheContainsPicture(const SkPicture* picture);
On 2016/06/11 00:29:33, Kevin M wrote:
> nit: make this const?

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:23:
BlimpEnginePictureCache cache_;
On 2016/06/11 00:29:33, Kevin M wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1982893002/diff/180001/blimp/test/support/com...
File blimp/test/support/compositor/picture_cache_test_support.h (right):

https://codereview.chromium.org/1982893002/diff/180001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:9: #include
"cc/proto/picture_cache.h"
On 2016/06/11 00:29:33, Kevin M wrote:
> A lot of these can be turned into prototypes

I'm not sure I understand what you want me to do with this?

The plan for this support tool was just to aid the writing of tests as they are
today, not for "all possible things that we could want in the future".

Maybe I'm just misunderstanding something?

https://codereview.chromium.org/1982893002/diff/180001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:24: const BlobId
GetBlobId(sk_sp<const SkPicture> picture);
On 2016/06/11 00:29:33, Kevin M wrote:
> Why return a const value type...?

Done.

[commit-bot: I haz the power, 2016-06-14 01:38:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/200001

[commit-bot: I haz the power, 2016-06-14 01:40:59]

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

[commit-bot: I haz the power, 2016-06-14 01:41:00]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nyquist, 2016-06-16 18:36:47]

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

[commit-bot: I haz the power, 2016-06-16 18:37:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/240001

[commit-bot: I haz the power, 2016-06-16 20:13:08]

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

[commit-bot: I haz the power, 2016-06-16 20:13:09]

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

[vmpstr, 2016-06-16 22:09:59]

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:40:
AddNewPicturesToCache(cache_update);
That's a silly function. Why not sure put the code from AddNewPicturesToCache to
here?

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:71: sk_sp<const
SkPicture> deserialized_picture =
I'm kind of nit picking here, but maybe

DCHECK(pictures_.find(picture_data.unique_id) == pictures_.end());
pictures_[picture_data.unique_id] = DeserializePicture(pixel_deserializer_,
picture_data);

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:85: for (const
auto& it : removed) {
Don't call it |it| if it's not an iterator

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:87: DCHECK(entry
!= pictures_.end());
likewise here,

DCHECK(pictures_.find(it) != pictures_.end());
pictures_.erase(it);

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:98: #if
DCHECK_IS_ON()
Just to save on #if everywhere you call this, you can have it defined always,
and put this check inside the function

void Verify() {
#if DCHECK_IS_ON()
...
#endif  // DCHECK_IS_ON()
}

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:28: class
BlimpClientPictureCache : public cc::ClientPictureCache {
might need an export

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:46: void
RemoveUnusedPicturesFromCache(const std::vector<uint32_t>& picture_ids);
comment on what the parameter is

https://codereview.chromium.org/1982893002/diff/240001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.h (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/common/composito...
blimp/common/compositor/reference_tracker.h:48: std::unordered_map<uint32_t,
uint32_t> active_ref_counts_;
i think the value can just be "int"

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:63: return
pictures_.find(picture->uniqueID()) != pictures_.end();
inline this wherever it's used

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:73: cc::PictureData
picture_data;
Can this have a constructor that takes two parameters, and then

pictures_[picture_data.unique_id] = cc::PictureData(picture->uniqueID(),
stream.copyToData());?

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:43:
EXPECT_THAT(update, testing::UnorderedElementsAre(PictureMatches(picture1),
O_O TIL

https://codereview.chromium.org/1982893002/diff/240001/cc/layers/picture_laye...
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:70: if (!host)
Should this if also reset the cache on a recording_source_ if it exists?

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
File cc/playback/display_item.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item.cc:14: EnginePictureCache* engine_picture_cache) const
{}
empty virtuals can be inlined iirc

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.cc:53: uint32_t
GetUniqueSkPictureIDOrNothing(const proto::DisplayItem& item_proto) {
OrNothing is wrong here. It doesn't return nothing.

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.cc:88:
engine_picture_ids.push_back(GetUniqueSkPictureIDOrNothing(item_proto));
So this is meant to have a bunch of 0s in there?

Would this be better written as 

if (item_proto.type() == proto::DisplayItem::Type_Drawing) {
  DCHEKC(item_proto.has_drawing_item());
 
engine_picture_ids.push_back(GetDrawingDisplayItemId(item_proto.drawing_item()));
} else {
  // maybe just skip this case?
  engine_picture_ids.push_back(0);
}

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.cc:95:
list->engine_picture_ids_.swap(engine_picture_ids);
Sorry if I missed this, but why are storing engine_picture_ids_ on display item
list?

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.cc:141: for (const auto& item : items_)
What I proposed earlier is to maybe provide this type of iterator publicly

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.h:63: void
MarkForRegistrationEngine(EnginePictureCache* engine_picture_cache);
I'm really not a fan of having duplicate set of functions because this display
item list can be on both the server and client... 

What I mean to say is that from the design perspective, these functions don't
belong on a display item list.

Is it possible to instead add an iterator to the display item list that iterates
and returns the display items and then use that iterator to mark things in the
engine/client directly?

for (auto& item : display_item_list)
 engine_picture_cache_->MarkForRegistration(item.GetPicture());

with maybe display item having a virtual GetPicture that returns either nullptr
or a picture as appropriate..

WDYt? I'm not suggesting that you do this, but I think we need a better design
instead of this double dispatch-esque approach. This is especially true since
you have to have these functions in pairs depending on what cache you're
registering this in. So if there's another alternative, that's fine as well.

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/drawing_di...
File cc/playback/drawing_display_item.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/drawing_di...
cc/playback/drawing_display_item.cc:77: if (picture_ && engine_picture_cache)
Maybe just expose picture_?

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/recording_...
File cc/playback/recording_source.h (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/recording_...
cc/playback/recording_source.h:105: void MarkForRegistrationEngine();
Similarly here... is it possible to expose the iteration of the display items
instead?

I know I said that I'd prefer you move these onto the raster source, but
again... I'm really not a fan of having engine/client pointers on an object that
is otherwise independent of blimp and just represents a way to draw something
into a canvas

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache.cc
File cc/proto/picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.cc:9: PictureData::PictureData() {}
= default

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.cc:13: PictureData::~PictureData() {}
= default

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache.h
File cc/proto/picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
Why is this defined in cc/proto? This doesn't look like a
serialization/conversion helper

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.h:21: struct CC_EXPORT PictureData {
Each individual struct/class should have its own file.

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.h:22: PictureData();
remove this have unique_id = 0 below

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.h:30: using PictureCacheUpdate =
std::vector<PictureData>;
Maybe just use std::vector<PictureData> wherever you have a PictureCacheUpdate?
It wasn't clear to me earlier what this was except that you could get a
PictureData out of it.

https://codereview.chromium.org/1982893002/diff/240001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:336: InitializePictureCacheForTesting();
I'm not sure the compiler will let you have this call in a non test code, even
if it's only called from a ForTesting function... if it works it works

[nyquist, 2016-06-16 22:34:09]

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

[commit-bot: I haz the power, 2016-06-16 22:35:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/260001

[commit-bot: I haz the power, 2016-06-17 00:26:02]

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

[commit-bot: I haz the power, 2016-06-17 00:26:03]

Dry run: This issue passed the CQ dry run.

[Kevin M, 2016-06-17 17:53:38]

Looking pretty good... just a few minor commentz

https://codereview.chromium.org/1982893002/diff/260001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc
(right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:21: bool
FakeImageDecoder(const void* input, size_t input_size, SkBitmap* bitmap) {
Turn this into a MOCK_METHOD of BlimpClientPictureCacheTest so that you can
verify that it was called?

https://codereview.chromium.org/1982893002/diff/260001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.h (right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/common/composito...
blimp/common/compositor/reference_tracker.h:39: // Calculates the delta of items
that still have a positive reference count
Also be clear about the side effects of this method - it comes across as a
read-only operation.

https://codereview.chromium.org/1982893002/diff/260001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:7: #include <sstream>
O_o 

Is this needed?

https://codereview.chromium.org/1982893002/diff/260001/blimp/engine/renderer/...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/engine/renderer/...
blimp/engine/renderer/engine_image_serialization_processor.cc:108:
EngineImageSerializationProcessor::CreateClientPictureCache() {
Add NOTREACHED() here?

https://codereview.chromium.org/1982893002/diff/260001/blimp/test/support/com...
File blimp/test/support/compositor/picture_cache_test_support.h (right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:31: bool
SamePicture(sk_sp<const SkPicture> picture,
Is this used by anything other than the matcher?

If not, can we put the matcher here, too?

[nyquist, 2016-06-17 21:31:43]

kmarshall: PTAL

https://codereview.chromium.org/1982893002/diff/260001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc
(right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:21: bool
FakeImageDecoder(const void* input, size_t input_size, SkBitmap* bitmap) {
On 2016/06/17 17:53:37, Kevin M wrote:
> Turn this into a MOCK_METHOD of BlimpClientPictureCacheTest so that you can
> verify that it was called?

I could not find a good way of doing this. Mocking static method? Using
base::Bind()? But in the end, skia requires a raw function pointer, which means
I can't use base::Callback :-/

https://codereview.chromium.org/1982893002/diff/260001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.h (right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/common/composito...
blimp/common/compositor/reference_tracker.h:39: // Calculates the delta of items
that still have a positive reference count
On 2016/06/17 17:53:37, Kevin M wrote:
> Also be clear about the side effects of this method - it comes across as a
> read-only operation.

Done.

https://codereview.chromium.org/1982893002/diff/260001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:7: #include <sstream>
On 2016/06/17 17:53:37, Kevin M wrote:
> O_o 
> 
> Is this needed?

Done.

https://codereview.chromium.org/1982893002/diff/260001/blimp/engine/renderer/...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/engine/renderer/...
blimp/engine/renderer/engine_image_serialization_processor.cc:108:
EngineImageSerializationProcessor::CreateClientPictureCache() {
On 2016/06/17 17:53:38, Kevin M wrote:
> Add NOTREACHED() here?

Done.

https://codereview.chromium.org/1982893002/diff/260001/blimp/test/support/com...
File blimp/test/support/compositor/picture_cache_test_support.h (right):

https://codereview.chromium.org/1982893002/diff/260001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:31: bool
SamePicture(sk_sp<const SkPicture> picture,
On 2016/06/17 17:53:38, Kevin M wrote:
> Is this used by anything other than the matcher?
> 
> If not, can we put the matcher here, too?

Done.

[nyquist, 2016-06-17 21:31:55]

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

[commit-bot: I haz the power, 2016-06-17 21:32:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/280001

[commit-bot: I haz the power, 2016-06-17 23:08:12]

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

[commit-bot: I haz the power, 2016-06-17 23:08:15]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Kevin M, 2016-06-21 21:35:05]

blimp lgtm

https://codereview.chromium.org/1982893002/diff/280001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc
(right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:27:
BlimpClientPictureCacheTest() : cache_(&FakeImageDecoder) {}
Needs an override dtor, otherwise members of BlimpClientPictureCacheTest won't
be deleted.

https://codereview.chromium.org/1982893002/diff/280001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.cc (right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/common/composito...
blimp/common/compositor/reference_tracker.cc:39: committed_.insert(key);
Add a one-line comment, like on 48?

https://codereview.chromium.org/1982893002/diff/280001/blimp/common/composito...
File blimp/common/compositor/reference_tracker_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/common/composito...
blimp/common/compositor/reference_tracker_unittest.cc:20: ReferenceTrackerTest()
{}
Add override dtor

https://codereview.chromium.org/1982893002/diff/280001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:43: void
SerializePictureAndAddToCache(const SkPicture* picture);
You used "Put()" in BlobCache. How about "Put()" here too? IMO the cache part is
implied and the serialize aspect isn't really necessary to expose in the
interface.

https://codereview.chromium.org/1982893002/diff/280001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:46: bool
CacheContainsPicture(const SkPicture* picture) const;
"Cache" and "Picture" are both implied. How about just "Contains()" ?

https://codereview.chromium.org/1982893002/diff/280001/blimp/test/support/com...
File blimp/test/support/compositor/picture_cache_test_support.h (right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:11: #include
"third_party/skia/include/core/SkColor.h"
switch these to forward declares where possible?

https://codereview.chromium.org/1982893002/diff/280001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:32: bool
SamePicture(sk_sp<const SkPicture> picture,
nittiest of nits: PicturesEqual() ?

[nyquist, 2016-06-24 10:33:43]

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

[commit-bot: I haz the power, 2016-06-24 10:34:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982893002/320001

[nyquist, 2016-06-24 11:11:15]

vmpstr: PTAL
kmarshall: I've addressed the comments you had, but there are a few new changes
based on comments from vmpstr, so feel free to take a look at those as well.

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:40:
AddNewPicturesToCache(cache_update);
On 2016/06/16 22:09:58, vmpstr wrote:
> That's a silly function. Why not sure put the code from AddNewPicturesToCache
to
> here?

Done.

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:71: sk_sp<const
SkPicture> deserialized_picture =
On 2016/06/16 22:09:58, vmpstr wrote:
> I'm kind of nit picking here, but maybe
> 
> DCHECK(pictures_.find(picture_data.unique_id) == pictures_.end());
> pictures_[picture_data.unique_id] = DeserializePicture(pixel_deserializer_,
> picture_data);
> 

Done.

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:85: for (const
auto& it : removed) {
On 2016/06/16 22:09:58, vmpstr wrote:
> Don't call it |it| if it's not an iterator

Done.

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:87: DCHECK(entry
!= pictures_.end());
On 2016/06/16 22:09:58, vmpstr wrote:
> likewise here,
> 
> DCHECK(pictures_.find(it) != pictures_.end());
> pictures_.erase(it);

Done.

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.cc:98: #if
DCHECK_IS_ON()
On 2016/06/16 22:09:58, vmpstr wrote:
> Just to save on #if everywhere you call this, you can have it defined always,
> and put this check inside the function
> 
> void Verify() {
> #if DCHECK_IS_ON()
> ...
> #endif  // DCHECK_IS_ON()
> }

Done.

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:28: class
BlimpClientPictureCache : public cc::ClientPictureCache {
On 2016/06/16 22:09:58, vmpstr wrote:
> might need an export

Wouldn't that lead to some kind of compile error or something along those lines
if I needed it?

https://codereview.chromium.org/1982893002/diff/240001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache.h:46: void
RemoveUnusedPicturesFromCache(const std::vector<uint32_t>& picture_ids);
On 2016/06/16 22:09:58, vmpstr wrote:
> comment on what the parameter is

Done.

https://codereview.chromium.org/1982893002/diff/240001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.h (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/common/composito...
blimp/common/compositor/reference_tracker.h:48: std::unordered_map<uint32_t,
uint32_t> active_ref_counts_;
On 2016/06/16 22:09:58, vmpstr wrote:
> i think the value can just be "int"

Done.

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:63: return
pictures_.find(picture->uniqueID()) != pictures_.end();
On 2016/06/16 22:09:58, vmpstr wrote:
> inline this wherever it's used

Done.

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.cc:73: cc::PictureData
picture_data;
On 2016/06/16 22:09:58, vmpstr wrote:
> Can this have a constructor that takes two parameters, and then
> 
> pictures_[picture_data.unique_id] = cc::PictureData(picture->uniqueID(),
> stream.copyToData());?

Done.

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache_unittest.cc:43:
EXPECT_THAT(update, testing::UnorderedElementsAre(PictureMatches(picture1),
On 2016/06/16 22:09:59, vmpstr wrote:
> O_O TIL

:-)

https://codereview.chromium.org/1982893002/diff/240001/cc/layers/picture_laye...
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:70: if (!host)
On 2016/06/16 22:09:59, vmpstr wrote:
> Should this if also reset the cache on a recording_source_ if it exists?

Unnecessary now.

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
File cc/playback/display_item.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item.cc:14: EnginePictureCache* engine_picture_cache) const
{}
On 2016/06/16 22:09:59, vmpstr wrote:
> empty virtuals can be inlined iirc

Removed.

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.cc:53: uint32_t
GetUniqueSkPictureIDOrNothing(const proto::DisplayItem& item_proto) {
On 2016/06/16 22:09:59, vmpstr wrote:
> OrNothing is wrong here. It doesn't return nothing.

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.cc:88:
engine_picture_ids.push_back(GetUniqueSkPictureIDOrNothing(item_proto));
On 2016/06/16 22:09:59, vmpstr wrote:
> So this is meant to have a bunch of 0s in there?
> 
> Would this be better written as 
> 
> if (item_proto.type() == proto::DisplayItem::Type_Drawing) {
>   DCHEKC(item_proto.has_drawing_item());
>  
>
engine_picture_ids.push_back(GetDrawingDisplayItemId(item_proto.drawing_item()));
> } else {
>   // maybe just skip this case?
>   engine_picture_ids.push_back(0);
> }

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.cc:95:
list->engine_picture_ids_.swap(engine_picture_ids);
On 2016/06/16 22:09:59, vmpstr wrote:
> Sorry if I missed this, but why are storing engine_picture_ids_ on display
item
> list?

From the header where the list of IDs is declared:
"These IDs are intentionally kept separate because they are not needed while
walking the |items_| for raster."

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.cc:141: for (const auto& item : items_)
On 2016/06/16 22:09:59, vmpstr wrote:
> What I proposed earlier is to maybe provide this type of iterator publicly

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/display_it...
cc/playback/display_item_list.h:63: void
MarkForRegistrationEngine(EnginePictureCache* engine_picture_cache);
On 2016/06/16 22:09:59, vmpstr wrote:
> I'm really not a fan of having duplicate set of functions because this display
> item list can be on both the server and client... 
> 
> What I mean to say is that from the design perspective, these functions don't
> belong on a display item list.
> 
> Is it possible to instead add an iterator to the display item list that
iterates
> and returns the display items and then use that iterator to mark things in the
> engine/client directly?
> 
> for (auto& item : display_item_list)
>  engine_picture_cache_->MarkForRegistration(item.GetPicture());
> 
> with maybe display item having a virtual GetPicture that returns either
nullptr
> or a picture as appropriate..
> 
> WDYt? I'm not suggesting that you do this, but I think we need a better design
> instead of this double dispatch-esque approach. This is especially true since
> you have to have these functions in pairs depending on what cache you're
> registering this in. So if there's another alternative, that's fine as well.

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/drawing_di...
File cc/playback/drawing_display_item.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/drawing_di...
cc/playback/drawing_display_item.cc:77: if (picture_ && engine_picture_cache)
On 2016/06/16 22:09:59, vmpstr wrote:
> Maybe just expose picture_?

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/recording_...
File cc/playback/recording_source.h (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/playback/recording_...
cc/playback/recording_source.h:105: void MarkForRegistrationEngine();
On 2016/06/16 22:09:59, vmpstr wrote:
> Similarly here... is it possible to expose the iteration of the display items
> instead?
> 
> I know I said that I'd prefer you move these onto the raster source, but
> again... I'm really not a fan of having engine/client pointers on an object
that
> is otherwise independent of blimp and just represents a way to draw something
> into a canvas

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache.cc
File cc/proto/picture_cache.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.cc:9: PictureData::PictureData() {}
On 2016/06/16 22:09:59, vmpstr wrote:
> = default

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.cc:13: PictureData::~PictureData() {}
On 2016/06/16 22:09:59, vmpstr wrote:
> = default

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache.h
File cc/proto/picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
On 2016/06/16 22:09:59, vmpstr wrote:
> Why is this defined in cc/proto? This doesn't look like a
> serialization/conversion helper

Maybe it looks more like that now? Now it's only called from ToProto and
FromProto places. Happy to move it if you still feel like that's a good idea.

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.h:21: struct CC_EXPORT PictureData {
On 2016/06/16 22:09:59, vmpstr wrote:
> Each individual struct/class should have its own file.

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.h:22: PictureData();
On 2016/06/16 22:09:59, vmpstr wrote:
> remove this have unique_id = 0 below

C++ yells at me because it's a complex struct (the sk_sp). And if I remove only
PictureData() constructor, I can't construct it anymore with
PictureData data;

https://codereview.chromium.org/1982893002/diff/240001/cc/proto/picture_cache...
cc/proto/picture_cache.h:30: using PictureCacheUpdate =
std::vector<PictureData>;
On 2016/06/16 22:09:59, vmpstr wrote:
> Maybe just use std::vector<PictureData> wherever you have a
PictureCacheUpdate?
> It wasn't clear to me earlier what this was except that you could get a
> PictureData out of it.

Done.

https://codereview.chromium.org/1982893002/diff/240001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1982893002/diff/240001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:336: InitializePictureCacheForTesting();
On 2016/06/16 22:09:59, vmpstr wrote:
> I'm not sure the compiler will let you have this call in a non test code, even
> if it's only called from a ForTesting function... if it works it works

'git cl upload' asks me if I'm sure, but other than that it's fine. do you want
me to merge the contents of that function here?

https://codereview.chromium.org/1982893002/diff/280001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc
(right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_client_picture_cache_unittest.cc:27:
BlimpClientPictureCacheTest() : cache_(&FakeImageDecoder) {}
On 2016/06/21 21:35:04, Kevin M wrote:
> Needs an override dtor, otherwise members of BlimpClientPictureCacheTest won't
> be deleted.

Done.

https://codereview.chromium.org/1982893002/diff/280001/blimp/common/composito...
File blimp/common/compositor/reference_tracker.cc (right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/common/composito...
blimp/common/compositor/reference_tracker.cc:39: committed_.insert(key);
On 2016/06/21 21:35:04, Kevin M wrote:
> Add a one-line comment, like on 48?

Done.

https://codereview.chromium.org/1982893002/diff/280001/blimp/common/composito...
File blimp/common/compositor/reference_tracker_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/common/composito...
blimp/common/compositor/reference_tracker_unittest.cc:20: ReferenceTrackerTest()
{}
On 2016/06/21 21:35:04, Kevin M wrote:
> Add override dtor

Done.

https://codereview.chromium.org/1982893002/diff/280001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_engine_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:43: void
SerializePictureAndAddToCache(const SkPicture* picture);
On 2016/06/21 21:35:04, Kevin M wrote:
> You used "Put()" in BlobCache. How about "Put()" here too? IMO the cache part
is
> implied and the serialize aspect isn't really necessary to expose in the
> interface.

Done.

https://codereview.chromium.org/1982893002/diff/280001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_engine_picture_cache.h:46: bool
CacheContainsPicture(const SkPicture* picture) const;
On 2016/06/21 21:35:04, Kevin M wrote:
> "Cache" and "Picture" are both implied. How about just "Contains()" ?

Removed the whole method now.

https://codereview.chromium.org/1982893002/diff/280001/blimp/test/support/com...
File blimp/test/support/compositor/picture_cache_test_support.h (right):

https://codereview.chromium.org/1982893002/diff/280001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:11: #include
"third_party/skia/include/core/SkColor.h"
On 2016/06/21 21:35:04, Kevin M wrote:
> switch these to forward declares where possible?

Sadly, I need this for all of them :-/ SkColor is a number, but is defined in
that header. SkPicture is used in an sk_sp, so it needs the full type, and sk_sp
is declared in SkRefCnt.h...

https://codereview.chromium.org/1982893002/diff/280001/blimp/test/support/com...
blimp/test/support/compositor/picture_cache_test_support.h:32: bool
SamePicture(sk_sp<const SkPicture> picture,
On 2016/06/21 21:35:04, Kevin M wrote:
> nittiest of nits: PicturesEqual() ?

Done.

[commit-bot: I haz the power, 2016-06-24 11:35:14]

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

[commit-bot: I haz the power, 2016-06-24 11:35:16]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2016-06-24 18:58:32]

overall looks good

https://codereview.chromium.org/1982893002/diff/320001/cc/layers/picture_laye...
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:185: scoped_refptr<const DisplayItemList>
display_list =
i think you can 

const scoped_refptr<const DisplayItemList>& 

here... or maybe GetDisplayItemList can return a raw ptr?

https://codereview.chromium.org/1982893002/diff/320001/cc/layers/picture_laye...
File cc/layers/picture_layer_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/layers/picture_laye...
cc/layers/picture_layer_unittest.cc:103: return std::vector<uint32_t>();
super nit: I wonder if you're ruining rvo by having multiple return types

Maybe 

std::vector<uint32_t> ids;
if (!display_list)
  return ids;

...

return ids;?

https://codereview.chromium.org/1982893002/diff/320001/cc/playback/display_it...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/playback/display_it...
cc/playback/display_item_list_unittest.cc:89:
std::unique_ptr<EnginePictureCache> fake_engine_picture_cache_uniq =
in cc, we usually do

std::unique_ptr<Foo> foo = ...;
Foo* foo_ptr = foo.get();

or

Foo* raw_foo = foo.get();

https://codereview.chromium.org/1982893002/diff/320001/cc/playback/recording_...
File cc/playback/recording_source.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/playback/recording_...
cc/playback/recording_source.cc:214: scoped_refptr<const DisplayItemList>
RecordingSource::GetDisplayItemList() {
Make this return a raw ptr

https://codereview.chromium.org/1982893002/diff/320001/cc/proto/client_pictur...
File cc/proto/client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/proto/client_pictur...
cc/proto/client_picture_cache.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
I don't think this file should live in cc/proto

https://codereview.chromium.org/1982893002/diff/320001/cc/test/fake_client_pi...
File cc/test/fake_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/test/fake_client_pi...
cc/test/fake_client_picture_cache.h:24: std::vector<uint32_t>
GetAllUsedPictureIds();
return const& here.

https://codereview.chromium.org/1982893002/diff/320001/cc/test/picture_cache_...
File cc/test/picture_cache_model.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/test/picture_cache_...
cc/test/picture_cache_model.cc:40: sk_sp<const SkPicture>
PictureCacheModel::GetPicture(uint32_t unique_id) {
Does this need to be sk_sp? Can you just return a raw ptr?

[nyquist, 2016-06-24 20:31:06]

Description was changed from

==========
[blimp] Add SkPicture caching support.

This CL adds support for de-duplication of SkPicture blobs across
consecutive frames.

Previously all DrawingDisplayItems and their respective SkPictures were
transferred from the engine to the client whenever the frames that
contained them was sent to the client. This lead to the same SkPictures
being sent over and over.

This CL changes this behavior so the engine can assume that the client
always keeps around the contents of the interest rect since last commit
and only sends down the new SkPictures that occur in the interest rect.

Both the engine and the client agrees on what is currently part of the
interest rect, and as such they can separately control their own caches
accordingly.

This has the benefit of sending less data from the engine to the client
whenever two consecutive frames have a lot of the same content.

BUG=597811
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[blimp] Add SkPicture caching support.

This CL adds support for de-duplication of SkPicture blobs across
consecutive frames.

Previously all DrawingDisplayItems and their respective SkPictures were
transferred from the engine to the client whenever the frames that
contained them was sent to the client. This lead to the same SkPictures
being sent over and over.

This CL changes this behavior so the engine can assume that the client
always keeps around the contents of the interest rect since last commit
and only sends down the new SkPictures that occur in the interest rect.

Both the engine and the client agrees on what is currently part of the
interest rect, and as such they can separately control their own caches
accordingly.

This has the benefit of sending less data from the engine to the client
whenever two consecutive frames have a lot of the same content.

This CL also creates //cc/blimp which is to be used for blimp-related
code living in //cc.

BUG=597811
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[nyquist, 2016-06-24 20:31:23]

vmpstr: PTAL

https://codereview.chromium.org/1982893002/diff/320001/cc/layers/picture_laye...
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:185: scoped_refptr<const DisplayItemList>
display_list =
On 2016/06/24 18:58:32, vmpstr wrote:
> i think you can 
> 
> const scoped_refptr<const DisplayItemList>& 
> 
> here... or maybe GetDisplayItemList can return a raw ptr?

Done.

https://codereview.chromium.org/1982893002/diff/320001/cc/layers/picture_laye...
File cc/layers/picture_layer_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/layers/picture_laye...
cc/layers/picture_layer_unittest.cc:103: return std::vector<uint32_t>();
On 2016/06/24 18:58:32, vmpstr wrote:
> super nit: I wonder if you're ruining rvo by having multiple return types
> 
> Maybe 
> 
> std::vector<uint32_t> ids;
> if (!display_list)
>   return ids;
> 
> ...
> 
> return ids;?

Done.

https://codereview.chromium.org/1982893002/diff/320001/cc/playback/display_it...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/playback/display_it...
cc/playback/display_item_list_unittest.cc:89:
std::unique_ptr<EnginePictureCache> fake_engine_picture_cache_uniq =
On 2016/06/24 18:58:32, vmpstr wrote:
> in cc, we usually do
> 
> std::unique_ptr<Foo> foo = ...;
> Foo* foo_ptr = foo.get();
> 
> or
> 
> Foo* raw_foo = foo.get();

Done.

https://codereview.chromium.org/1982893002/diff/320001/cc/playback/recording_...
File cc/playback/recording_source.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/playback/recording_...
cc/playback/recording_source.cc:214: scoped_refptr<const DisplayItemList>
RecordingSource::GetDisplayItemList() {
On 2016/06/24 18:58:32, vmpstr wrote:
> Make this return a raw ptr

Done.

https://codereview.chromium.org/1982893002/diff/320001/cc/test/fake_client_pi...
File cc/test/fake_client_picture_cache.h (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/test/fake_client_pi...
cc/test/fake_client_picture_cache.h:24: std::vector<uint32_t>
GetAllUsedPictureIds();
On 2016/06/24 18:58:32, vmpstr wrote:
> return const& here.

Done.

https://codereview.chromium.org/1982893002/diff/320001/cc/test/picture_cache_...
File cc/test/picture_cache_model.cc (right):

https://codereview.chromium.org/1982893002/diff/320001/cc/test/picture_cache_...
cc/test/picture_cache_model.cc:40: sk_sp<const SkPicture>
PictureCacheModel::GetPicture(uint32_t unique_id) {
On 2016/06/24 18:58:32, vmpstr wrote:
> Does this need to be sk_sp? Can you just return a raw ptr?

That makes all the tests using it crash. It ends up as sk_sp<const SkPicture> in
drawing display item.

[nyquist, 2016-06-24 20:31:51]

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

[commit-bot: I haz the power, 2016-06-24 20:32:19]

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, 2016-06-24 21:32:36]

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

[commit-bot: I haz the power, 2016-06-24 21:32:38]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2016-06-24 23:05:29]

lgtm

[nyquist, 2016-06-24 23:07:40]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2016-06-24 23:07:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1982893002/#ps340001
(title: "Addressed comments from vmpstr, including adding //cc/blimp")

[commit-bot: I haz the power, 2016-06-24 23:08:07]

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, 2016-06-24 23:15:26]

Description was changed from

==========
[blimp] Add SkPicture caching support.

This CL adds support for de-duplication of SkPicture blobs across
consecutive frames.

Previously all DrawingDisplayItems and their respective SkPictures were
transferred from the engine to the client whenever the frames that
contained them was sent to the client. This lead to the same SkPictures
being sent over and over.

This CL changes this behavior so the engine can assume that the client
always keeps around the contents of the interest rect since last commit
and only sends down the new SkPictures that occur in the interest rect.

Both the engine and the client agrees on what is currently part of the
interest rect, and as such they can separately control their own caches
accordingly.

This has the benefit of sending less data from the engine to the client
whenever two consecutive frames have a lot of the same content.

This CL also creates //cc/blimp which is to be used for blimp-related
code living in //cc.

BUG=597811
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[blimp] Add SkPicture caching support.

This CL adds support for de-duplication of SkPicture blobs across
consecutive frames.

Previously all DrawingDisplayItems and their respective SkPictures were
transferred from the engine to the client whenever the frames that
contained them was sent to the client. This lead to the same SkPictures
being sent over and over.

This CL changes this behavior so the engine can assume that the client
always keeps around the contents of the interest rect since last commit
and only sends down the new SkPictures that occur in the interest rect.

Both the engine and the client agrees on what is currently part of the
interest rect, and as such they can separately control their own caches
accordingly.

This has the benefit of sending less data from the engine to the client
whenever two consecutive frames have a lot of the same content.

This CL also creates //cc/blimp which is to be used for blimp-related
code living in //cc.

BUG=597811
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-24 23:15:28]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-06-24 23:18:09]

Description was changed from

==========
[blimp] Add SkPicture caching support.

This CL adds support for de-duplication of SkPicture blobs across
consecutive frames.

Previously all DrawingDisplayItems and their respective SkPictures were
transferred from the engine to the client whenever the frames that
contained them was sent to the client. This lead to the same SkPictures
being sent over and over.

This CL changes this behavior so the engine can assume that the client
always keeps around the contents of the interest rect since last commit
and only sends down the new SkPictures that occur in the interest rect.

Both the engine and the client agrees on what is currently part of the
interest rect, and as such they can separately control their own caches
accordingly.

This has the benefit of sending less data from the engine to the client
whenever two consecutive frames have a lot of the same content.

This CL also creates //cc/blimp which is to be used for blimp-related
code living in //cc.

BUG=597811
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[blimp] Add SkPicture caching support.

This CL adds support for de-duplication of SkPicture blobs across
consecutive frames.

Previously all DrawingDisplayItems and their respective SkPictures were
transferred from the engine to the client whenever the frames that
contained them was sent to the client. This lead to the same SkPictures
being sent over and over.

This CL changes this behavior so the engine can assume that the client
always keeps around the contents of the interest rect since last commit
and only sends down the new SkPictures that occur in the interest rect.

Both the engine and the client agrees on what is currently part of the
interest rect, and as such they can separately control their own caches
accordingly.

This has the benefit of sending less data from the engine to the client
whenever two consecutive frames have a lot of the same content.

This CL also creates //cc/blimp which is to be used for blimp-related
code living in //cc.

BUG=597811
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/fbaee11e729589960ece321f39dee33e01503368
Cr-Commit-Position: refs/heads/master@{#402006}
==========

[commit-bot: I haz the power, 2016-06-24 23:18:11]

Patchset 18 (id:??) landed as
https://crrev.com/fbaee11e729589960ece321f39dee33e01503368
Cr-Commit-Position: refs/heads/master@{#402006}