Add Mojo IPC for seeding new Renderer with Browser's cached blob state.

Add Mojo IPC for seeding new Renderer with Browser's cached blob state.

Cache key state is currently stored in the renderer and is used
by the image processor to determine which images should be transcoded
and sent to the client, and which images do not need to be sent,
reducing latency and compute resources. A drawback of storing this
state in the renderer is that the cache state is dumped when the
renderer is torn down, as in the case when the user navigates
to another site.

This patch provides the renderer with a snapshot of the browser's
cache state, which means that the knowledge of cache state is
retained when the renderer of a given site is torn down
and brought back again.

* Add GetCacheState() to BlobCache
* Add GetCacheState() to BlobChannelSender()
* Add GetCacheState() to .mojom file.

R=nyquist@chromium.org,wez@chromium.org
BUG=600719
Committed: https://crrev.com/2cfb64fdb531ebf178913ccda75d8b6954b9bae9
Cr-Commit-Position: refs/heads/master@{#406658}

[Kevin M, 2016-06-10 22:09:14]

Unit tests forthcoming, in a followup patch.

[Wez, 2016-06-21 00:33:33]

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
File blimp/common/blob_cache/blob_cache.h (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
blimp/common/blob_cache/blob_cache.h:29: virtual std::vector<BlobId>
GetCacheState() const = 0;
In general this seems like a potentially large structure to be generating and
returning?

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
blimp/common/blob_cache/blob_cache.h:29: virtual std::vector<BlobId>
GetCacheState() const = 0;
nit: GetCachedBlobIds()?

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
File blimp/common/blob_cache/in_memory_blob_cache.cc (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
blimp/common/blob_cache/in_memory_blob_cache.cc:15: std::vector<std::string>
InMemoryBlobCache::GetCacheState() const {
string->BlobId

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
File blimp/common/blob_cache/in_memory_blob_cache.h (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
blimp/common/blob_cache/in_memory_blob_cache.h:27: std::vector<std::string>
GetCacheState() const override;
std::string->BlobId

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
File blimp/engine/mojo/blob_channel.mojom (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel.mojom:8: // The non-human-readable ID of the
cache item.
nit: Is it relevant that it's not human-readable?

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel.mojom:12: bool is_replicated;
was_delivered, to match the terminology below?

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel.mojom:18: GetCacheState() =>
(array<CacheStateEntry> items);
You're calling this CacheStateEntry but then the parameter is "items" - should
it be "cache_state"?

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
File blimp/engine/mojo/blob_channel_service.cc (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.cc:27: // Get the cache state & marshal
the contents to the Mojo response.
"marshal the contents to ... response" sounds odd - you mean just pass them to
the |callback|?

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_proxy.cc (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_proxy.cc:33: // for IsInEngineCache()
and IsInClientCache().
This is a property the caller needs to be aware of, surely, so not really
something to document in the implementation?

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_proxy.cc:72: return
std::vector<BlobChannelSender::CacheState>();
Why not-implemented? Why not return a copy of replication_state_'s keys?

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_proxy.h (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_proxy.h:40: void
GotCacheState(mojo::Array<mojom::CacheStateEntryPtr> cache_state);
nit: OnGetCacheStateComplete()?

https://codereview.chromium.org/2056993003/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_sender.h (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_sender.h:17: struct CacheState {
nit: CacheStateEntry, for consistency w/ the Mojo?

[Kevin M, 2016-06-21 21:23:52]

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
File blimp/common/blob_cache/blob_cache.h (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
blimp/common/blob_cache/blob_cache.h:29: virtual std::vector<BlobId>
GetCacheState() const = 0;
On 2016/06/21 00:33:33, Wez wrote:
> In general this seems like a potentially large structure to be generating and
> returning?

Agreed, but in general I think if this saves us even a few image encodes, the
benefits might outweigh the cost?

I think we can do away with this approach once we get asynchronous image loading
in Skia, since renderer will be able to directly query the browser's cache state
using Contains() and postpone the encode/IPC until it receives a negative
result.

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
blimp/common/blob_cache/blob_cache.h:29: virtual std::vector<BlobId>
GetCacheState() const = 0;
On 2016/06/21 00:33:33, Wez wrote:
> nit: GetCachedBlobIds()?

Done.

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
File blimp/common/blob_cache/in_memory_blob_cache.cc (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
blimp/common/blob_cache/in_memory_blob_cache.cc:15: std::vector<std::string>
InMemoryBlobCache::GetCacheState() const {
On 2016/06/21 00:33:33, Wez wrote:
> string->BlobId

Done.

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
File blimp/common/blob_cache/in_memory_blob_cache.h (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
blimp/common/blob_cache/in_memory_blob_cache.h:27: std::vector<std::string>
GetCacheState() const override;
On 2016/06/21 00:33:33, Wez wrote:
> std::string->BlobId

Done.

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
File blimp/engine/mojo/blob_channel.mojom (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel.mojom:8: // The non-human-readable ID of the
cache item.
On 2016/06/21 00:33:33, Wez wrote:
> nit: Is it relevant that it's not human-readable?

I think so. We convert this ID to hex for logging, so there might be some
expectation from developers that the same ID will be used in both places.

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel.mojom:12: bool is_replicated;
On 2016/06/21 00:33:33, Wez wrote:
> was_delivered, to match the terminology below?

Done.

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel.mojom:18: GetCacheState() =>
(array<CacheStateEntry> items);
On 2016/06/21 00:33:33, Wez wrote:
> You're calling this CacheStateEntry but then the parameter is "items" - should
> it be "cache_state"?

Done.

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
File blimp/engine/mojo/blob_channel_service.cc (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.cc:27: // Get the cache state & marshal
the contents to the Mojo response.
On 2016/06/21 00:33:33, Wez wrote:
> "marshal the contents to ... response" sounds odd - you mean just pass them to
> the |callback|?

Done.

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_proxy.cc (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_proxy.cc:33: // for IsInEngineCache()
and IsInClientCache().
On 2016/06/21 00:33:33, Wez wrote:
> This is a property the caller needs to be aware of, surely, so not really
> something to document in the implementation?

Done, moved to header file.

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_proxy.cc:72: return
std::vector<BlobChannelSender::CacheState>();
On 2016/06/21 00:33:33, Wez wrote:
> Why not-implemented? Why not return a copy of replication_state_'s keys?

Done.

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_proxy.h (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_proxy.h:40: void
GotCacheState(mojo::Array<mojom::CacheStateEntryPtr> cache_state);
On 2016/06/21 00:33:33, Wez wrote:
> nit: OnGetCacheStateComplete()?

Done.

https://codereview.chromium.org/2056993003/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_sender.h (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_sender.h:17: struct CacheState {
On 2016/06/21 00:33:33, Wez wrote:
> nit: CacheStateEntry, for consistency w/ the Mojo?

Done.

[Wez, 2016-06-22 19:54:23]

LGTM w/ nits

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
File blimp/common/blob_cache/blob_cache.h (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/common/blob_cache...
blimp/common/blob_cache/blob_cache.h:29: virtual std::vector<BlobId>
GetCacheState() const = 0;
On 2016/06/21 21:23:51, Kevin M wrote:
> On 2016/06/21 00:33:33, Wez wrote:
> > In general this seems like a potentially large structure to be generating
and
> > returning?
> 
> Agreed, but in general I think if this saves us even a few image encodes, the
> benefits might outweigh the cost?
> 
> I think we can do away with this approach once we get asynchronous image
loading
> in Skia, since renderer will be able to directly query the browser's cache
state
> using Contains() and postpone the encode/IPC until it receives a negative
> result.

Acknowledged.

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
File blimp/engine/mojo/blob_channel.mojom (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel.mojom:8: // The non-human-readable ID of the
cache item.
On 2016/06/21 21:23:51, Kevin M wrote:
> On 2016/06/21 00:33:33, Wez wrote:
> > nit: Is it relevant that it's not human-readable?
> 
> I think so. We convert this ID to hex for logging, so there might be some
> expectation from developers that the same ID will be used in both places.

Hmmm; I'd argue that BlobId is intrinsically binary, and the fact that we log
hex is just that nothing else would make sense, but OK :)

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_proxy.cc (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_proxy.cc:72: return
std::vector<BlobChannelSender::CacheState>();
On 2016/06/21 21:23:52, Kevin M wrote:
> On 2016/06/21 00:33:33, Wez wrote:
> > Why not-implemented? Why not return a copy of replication_state_'s keys?
> 
> Done.

(Or is it the case that no-one will ever call this..?)

[Kevin M, 2016-06-22 23:52:53]

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_proxy.cc (right):

https://codereview.chromium.org/2056993003/diff/60001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_proxy.cc:72: return
std::vector<BlobChannelSender::CacheState>();
On 2016/06/22 19:54:23, Wez wrote:
> On 2016/06/21 21:23:52, Kevin M wrote:
> > On 2016/06/21 00:33:33, Wez wrote:
> > > Why not-implemented? Why not return a copy of replication_state_'s keys?
> > 
> > Done.
> 
> (Or is it the case that no-one will ever call this..?)

Yes, that is the case. <_<

[Kevin M, 2016-06-22 23:52:59]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-06-22 23:52:59]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2056993003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-22 23:53:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056993003/100001

[commit-bot: I haz the power, 2016-06-23 00:03:48]

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

[commit-bot: I haz the power, 2016-06-23 00:03:50]

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

[Kevin M, 2016-06-23 18:09:08]

kmarshall@chromium.org changed reviewers:
+ dcheng@chromium.org

[Kevin M, 2016-06-23 18:09:09]

+dcheng for Mojo change (IPC security review)

[dcheng, 2016-06-25 00:55:55]

https://codereview.chromium.org/2056993003/diff/140001/blimp/engine/mojo/blob...
File blimp/engine/mojo/blob_channel_service.cc (right):

https://codereview.chromium.org/2056993003/diff/140001/blimp/engine/mojo/blob...
blimp/engine/mojo/blob_channel_service.cc:34: converted->was_delivered =
cache_state[i].was_delivered;
Would it make sense to write a typemap for this rather than marshalling this
manually?

[dcheng, 2016-06-27 21:29:02]

On 2016/06/27 17:55:00, Kevin M wrote:

I clarify, I'm referring to
https://www.chromium.org/developers/design-documents/mojo/type-mapping, which is
distinct from writing a TypeConverter (which are actually deprecated, though
unfortunately, I don't think that's really noted anywhere).

[Kevin M, 2016-06-28 17:36:11]

That looks cool. I started going down that path, but it looks like a **lot** of
boilerplate for a little data pair that is only meant to be consumed by a couple
classes. I'll keep that approach in mind if we end up creating more elaborate
structs.

[Kevin M, 2016-06-28 17:39:14]

Reverted the deprecated type converter patch.

[dcheng, 2016-06-28 21:34:52]

On 2016/06/28 17:36:11, Kevin M wrote:
> That looks cool. I started going down that path, but it looks like a **lot**
of
> boilerplate for a little data pair that is only meant to be consumed by a
couple
> classes. I'll keep that approach in mind if we end up creating more elaborate
> structs.

For security, marshalling code should be in struct traits. However, I took
another look, and I think we can just send this directly as a map rather than a
vector of pairs? GetCachedBlobIds() can just return a map, then the mojo method
can just use map<string, bool>, and we don't need any structs.

Unfortunately, Mojo doesn't seamlessly transition between STL and mojo types
yet, but I suspect this will lead to even less boilerplate than the current
code, since you can just Swap() and PassStorage().

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

https://codereview.chromium.org/2056993003/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blob_channel_sender_proxy.cc:71:
replication_state_.clear();
Are there callbacks to update this state later? I'm just wondering if this will
collide with mutations in DeliverBlob() above.

[Kevin M, 2016-06-29 21:01:22]

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

https://codereview.chromium.org/2056993003/diff/180001/blimp/engine/renderer/...
blimp/engine/renderer/blob_channel_sender_proxy.cc:71:
replication_state_.clear();
On 2016/06/28 21:34:52, dcheng wrote:
> Are there callbacks to update this state later? I'm just wondering if this
will
> collide with mutations in DeliverBlob() above.

For now, this is called only once at the beginning of the renderer process'
lifetime, so we don't need to worry about collisions.

As the (temporary, in-memory) cache doesn't yet support evictions, we don't have
to add any listeners for cache invalidation events. When we do implement an
actual cache, we'll have to do it - probably by passing a delegate Interface
that allows the browser to push the cache state delta to the renderer.

[Kevin M, 2016-06-30 17:23:04]

Friendly ping! I'll be OOO soon, so this is a little time sensitive. (Ditto for
the other open Mojo CL.) Thanks.

[Wez, 2016-07-01 00:30:21]

LGTM w/ some naming nits.

https://codereview.chromium.org/2056993003/diff/200001/blimp/OWNERS
File blimp/OWNERS (left):

https://codereview.chromium.org/2056993003/diff/200001/blimp/OWNERS#oldcode5
blimp/OWNERS:5: haibinlu@chromium.org
nit: I'd generally recommend doing OWNERS updates as separate CLs, but up to
you.

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
File blimp/common/blob_cache/in_memory_blob_cache.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
blimp/common/blob_cache/in_memory_blob_cache.cc:16: std::vector<BlobId> output;
nit: Suggest cached_ids.

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
blimp/common/blob_cache/in_memory_blob_cache.cc:17: for (const auto& pair :
cache_) {
nit: Suggest blob_id_and_data_pair for clarity (since you are using auto, so the
type is not immediately clear without jumping to the header).

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
File blimp/engine/mojo/blob_channel.mojom (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
blimp/engine/mojo/blob_channel.mojom:8: // Gets the list of cache keys and their
replication status in the
nit: list of cached BlobIds?

Suggest clarifying the mapping of replication status onto bool - e.g. true means
the Client has it?

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
File blimp/engine/mojo/blob_channel_service.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
blimp/engine/mojo/blob_channel_service.cc:25: const
BlobChannelService::GetCachedBlobIdsCallback& callback) {
nit: |callback| seems unnecessarily generic - this is specifically the
"response" callback, so maybe call it "respond" or "response_callback"?

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
blimp/engine/mojo/blob_channel_service.cc:27: mojo::Map<mojo::String, bool>
output;
nit: cache_state, for consistency w/ other call sites?

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/renderer/...
File blimp/engine/renderer/blob_channel_sender_proxy.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/renderer/...
blimp/engine/renderer/blob_channel_sender_proxy.cc:27: // once cache
invalidations are introduced.
nit: Either refer to the bug for this TODO, or remove it, I think - no need for
a reminder in the code, as such, I don't think?

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/renderer/...
blimp/engine/renderer/blob_channel_sender_proxy.cc:69: ) {
nit: This is very strange looking line-wrapping! Is this what git cl format does
now?

https://codereview.chromium.org/2056993003/diff/200001/blimp/net/blob_channel...
File blimp/net/blob_channel/blob_channel_sender_impl.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/net/blob_channel...
blimp/net/blob_channel/blob_channel_sender_impl.cc:27: for (const std::string&
next_item_str : cache_state) {
nit: Why not cache_entry, or just cached_id or blob_id?

https://codereview.chromium.org/2056993003/diff/200001/blimp/net/blob_channel...
File blimp/net/blob_channel/blob_channel_sender_unittest.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/net/blob_channel...
blimp/net/blob_channel/blob_channel_sender_unittest.cc:100:
blob_sender_->DeliverBlob(blob_id1);
nit: It's strange to group these as a single logical code block, since one is
setting an expectation and the other is doing what the comment describes - move
the EXPECT up a line or two?

[dcheng, 2016-07-01 01:51:24]

On 2016/06/30 17:23:04, Kevin M wrote:
> Friendly ping! I'll be OOO soon, so this is a little time sensitive. (Ditto
for
> the other open Mojo CL.) Thanks.

Did you see my comment about using mojo::Map?

[nyquist, 2016-07-15 22:13:46]

lgtm

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
File blimp/common/blob_cache/in_memory_blob_cache_unittest.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:68: EXPECT_EQ(1u,
cache_state.size());
Nit: It feels a little bit weird of having two tests of a for-loop with only
ever returning the first element. I don't feel strongly though, it's a very
simple method anyway, but bonus points for having multiple entries at some
point.

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/renderer/...
File blimp/engine/renderer/blob_channel_sender_proxy.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/renderer/...
blimp/engine/renderer/blob_channel_sender_proxy.cc:27: // once cache
invalidations are introduced.
On 2016/07/01 00:30:20, Wez wrote:
> nit: Either refer to the bug for this TODO, or remove it, I think - no need
for
> a reminder in the code, as such, I don't think?

Yeah, and if there is no bug for it, just file one. Nowadays we should hopefully
also be able to do this by ourselves instead of waiting for fancy functionality
from Skia, pending the big change in how the compositor protocol works.

[Kevin M, 2016-07-18 16:58:17]

dcheng: I saw your comment about using mojo::Map() and did indeed incorporate
that into my .mojom. Perhaps you were looking at an old patch?

https://codereview.chromium.org/2056993003/diff/200001/blimp/OWNERS
File blimp/OWNERS (left):

https://codereview.chromium.org/2056993003/diff/200001/blimp/OWNERS#oldcode5
blimp/OWNERS:5: haibinlu@chromium.org
On 2016/07/01 00:30:20, Wez wrote:
> nit: I'd generally recommend doing OWNERS updates as separate CLs, but up to
> you.

Acknowledged.

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
File blimp/common/blob_cache/in_memory_blob_cache.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
blimp/common/blob_cache/in_memory_blob_cache.cc:16: std::vector<BlobId> output;
On 2016/07/01 00:30:20, Wez wrote:
> nit: Suggest cached_ids.

Done.

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
blimp/common/blob_cache/in_memory_blob_cache.cc:17: for (const auto& pair :
cache_) {
On 2016/07/01 00:30:20, Wez wrote:
> nit: Suggest blob_id_and_data_pair for clarity (since you are using auto, so
the
> type is not immediately clear without jumping to the header).

Done.

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
File blimp/common/blob_cache/in_memory_blob_cache_unittest.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/common/blob_cach...
blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:68: EXPECT_EQ(1u,
cache_state.size());
On 2016/07/15 22:13:46, nyquist wrote:
> Nit: It feels a little bit weird of having two tests of a for-loop with only
> ever returning the first element. I don't feel strongly though, it's a very
> simple method anyway, but bonus points for having multiple entries at some
> point.

Added multiple outputs to SimplePutContainsAndGetOperations

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
File blimp/engine/mojo/blob_channel.mojom (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
blimp/engine/mojo/blob_channel.mojom:8: // Gets the list of cache keys and their
replication status in the
On 2016/07/01 00:30:20, Wez wrote:
> nit: list of cached BlobIds?
> 
> Suggest clarifying the mapping of replication status onto bool - e.g. true
means
> the Client has it?

Done.

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
File blimp/engine/mojo/blob_channel_service.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
blimp/engine/mojo/blob_channel_service.cc:25: const
BlobChannelService::GetCachedBlobIdsCallback& callback) {
On 2016/07/01 00:30:20, Wez wrote:
> nit: |callback| seems unnecessarily generic - this is specifically the
> "response" callback, so maybe call it "respond" or "response_callback"?

Done.

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/mojo/blob...
blimp/engine/mojo/blob_channel_service.cc:27: mojo::Map<mojo::String, bool>
output;
On 2016/07/01 00:30:20, Wez wrote:
> nit: cache_state, for consistency w/ other call sites?

Done.

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/renderer/...
File blimp/engine/renderer/blob_channel_sender_proxy.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/renderer/...
blimp/engine/renderer/blob_channel_sender_proxy.cc:27: // once cache
invalidations are introduced.
On 2016/07/15 22:13:46, nyquist wrote:
> On 2016/07/01 00:30:20, Wez wrote:
> > nit: Either refer to the bug for this TODO, or remove it, I think - no need
> for
> > a reminder in the code, as such, I don't think?
> 
> Yeah, and if there is no bug for it, just file one. Nowadays we should
hopefully
> also be able to do this by ourselves instead of waiting for fancy
functionality
> from Skia, pending the big change in how the compositor protocol works.

Done.

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/renderer/...
blimp/engine/renderer/blob_channel_sender_proxy.cc:27: // once cache
invalidations are introduced.
On 2016/07/01 00:30:20, Wez wrote:
> nit: Either refer to the bug for this TODO, or remove it, I think - no need
for
> a reminder in the code, as such, I don't think?

Done.

https://codereview.chromium.org/2056993003/diff/200001/blimp/engine/renderer/...
blimp/engine/renderer/blob_channel_sender_proxy.cc:69: ) {
On 2016/07/01 00:30:21, Wez wrote:
> nit: This is very strange looking line-wrapping! Is this what git cl format
does
> now?

Re-ran git cl format, formatting is saner. I must've neglected to run it before
uploading the last patch.

https://codereview.chromium.org/2056993003/diff/200001/blimp/net/blob_channel...
File blimp/net/blob_channel/blob_channel_sender_impl.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/net/blob_channel...
blimp/net/blob_channel/blob_channel_sender_impl.cc:27: for (const std::string&
next_item_str : cache_state) {
On 2016/07/01 00:30:21, Wez wrote:
> nit: Why not cache_entry, or just cached_id or blob_id?

Done.

https://codereview.chromium.org/2056993003/diff/200001/blimp/net/blob_channel...
File blimp/net/blob_channel/blob_channel_sender_unittest.cc (right):

https://codereview.chromium.org/2056993003/diff/200001/blimp/net/blob_channel...
blimp/net/blob_channel/blob_channel_sender_unittest.cc:100:
blob_sender_->DeliverBlob(blob_id1);
On 2016/07/01 00:30:21, Wez wrote:
> nit: It's strange to group these as a single logical code block, since one is
> setting an expectation and the other is doing what the comment describes -
move
> the EXPECT up a line or two?

Done.

[dcheng, 2016-07-20 15:28:32]

Ah, I must have missed that.

Btw, the mojo default is to pass maps as stl types now: is it possible to just
use std::unordered_map all the way through now? I think you might have to delete
line https://cs.chromium.org/chromium/src/blimp/engine/BUILD.gn?rcl=0&l=368, but
the conversion should be relatively easy now. It'll be easier to do it now
rather than later.

LGTM if you can get that to work.

[Kevin M, 2016-07-20 19:45:26]

Nice! Switched to STL types. Submitting...

[Kevin M, 2016-07-20 19:45:38]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-07-20 19:45:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, wez@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2056993003/#ps240001
(title: "STL containers yay")

[commit-bot: I haz the power, 2016-07-20 19:46:11]

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

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-07-20 20:42:29]

Description was changed from

==========
Add Mojo IPC for seeding new Renderer with Browser's cached blob state.

Cache key state is currently stored in the renderer and is used
by the image processor to determine which images should be transcoded
and sent to the client, and which images do not need to be sent,
reducing latency and compute resources. A drawback of storing this
state in the renderer is that the cache state is dumped when the
renderer is torn down, as in the case when the user navigates
to another site.

This patch provides the renderer with a snapshot of the browser's
cache state, which means that the knowledge of cache state is
retained when the renderer of a given site is torn down
and brought back again.

* Add GetCacheState() to BlobCache
* Add GetCacheState() to BlobChannelSender()
* Add GetCacheState() to .mojom file.


R=nyquist@chromium.org,wez@chromium.org
BUG=600719
==========

to

==========
Add Mojo IPC for seeding new Renderer with Browser's cached blob state.

Cache key state is currently stored in the renderer and is used
by the image processor to determine which images should be transcoded
and sent to the client, and which images do not need to be sent,
reducing latency and compute resources. A drawback of storing this
state in the renderer is that the cache state is dumped when the
renderer is torn down, as in the case when the user navigates
to another site.

This patch provides the renderer with a snapshot of the browser's
cache state, which means that the knowledge of cache state is
retained when the renderer of a given site is torn down
and brought back again.

* Add GetCacheState() to BlobCache
* Add GetCacheState() to BlobChannelSender()
* Add GetCacheState() to .mojom file.

R=nyquist@chromium.org,wez@chromium.org
BUG=600719

Committed: https://crrev.com/2cfb64fdb531ebf178913ccda75d8b6954b9bae9
Cr-Commit-Position: refs/heads/master@{#406658}
==========

[commit-bot: I haz the power, 2016-07-20 20:42:30]

Patchset 13 (id:??) landed as
https://crrev.com/2cfb64fdb531ebf178913ccda75d8b6954b9bae9
Cr-Commit-Position: refs/heads/master@{#406658}