Incorporate BlobChannel into Blimp image encode/decode pipeline.

Incorporate BlobChannel into Blimp image encode/decode pipeline.

* Add "BlobChannelSenderHost" class which encapsulates calls to Mojo
  and manages a renderer-resident copy of the cache state.
  Access to the cache state enables the renderer to avoid
  performing redundant operations such as resampling/transcoding
  if the data has previously been processed by another renderer.
* Modify Mojo interface to transmit blob payloads as strings,
  which is consistent with the C++ BlobData typedef.
* Remove BlobChannel/BlobCache calls from WebPDecoder (now it only
  decodes WebP images.)
* Make ClientImageSerializationProcessor into a singleton.
* Add getter path for BlobChannel sender for Engine browser process.

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

[Kevin M, 2016-05-16 21:24:54]

Still working out my approach for testing with this CL.

[Kevin M, 2016-05-17 20:52:17]

Heads up, I'll be uploading a patch that uses shared memory for blob IPCs.
Please hold off on reviewing until then.

[Kevin M, 2016-05-17 23:14:52]

Reviewers: please resume.

I added shared memory support, but the interface churn blew out the scope of
this CL. I will keep this review as-is, with blobs transmitted in-line with the
Mojo messages, and create a separate followup CL for shared memory blob
transport.

[Kevin M, 2016-05-20 18:33:10]

ping

[Wez, 2016-05-20 21:50:37]

nit: Your description has a duplicate title line.

[Wez, 2016-05-20 21:52:07]

Why do we want a per-renderer model of the cache?

Are we then maintaining a global cache in the browser process, and sub-sets in
each renderer process?

[Wez, 2016-05-20 21:52:44]

On 2016/05/20 21:52:07, Wez wrote:
> Why do we want a per-renderer model of the cache?
> 
> Are we then maintaining a global cache in the browser process, and sub-sets in
> each renderer process?

Important to explain _why_ we want a cache in the renderer process, essentially
- not obvious from the CL description, which describes the changes but does not
explain their purpose.

[Kevin M, 2016-05-20 23:50:06]

Description was changed from

==========
Incorporate BlobChannel into Blimp image encode/decode pipeline.

Incorporate BlobChannel into Blimp image encode/decode pipeline.
* Add "BlobChannelSenderHost" class which encapsulates calls to Mojo
  and manages a renderer-resident copy of the cache state.
* Modify Mojo interface to transmit blob payloads as strings,
  which is consistent with the C++ BlobData typedef.
* Remove BlobChannel/BlobCache calls from WebPDecoder (now it only
  decodes WebP images.)
* Make ClientImageSerializationProcessor into a singleton.
* Add getter path for BlobChannel sender for Engine browser process.

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

to

==========
Incorporate BlobChannel into Blimp image encode/decode pipeline.

Incorporate BlobChannel into Blimp image encode/decode pipeline.
* Add "BlobChannelSenderHost" class which encapsulates calls to Mojo
  and manages a renderer-resident copy of the cache state.
  Access to the cache state enables the renderer to avoid
  performing redundant operations such as resampling/transcoding
  if the data has previously been processed by another renderer.
* Modify Mojo interface to transmit blob payloads as strings,
  which is consistent with the C++ BlobData typedef.
* Remove BlobChannel/BlobCache calls from WebPDecoder (now it only
  decodes WebP images.)
* Make ClientImageSerializationProcessor into a singleton.
* Add getter path for BlobChannel sender for Engine browser process.

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

[Kevin M, 2016-05-20 23:50:29]

Added clarification to the CL description.

"Access to the cache state enables the renderer to avoid performing redundant
operations such as resampling/transcoding if the data has previously been
processed by another renderer."

[Wez, 2016-05-21 01:08:05]

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/client_image_serialization_processor.cc
(right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.cc:49:
LOG(WARNING) << "Couldn't parse blob metadata structure.";
This seems like an error, not a warning? Presumably if this happens we want to
abort the connection, since it's a protocol error?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.cc:53:
DLOG(ERROR) << "Image payload is not a valid blob ID.";
Why only DLOG(ERROR) - you are LOG(WARNING)ing above...

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.cc:63: <<
BlobIdToString(parsed_metadata.id());
While we have synchronous fetch isn't this also a protocol error?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/client_image_serialization_processor.h
(right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:24: class
ClientImageSerializationProcessor
nit: Should this be BlobImageSerializationProcessor now?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:27:
virtual ~ClientImageSerializationProcessor();
Why is the destructor public if this is a singleton?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:29:
static ClientImageSerializationProcessor* GetInstance();
Do we actually need this to be a singleton? Is it the case that we expect
multiple decoders to all need to reference it, or will we only have a single
decoder, in practice, at the client?

If the latter then consider moving to an owned object with checks to ensure that
there is at-most-one of it, rather than a lazy singleton.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:31: void
set_blob_receiver(BlobChannelReceiver* blob_receiver) {
nit: Clarify ownership/lifetime expectations of blob_receiver.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:40:
ClientImageSerializationProcessor();
nit: Move this below the friend.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:53:
BlobChannelReceiver* blob_receiver_ = nullptr;
nit: Comment to explain this member - how is it used?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/decoding_image_generator.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/decoding_image_generator.cc:40: DLOG(WARNING) <<
"GetAndDecodeBlob failed.";
All the failure code paths in GetAndDecodeBlob already log something; suggest
logging inside, or outside, but not both!

If GetAndDecodeBlob fails here then something is seriously wrong - can we
restructure this to allow us to propagate the failure and disconnect the session
in this case?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:16: #include
"blimp/client/feature/compositor/client_image_serialization_processor.h"
Non-wrapped line exceeds 80 chars?

ESTYLEGEDDON

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:170:
base::WrapUnique(blob_delegate_))),
Would be super duper awesome to avoid this gross pass-but-kinda-hold-on-to
dance.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.h:26: class BlobCache;
You don't seem to use this below?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.h:97: HeliumBlobReceiverDelegate*
blob_delegate_ = nullptr;
nit: Need a comment to explain this member, and in particular why it is bare.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_browser_main_parts.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_browser_main_parts.h:45: BlobChannelSender*
GetBlobSender();
nit: GetBlobChannelSender (or rename the class to BlobSender ;)

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

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel.mojom:9: // TODO(kmarshall): Use shared memory
for more efficient data transfer.
nit: File a bug for that work and include the bug # here.

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

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.cc:27: blob_channel_->DeliverBlob(id);
nit: Why do the BlobChannelSender and Service have different terminology for
these operations?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
File blimp/engine/mojo/blob_channel_service.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.h:23: explicit
BlobChannelService(BlobChannelSender* blob_channel,
nit: Drop "explicit" here.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.h:27: // Factory method called by Mojo.
Why do we need both the factory method and the constructor to be visible?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.h:39: BlobChannelSender* blob_channel_;
nit: Add one-line comment to explain what this is used for.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_host.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.cc:42: GetReplicationState(id) &
ReplicationState::ENGINE);
This DCHECK_NE is effectively just a bool test; you'd be better off doing a
plain DCHECK.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.cc:49: // Callers should only
call Push() iff the blob is already in the BlobChannel
nit: I've been asked in prior reviews not to use "iff" because apparently it's
not well-known.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_host.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:19: // renderer/browser IPC
boundary.
Communicating with the BlobChannel service doesn't tell me what this does - what
is this host object actually for?  Looks like it is basically the actual
BlobChannel send-side implementation, which renderer processes use to describe
blobs that need sharing with the client.

BTW, how do we ensure that requests to this service to place blobs are actually
processed before compositor traffic from the renderer, given that the two go
over different IPC channels?

Looking at the implementation this class just seems to be a trivial wrapper
around the Mojo channel, plus a replication-state table, and some annoying
DCHECKs. Could we have a single class presenting the BlobChannelSender interface
and have that take care of the send-if-necessary logic instead?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:23: // Flags can be combined
with boolean AND.
Give a little context as to what these actually mean, e.g. does ENGINE mean
"this blob exists in the engine cache, but not in the client's, yet" or
something else?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:33: int
GetReplicationState(const std::string& id) const;
Would it be cleaner to provide separate IsInClientCache() and IsInEngineCache()
methods?

What is this actually used for?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:36: // doesn't already contain
the blob.
It's not clear what "local portion" means here.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:43: void Push(const
std::string& id);
Put & Push are just mirroring the BlobChannelSender interface, right? So why not
actually provide that interface, so the caller just uses it and the blob is
delivered to the browser process only if it's not already there?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:46: // BlobChannel Mojo IPC
stub.
Which is used for...? Receiving the BlobChannel IPCs from the renderer?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:51: base::hash_map<std::string,
int> replication_state_;
Wait... so this class is used in the _renderer_?  Why is it called "...Host",
then?  Host is usually the suffix for browser-side classes that handle renderer
requests for some purpose.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:59: SkData*
onEncode(const SkPixmap& pixmap) override {
This function has various error-handling (invalid dimensions, failure to
initialize WebP structures) - is it ever realistic for them to fail?

If not then should we change those to CHECKs?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:72:
BlobChannelSenderHost::ReplicationState::ENGINE)) {
nit: Would be more readable if you added a simple helper for the replication
state, or even just broke it into a struct with two bools!

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:77: // Initialize
an empty WebPConfig.
nit: This comment and the init one below don't reallu add anything.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:215:
BlobChannelSenderHost* blob_channel_;
nit: Brief comment to clarify purpose & ownership.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:227:
pixel_serializer_.reset(new WebPImageEncoder(blob_channel_.get()));
nit: Is there a reason why the EngineImageSerializationProcessor can't also
implement the PixelSerializer interface, rather than have two separate objects?

Does the EISP need to retain ownership of the blob_channel, or could it be
move()d, to make ownership/lifetime clearer?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:222:
base::WrapUnique(blob_delegate_)))),
Ick! Really not keen on this thing of passing ownership but retaining a
bare-pointer.

Can we switch to having the delegate be supplied to the sender after
construction, for example?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.h:215: HeliumBlobSenderDelegate*
blob_delegate_;
Add a comment to explain what this is for.

[nyquist, 2016-05-24 00:05:41]

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:255: "renderer/blimp_content_renderer_client.h",
Nit: I don't think you need to list this twice. Also, interesting that our tools
don't inform us of this.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:381: "//blimp/net:test_support",
Nit: I don't think you need to list this twice.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_host.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:20: class BLIMP_COMMON_EXPORT
BlobChannelSenderHost {
This is a little bit confusing naming to me. In //content a '*Host' suffix
usually implies that this is the part that runs in the browser process.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:73: DVLOG(2) <<
"Encoding image color_type=" << pixmap.colorType()
This method mixes low-level bits and bytes with high-level concepts as
replication state. Could you see if there was a way for this method to be more
human readable at a high level? Maybe extracting out parts of the core pieces,
such as the WebP encoding itself?

As in, passing in the writer state to the encode function as a pointer and doing
the Put() on the outside?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.h:95: BlobChannelSender* blob_sender()
{ return blob_sender_.get(); }
Nit: blob_channel_sender() or rename class here as well?

[Kevin M, 2016-05-27 22:35:31]

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/client_image_serialization_processor.cc
(right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.cc:49:
LOG(WARNING) << "Couldn't parse blob metadata structure.";
On 2016/05/21 01:08:03, Wez wrote:
> This seems like an error, not a warning? Presumably if this happens we want to
> abort the connection, since it's a protocol error?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.cc:53:
DLOG(ERROR) << "Image payload is not a valid blob ID.";
On 2016/05/21 01:08:03, Wez wrote:
> Why only DLOG(ERROR) - you are LOG(WARNING)ing above...

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.cc:63: <<
BlobIdToString(parsed_metadata.id());
On 2016/05/21 01:08:03, Wez wrote:
> While we have synchronous fetch isn't this also a protocol error?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/client_image_serialization_processor.h
(right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:24: class
ClientImageSerializationProcessor
On 2016/05/21 01:08:03, Wez wrote:
> nit: Should this be BlobImageSerializationProcessor now?

I know they're in different namespaces, but it seems like that name could be
ambiguous with its serialization counterpart in Engine?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:27:
virtual ~ClientImageSerializationProcessor();
On 2016/05/21 01:08:03, Wez wrote:
> Why is the destructor public if this is a singleton?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:29:
static ClientImageSerializationProcessor* GetInstance();
On 2016/05/21 01:08:03, Wez wrote:
> Do we actually need this to be a singleton? Is it the case that we expect
> multiple decoders to all need to reference it, or will we only have a single
> decoder, in practice, at the client?
> 
> If the latter then consider moving to an owned object with checks to ensure
that
> there is at-most-one of it, rather than a lazy singleton.

The Skia decode callback is a raw function pointer which can't be associated
with any object instance. I think that means we'll need a singleton, or
something that's like a singleton, to connect the function with a stateful
object.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:31: void
set_blob_receiver(BlobChannelReceiver* blob_receiver) {
On 2016/05/21 01:08:03, Wez wrote:
> nit: Clarify ownership/lifetime expectations of blob_receiver.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:40:
ClientImageSerializationProcessor();
On 2016/05/21 01:08:03, Wez wrote:
> nit: Move this below the friend.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:53:
BlobChannelReceiver* blob_receiver_ = nullptr;
On 2016/05/21 01:08:03, Wez wrote:
> nit: Comment to explain this member - how is it used?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/decoding_image_generator.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/decoding_image_generator.cc:40: DLOG(WARNING) <<
"GetAndDecodeBlob failed.";
On 2016/05/21 01:08:03, Wez wrote:
> All the failure code paths in GetAndDecodeBlob already log something; suggest
> logging inside, or outside, but not both!
> 
> If GetAndDecodeBlob fails here then something is seriously wrong - can we
> restructure this to allow us to propagate the failure and disconnect the
session
> in this case?

Done, see set_error_delegate for error reporting.

Upgraded this to DLOG(FATAL) so that this will break on debug builds when
GetAndDecodeBlob fails.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:16: #include
"blimp/client/feature/compositor/client_image_serialization_processor.h"
On 2016/05/21 01:08:04, Wez wrote:
> Non-wrapped line exceeds 80 chars?
> 
> ESTYLEGEDDON

Linter thinks it's cool; I don't think that rule applies to includes?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:170:
base::WrapUnique(blob_delegate_))),
On 2016/05/21 01:08:04, Wez wrote:
> Would be super duper awesome to avoid this gross pass-but-kinda-hold-on-to
> dance.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.h:26: class BlobCache;
On 2016/05/21 01:08:04, Wez wrote:
> You don't seem to use this below?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.h:97: HeliumBlobReceiverDelegate*
blob_delegate_ = nullptr;
On 2016/05/21 01:08:04, Wez wrote:
> nit: Need a comment to explain this member, and in particular why it is bare.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:255: "renderer/blimp_content_renderer_client.h",
On 2016/05/24 00:05:41, nyquist wrote:
> Nit: I don't think you need to list this twice. Also, interesting that our
tools
> don't inform us of this.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:381: "//blimp/net:test_support",
On 2016/05/24 00:05:41, nyquist wrote:
> Nit: I don't think you need to list this twice.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_browser_main_parts.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_browser_main_parts.h:45: BlobChannelSender*
GetBlobSender();
On 2016/05/21 01:08:04, Wez wrote:
> nit: GetBlobChannelSender (or rename the class to BlobSender ;)

Done.

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

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel.mojom:9: // TODO(kmarshall): Use shared memory
for more efficient data transfer.
On 2016/05/21 01:08:04, Wez wrote:
> nit: File a bug for that work and include the bug # here.

Done.

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

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.cc:27: blob_channel_->DeliverBlob(id);
On 2016/05/21 01:08:04, Wez wrote:
> nit: Why do the BlobChannelSender and Service have different terminology for
> these operations?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
File blimp/engine/mojo/blob_channel_service.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.h:23: explicit
BlobChannelService(BlobChannelSender* blob_channel,
On 2016/05/21 01:08:04, Wez wrote:
> nit: Drop "explicit" here.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.h:27: // Factory method called by Mojo.
On 2016/05/21 01:08:04, Wez wrote:
> Why do we need both the factory method and the constructor to be visible?

Good question! Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/mojo/blob_...
blimp/engine/mojo/blob_channel_service.h:39: BlobChannelSender* blob_channel_;
On 2016/05/21 01:08:04, Wez wrote:
> nit: Add one-line comment to explain what this is used for.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_host.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.cc:42: GetReplicationState(id) &
ReplicationState::ENGINE);
On 2016/05/21 01:08:04, Wez wrote:
> This DCHECK_NE is effectively just a bool test; you'd be better off doing a
> plain DCHECK.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.cc:49: // Callers should only
call Push() iff the blob is already in the BlobChannel
On 2016/05/21 01:08:04, Wez wrote:
> nit: I've been asked in prior reviews not to use "iff" because apparently it's
> not well-known.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
File blimp/engine/renderer/blob_channel_sender_host.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:19: // renderer/browser IPC
boundary.
On 2016/05/21 01:08:04, Wez wrote:
> Communicating with the BlobChannel service doesn't tell me what this does -
what
> is this host object actually for?  Looks like it is basically the actual
> BlobChannel send-side implementation, which renderer processes use to describe
> blobs that need sharing with the client.

Description updated.

> BTW, how do we ensure that requests to this service to place blobs are
actually
> processed before compositor traffic from the renderer, given that the two go
> over different IPC channels?

The IPC channel is the same. Mojo MessagePipe writes are synchronous and
unbuffered, so they should arrive on the browser side before subsequent writes
to the channel.

> 
> Looking at the implementation this class just seems to be a trivial wrapper
> around the Mojo channel, plus a replication-state table, and some annoying
> DCHECKs. Could we have a single class presenting the BlobChannelSender
interface
> and have that take care of the send-if-necessary logic instead?

OK, I moved it to a BlobChannelSender, but it does mean that we are passing very
short lived refcounted BlobDataPtrs for the payload.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:20: class BLIMP_COMMON_EXPORT
BlobChannelSenderHost {
On 2016/05/24 00:05:41, nyquist wrote:
> This is a little bit confusing naming to me. In //content a '*Host' suffix
> usually implies that this is the part that runs in the browser process.

Done, it's now "proxy".

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:23: // Flags can be combined
with boolean AND.
On 2016/05/21 01:08:04, Wez wrote:
> Give a little context as to what these actually mean, e.g. does ENGINE mean
> "this blob exists in the engine cache, but not in the client's, yet" or
> something else?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:33: int
GetReplicationState(const std::string& id) const;
On 2016/05/21 01:08:04, Wez wrote:
> Would it be cleaner to provide separate IsInClientCache() and
IsInEngineCache()
> methods?
> 
> What is this actually used for?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:36: // doesn't already contain
the blob.
On 2016/05/21 01:08:04, Wez wrote:
> It's not clear what "local portion" means here.

Comments for implementation have been removed.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:43: void Push(const
std::string& id);
On 2016/05/21 01:08:04, Wez wrote:
> Put & Push are just mirroring the BlobChannelSender interface, right? So why
not
> actually provide that interface, so the caller just uses it and the blob is
> delivered to the browser process only if it's not already there?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:46: // BlobChannel Mojo IPC
stub.
On 2016/05/21 01:08:04, Wez wrote:
> Which is used for...? Receiving the BlobChannel IPCs from the renderer?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/b...
blimp/engine/renderer/blob_channel_sender_host.h:51: base::hash_map<std::string,
int> replication_state_;
On 2016/05/21 01:08:04, Wez wrote:
> Wait... so this class is used in the _renderer_?  Why is it called "...Host",
> then?  Host is usually the suffix for browser-side classes that handle
renderer
> requests for some purpose.

Acknowledged.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:59: SkData*
onEncode(const SkPixmap& pixmap) override {
On 2016/05/21 01:08:04, Wez wrote:
> This function has various error-handling (invalid dimensions, failure to
> initialize WebP structures) - is it ever realistic for them to fail?
> 
> If not then should we change those to CHECKs?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:72:
BlobChannelSenderHost::ReplicationState::ENGINE)) {
On 2016/05/21 01:08:05, Wez wrote:
> nit: Would be more readable if you added a simple helper for the replication
> state, or even just broke it into a struct with two bools!

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:73: DVLOG(2) <<
"Encoding image color_type=" << pixmap.colorType()
On 2016/05/24 00:05:41, nyquist wrote:
> This method mixes low-level bits and bytes with high-level concepts as
> replication state. Could you see if there was a way for this method to be more
> human readable at a high level? Maybe extracting out parts of the core pieces,
> such as the WebP encoding itself?
> 
> As in, passing in the writer state to the encode function as a pointer and
doing
> the Put() on the outside?

How's this?

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:77: // Initialize
an empty WebPConfig.
On 2016/05/21 01:08:04, Wez wrote:
> nit: This comment and the init one below don't reallu add anything.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:215:
BlobChannelSenderHost* blob_channel_;
On 2016/05/21 01:08:04, Wez wrote:
> nit: Brief comment to clarify purpose & ownership.

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:227:
pixel_serializer_.reset(new WebPImageEncoder(blob_channel_.get()));
On 2016/05/21 01:08:05, Wez wrote:
> nit: Is there a reason why the EngineImageSerializationProcessor can't also
> implement the PixelSerializer interface, rather than have two separate
objects?
> 
> Does the EISP need to retain ownership of the blob_channel, or could it be
> move()d, to make ownership/lifetime clearer?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:222:
base::WrapUnique(blob_delegate_)))),
On 2016/05/21 01:08:05, Wez wrote:
> Ick! Really not keen on this thing of passing ownership but retaining a
> bare-pointer.
> 
> Can we switch to having the delegate be supplied to the sender after
> construction, for example?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.h (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.h:95: BlobChannelSender* blob_sender()
{ return blob_sender_.get(); }
On 2016/05/24 00:05:41, nyquist wrote:
> Nit: blob_channel_sender() or rename class here as well?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.h:215: HeliumBlobSenderDelegate*
blob_delegate_;
On 2016/05/21 01:08:05, Wez wrote:
> Add a comment to explain what this is for.

Done.

[nyquist, 2016-06-03 18:30:44]

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

https://codereview.chromium.org/1985863002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.h:99:
ClientImageSerializationProcessor* image_serialization_processor_;
Nit: Do we want to mention any thing about this being a singleton here, to
clarify the lifetime of the object?

https://codereview.chromium.org/1985863002/diff/60001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1985863002/diff/60001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:201:
std::unique_ptr<HeliumBlobReceiverDelegate> blob_delegate(
You seem to do these operations in the constructor args above as well, is that
intentional?

https://codereview.chromium.org/1985863002/diff/60001/blimp/engine/renderer/e...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1985863002/diff/60001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:198: // the memory
used by the picture object itself.
Nit: Make two lines?

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

nyquist: PTAL
wez: pingggggg!

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

https://codereview.chromium.org/1985863002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.h:99:
ClientImageSerializationProcessor* image_serialization_processor_;
On 2016/06/03 18:30:44, nyquist wrote:
> Nit: Do we want to mention any thing about this being a singleton here, to
> clarify the lifetime of the object?

I'm not sure if that's necessary, given that the variable is bound inside the
module instead of being taken from a caller. IMO lifetime constraints are more
important if they are a part of the public interface, so that the object owner
knows to handle the object lifetimes properly.

If you feel strongly I can still document it, but I'm just explaining my
rationale. :)

https://codereview.chromium.org/1985863002/diff/60001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1985863002/diff/60001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:201:
std::unique_ptr<HeliumBlobReceiverDelegate> blob_delegate(
On 2016/06/03 18:30:44, nyquist wrote:
> You seem to do these operations in the constructor args above as well, is that
> intentional?

Uh, whoops.

Done.

https://codereview.chromium.org/1985863002/diff/60001/blimp/engine/renderer/e...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1985863002/diff/60001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:198: // the memory
used by the picture object itself.
On 2016/06/03 18:30:44, nyquist wrote:
> Nit: Make two lines?

Done.

[nyquist, 2016-06-06 21:10:46]

lgtm!

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

https://codereview.chromium.org/1985863002/diff/60001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.h:99:
ClientImageSerializationProcessor* image_serialization_processor_;
On 2016/06/06 17:43:38, Kevin M wrote:
> On 2016/06/03 18:30:44, nyquist wrote:
> > Nit: Do we want to mention any thing about this being a singleton here, to
> > clarify the lifetime of the object?
> 
> I'm not sure if that's necessary, given that the variable is bound inside the
> module instead of being taken from a caller. IMO lifetime constraints are more
> important if they are a part of the public interface, so that the object owner
> knows to handle the object lifetimes properly.
> 
> If you feel strongly I can still document it, but I'm just explaining my
> rationale. :)

Nah, I agree.

[Wez, 2016-06-07 01:18:32]

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/client_image_serialization_processor.h
(right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:24: class
ClientImageSerializationProcessor
On 2016/05/27 22:35:29, Kevin M wrote:
> On 2016/05/21 01:08:03, Wez wrote:
> > nit: Should this be BlobImageSerializationProcessor now?
> 
> I know they're in different namespaces, but it seems like that name could be
> ambiguous with its serialization counterpart in Engine?

My point is that names ssociating them w/ Client & Engine are not necessarily a
good thing; we could in future pass Blobs from Client->Engine, for example -
this class' function isn't intrinsically tied to the Client, it just enables
Skia in the calling process to fetch imagery that has been pushed into our local
BlobCache, right?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:29:
static ClientImageSerializationProcessor* GetInstance();
On 2016/05/27 22:35:29, Kevin M wrote:
> On 2016/05/21 01:08:03, Wez wrote:
> > Do we actually need this to be a singleton? Is it the case that we expect
> > multiple decoders to all need to reference it, or will we only have a single
> > decoder, in practice, at the client?
> > 
> > If the latter then consider moving to an owned object with checks to ensure
> that
> > there is at-most-one of it, rather than a lazy singleton.
> 
> The Skia decode callback is a raw function pointer which can't be associated
> with any object instance. I think that means we'll need a singleton, or
> something that's like a singleton, to connect the function with a stateful
> object.

Right; I get that we need a function to
get-the-current-process-wide-serialization-processor but does it need to be a
lazily-constructed singleton, as it is here?

Could it instead follow a pattern like MessageLoop, where the caller manages the
lifetime, and internally it sets itself as the "current" processor, for the
decode callback to use?

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/decoding_image_generator.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/decoding_image_generator.cc:40: DLOG(WARNING) <<
"GetAndDecodeBlob failed.";
On 2016/05/27 22:35:29, Kevin M wrote:
> On 2016/05/21 01:08:03, Wez wrote:
> > All the failure code paths in GetAndDecodeBlob already log something;
suggest
> > logging inside, or outside, but not both!
> > 
> > If GetAndDecodeBlob fails here then something is seriously wrong - can we
> > restructure this to allow us to propagate the failure and disconnect the
> session
> > in this case?
> 
> Done, see set_error_delegate for error reporting.
> 
> Upgraded this to DLOG(FATAL) so that this will break on debug builds when
> GetAndDecodeBlob fails.

I don't understand why we need the error delegate on the
ClientImageSerializationProcessor - isn't it effectively the same as the false
return-value here, in which case why does this class not handle it?  Why do we
need both the delegate and the return-value?

nit: Seems redundant to say "GetAndDecodeBlob() failed" here - you might just
do:

bool result = GetAndDecodeBlob()
DCHECK(result);

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:16: #include
"blimp/client/feature/compositor/client_image_serialization_processor.h"
On 2016/05/27 22:35:30, Kevin M wrote:
> On 2016/05/21 01:08:04, Wez wrote:
> > Non-wrapped line exceeds 80 chars?
> > 
> > ESTYLEGEDDON
> 
> Linter thinks it's cool; I don't think that rule applies to includes?

Yup, i think not.

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:222:
base::WrapUnique(blob_delegate_)))),
On 2016/05/27 22:35:31, Kevin M wrote:
> On 2016/05/21 01:08:05, Wez wrote:
> > Ick! Really not keen on this thing of passing ownership but retaining a
> > bare-pointer.
> > 
> > Can we switch to having the delegate be supplied to the sender after
> > construction, for example?
> 
> Done.

Looks like you're still doing the same thing, except that you're doing it in the
constructor body rather than in the initializers.

What I meant was that RegisterFeatures() is the only user of |blob_delegate_|,
so could you instantiate the BlobChannelSenderImpl and pass it the
BlobSenderDelegate _after_ having registered the delegate with the
ThreadPipeManager..?

https://codereview.chromium.org/1985863002/diff/80001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:297:
base::Unretained(net_components_.get())));
DropCurrentConnection() is expected to be called at-most-once, so don't we need
a check here, or in the ClientNetworkComponents, to protected against multiple
calls?

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:123: return
nullptr;
Can these ever fail in reality, or would it indicate a caller error? i.e. is the
|pixmap| generated directly from content-supplied fields, or by Blink code that
we can assume creates valid data? If the latter then consider [D]CHECKing here.

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:144: SkData*
sk_data = BlobCacheImageMetadataProtoAsSkData(proto);
Under what circumstances can this call actually fail?

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:169: LOG(FATAL) <<
"PlatformPictureImport() failed.";
Don't mix CHECK() and equivalents in with error-returns like this, except in the
special-case that we're dealing with data received from a potentially-malicious
source.

In this case we presumably expect that WebPConfigInit, WebPPictureInit and
PlatformPictureImport will all always succeed - only the latter appears to
actually depend on the supplied pixmap, so it's actually the only one I could
conceivably see failing at run-time due to anything other than a coding error.

So I think these should all be CHECK()s?

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:201: LOG(FATAL) <<
"WebPPictureFree() failed.";
Looks like this means WebPEncode() failed, not WebPPictureFree()?

https://codereview.chromium.org/1985863002/diff/80001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler.h (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:55: void DropCurrentConnection();
This is only valid to call while there is a connection active, so that
limitation at least needs to be documented here.  e.g. What would happen if we
had several image-decode operations in a row fail? Wouldn't that result in
several DropCurrentConnection calls?

[Kevin M, 2016-06-08 00:21:09]

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/client_image_serialization_processor.h
(right):

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:24: class
ClientImageSerializationProcessor
On 2016/06/07 01:18:31, Wez wrote:
> On 2016/05/27 22:35:29, Kevin M wrote:
> > On 2016/05/21 01:08:03, Wez wrote:
> > > nit: Should this be BlobImageSerializationProcessor now?
> > 
> > I know they're in different namespaces, but it seems like that name could be
> > ambiguous with its serialization counterpart in Engine?
> 
> My point is that names ssociating them w/ Client & Engine are not necessarily
a
> good thing; we could in future pass Blobs from Client->Engine, for example -
> this class' function isn't intrinsically tied to the Client, it just enables
> Skia in the calling process to fetch imagery that has been pushed into our
local
> BlobCache, right?

Done.

https://codereview.chromium.org/1985863002/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/client_image_serialization_processor.h:29:
static ClientImageSerializationProcessor* GetInstance();
On 2016/06/07 01:18:31, Wez wrote:
> On 2016/05/27 22:35:29, Kevin M wrote:
> > On 2016/05/21 01:08:03, Wez wrote:
> > > Do we actually need this to be a singleton? Is it the case that we expect
> > > multiple decoders to all need to reference it, or will we only have a
single
> > > decoder, in practice, at the client?
> > > 
> > > If the latter then consider moving to an owned object with checks to
ensure
> > that
> > > there is at-most-one of it, rather than a lazy singleton.
> > 
> > The Skia decode callback is a raw function pointer which can't be associated
> > with any object instance. I think that means we'll need a singleton, or
> > something that's like a singleton, to connect the function with a stateful
> > object.
> 
> Right; I get that we need a function to
> get-the-current-process-wide-serialization-processor but does it need to be a
> lazily-constructed singleton, as it is here?
> 
> Could it instead follow a pattern like MessageLoop, where the caller manages
the
> lifetime, and internally it sets itself as the "current" processor, for the
> decode callback to use?

Interesting, done.

https://codereview.chromium.org/1985863002/diff/80001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:297:
base::Unretained(net_components_.get())));
On 2016/06/07 01:18:31, Wez wrote:
> DropCurrentConnection() is expected to be called at-most-once, so don't we
need
> a check here, or in the ClientNetworkComponents, to protected against multiple
> calls?

I made BrowserConnectionHandler::DropCurrentConnection idempotent, how's that?

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:123: return
nullptr;
On 2016/06/07 01:18:31, Wez wrote:
> Can these ever fail in reality, or would it indicate a caller error? i.e. is
the
> |pixmap| generated directly from content-supplied fields, or by Blink code
that
> we can assume creates valid data? If the latter then consider [D]CHECKing
here.

Done.

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:144: SkData*
sk_data = BlobCacheImageMetadataProtoAsSkData(proto);
On 2016/06/07 01:18:31, Wez wrote:
> Under what circumstances can this call actually fail?

I don't think it can so I'll remove the DCHECK, unless +nyquist@chromium.org can
talk me out of it :)

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:169: LOG(FATAL) <<
"PlatformPictureImport() failed.";
On 2016/06/07 01:18:31, Wez wrote:
> Don't mix CHECK() and equivalents in with error-returns like this, except in
the
> special-case that we're dealing with data received from a
potentially-malicious
> source.
> 
> In this case we presumably expect that WebPConfigInit, WebPPictureInit and
> PlatformPictureImport will all always succeed - only the latter appears to
> actually depend on the supplied pixmap, so it's actually the only one I could
> conceivably see failing at run-time due to anything other than a coding error.
> 
> So I think these should all be CHECK()s?

Done.

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:201: LOG(FATAL) <<
"WebPPictureFree() failed.";
On 2016/06/07 01:18:31, Wez wrote:
> Looks like this means WebPEncode() failed, not WebPPictureFree()?

Done.

https://codereview.chromium.org/1985863002/diff/80001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler.h (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:55: void DropCurrentConnection();
On 2016/06/07 01:18:31, Wez wrote:
> This is only valid to call while there is a connection active, so that
> limitation at least needs to be documented here.  e.g. What would happen if we
> had several image-decode operations in a row fail? Wouldn't that result in
> several DropCurrentConnection calls?

See previous comment about idempotent connection drops.

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

pingggggggg
ggg
ggg

[Wez, 2016-06-10 22:29:54]

LGTM!

https://codereview.chromium.org/1985863002/diff/80001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:297:
base::Unretained(net_components_.get())));
On 2016/06/08 00:21:09, Kevin M wrote:
> On 2016/06/07 01:18:31, Wez wrote:
> > DropCurrentConnection() is expected to be called at-most-once, so don't we
> need
> > a check here, or in the ClientNetworkComponents, to protected against
multiple
> > calls?
> 
> I made BrowserConnectionHandler::DropCurrentConnection idempotent, how's that?

Acknowledged.

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:123: return
nullptr;
On 2016/06/08 00:21:09, Kevin M wrote:
> On 2016/06/07 01:18:31, Wez wrote:
> > Can these ever fail in reality, or would it indicate a caller error? i.e. is
> the
> > |pixmap| generated directly from content-supplied fields, or by Blink code
> that
> > we can assume creates valid data? If the latter then consider [D]CHECKing
> here.
> 
> Done.

I'd recommend using DCHECK_GT(width, 0) here, rather than DCHECK(width), for
clarity - SkPixmap::width() is an |int|, so the existing code wasn't strictly
correct.

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:144: SkData*
sk_data = BlobCacheImageMetadataProtoAsSkData(proto);
On 2016/06/08 00:21:09, Kevin M wrote:
> On 2016/06/07 01:18:31, Wez wrote:
> > Under what circumstances can this call actually fail?
> 
> I don't think it can so I'll remove the DCHECK, unless
mailto:+nyquist@chromium.org can
> talk me out of it :)

Acknowledged.

https://codereview.chromium.org/1985863002/diff/80001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler.h (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:55: void DropCurrentConnection();
On 2016/06/08 00:21:09, Kevin M wrote:
> On 2016/06/07 01:18:31, Wez wrote:
> > This is only valid to call while there is a connection active, so that
> > limitation at least needs to be documented here.  e.g. What would happen if
we
> > had several image-decode operations in a row fail? Wouldn't that result in
> > several DropCurrentConnection calls?
> 
> See previous comment about idempotent connection drops.

Acknowledged.

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
File blimp/client/feature/compositor/blob_image_serialization_processor.cc
(right):

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.cc:24: //
static
nit: Do we need a // static comment in front of member declarations? Seems
obvious, unlike the static-ness of member functions.

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.cc:29:
CHECK(current_instance_);
nit: DCHECK here and below?

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.cc:47:
DCHECK(bitmap);
nit: GetAndDecodeBlob will touch |input| at least, so is it worth DCHECKing them
explicitly here?

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
File blimp/client/feature/compositor/blob_image_serialization_processor.h
(right):

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.h:31: static
BlobImageSerializationProcessor* current();
nit: Brief comment to summarize that this returns a globally-allocated instance
that the caller must have created with an appropriate lifetime (are there any
threading constraints here, too?)

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.h:55: //
GetAndDecodeBlob().
nit: Suggest "to a GetAndDecodeBlob() call on the current processor." or
similar.

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.h:70: //
deletion. Only one BlobImageSerializationProcessor may live at a time.
nit: "be live" would read more naturally that "live"?

https://codereview.chromium.org/1985863002/diff/140001/blimp/net/browser_conn...
File blimp/net/browser_connection_handler.cc (right):

https://codereview.chromium.org/1985863002/diff/140001/blimp/net/browser_conn...
blimp/net/browser_connection_handler.cc:66:
output_buffer_->SetOutputProcessor(nullptr);
nit: You might consider adding an early-return in case that |connection_| is
null, to guard against some later change adding non-idempotent cleanup code.

[Kevin M, 2016-06-10 23:51:50]

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1985863002/diff/80001/blimp/engine/renderer/e...
blimp/engine/renderer/engine_image_serialization_processor.cc:144: SkData*
sk_data = BlobCacheImageMetadataProtoAsSkData(proto);
On 2016/06/10 22:29:54, Wez wrote:
> On 2016/06/08 00:21:09, Kevin M wrote:
> > On 2016/06/07 01:18:31, Wez wrote:
> > > Under what circumstances can this call actually fail?
> > 
> > I don't think it can so I'll remove the DCHECK, unless
> mailto:+nyquist@chromium.org can
> > talk me out of it :)
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
File blimp/client/feature/compositor/blob_image_serialization_processor.cc
(right):

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.cc:24: //
static
On 2016/06/10 22:29:54, Wez wrote:
> nit: Do we need a // static comment in front of member declarations? Seems
> obvious, unlike the static-ness of member functions.

Done.

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.cc:29:
CHECK(current_instance_);
On 2016/06/10 22:29:54, Wez wrote:
> nit: DCHECK here and below?

Done.

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.cc:47:
DCHECK(bitmap);
On 2016/06/10 22:29:54, Wez wrote:
> nit: GetAndDecodeBlob will touch |input| at least, so is it worth DCHECKing
them
> explicitly here?

Done.

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
File blimp/client/feature/compositor/blob_image_serialization_processor.h
(right):

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.h:31: static
BlobImageSerializationProcessor* current();
On 2016/06/10 22:29:54, Wez wrote:
> nit: Brief comment to summarize that this returns a globally-allocated
instance
> that the caller must have created with an appropriate lifetime (are there any
> threading constraints here, too?)

Done. No threading constraints - this class is meant to be callable from a
number of raster threads - the cache manages concurrent access w/a lock. I made
GetAndDecodeBlob() const to ensure that no object state can change.

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.h:55: //
GetAndDecodeBlob().
On 2016/06/10 22:29:54, Wez wrote:
> nit: Suggest "to a GetAndDecodeBlob() call on the current processor." or
> similar.

Done.

https://codereview.chromium.org/1985863002/diff/140001/blimp/client/feature/c...
blimp/client/feature/compositor/blob_image_serialization_processor.h:70: //
deletion. Only one BlobImageSerializationProcessor may live at a time.
On 2016/06/10 22:29:54, Wez wrote:
> nit: "be live" would read more naturally that "live"?

I had to read that in my head a few times phonetically before that made sense.
Dumb English language, arrr!

https://codereview.chromium.org/1985863002/diff/140001/blimp/net/browser_conn...
File blimp/net/browser_connection_handler.cc (right):

https://codereview.chromium.org/1985863002/diff/140001/blimp/net/browser_conn...
blimp/net/browser_connection_handler.cc:66:
output_buffer_->SetOutputProcessor(nullptr);
On 2016/06/10 22:29:54, Wez wrote:
> nit: You might consider adding an early-return in case that |connection_| is
> null, to guard against some later change adding non-idempotent cleanup code.

Done.

[Kevin M, 2016-06-10 23:52:44]

Description was changed from

==========
Incorporate BlobChannel into Blimp image encode/decode pipeline.

Incorporate BlobChannel into Blimp image encode/decode pipeline.
* Add "BlobChannelSenderHost" class which encapsulates calls to Mojo
  and manages a renderer-resident copy of the cache state.
  Access to the cache state enables the renderer to avoid
  performing redundant operations such as resampling/transcoding
  if the data has previously been processed by another renderer.
* Modify Mojo interface to transmit blob payloads as strings,
  which is consistent with the C++ BlobData typedef.
* Remove BlobChannel/BlobCache calls from WebPDecoder (now it only
  decodes WebP images.)
* Make ClientImageSerializationProcessor into a singleton.
* Add getter path for BlobChannel sender for Engine browser process.

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

to

==========
Incorporate BlobChannel into Blimp image encode/decode pipeline.

* Add "BlobChannelSenderHost" class which encapsulates calls to Mojo
  and manages a renderer-resident copy of the cache state.
  Access to the cache state enables the renderer to avoid
  performing redundant operations such as resampling/transcoding
  if the data has previously been processed by another renderer.
* Modify Mojo interface to transmit blob payloads as strings,
  which is consistent with the C++ BlobData typedef.
* Remove BlobChannel/BlobCache calls from WebPDecoder (now it only
  decodes WebP images.)
* Make ClientImageSerializationProcessor into a singleton.
* Add getter path for BlobChannel sender for Engine browser process.

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

[Kevin M, 2016-06-11 00:04:31]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-06-11 00:04:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1985863002/#ps180001
(title: "wez feedback")

[commit-bot: I haz the power, 2016-06-11 00:04:58]

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

[commit-bot: I haz the power, 2016-06-11 00:10:01]

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

[commit-bot: I haz the power, 2016-06-11 00:10:02]

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_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...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin M, 2016-06-11 00:37:00]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-06-11 00:37:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1985863002/#ps200001
(title: "GN dependency fix")

[commit-bot: I haz the power, 2016-06-11 00:37:43]

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

[commit-bot: I haz the power, 2016-06-11 00:43:28]

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

[commit-bot: I haz the power, 2016-06-11 00:43:30]

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_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin M, 2016-06-14 01:11:39]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-06-14 01:11:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1985863002/#ps220001
(title: "fix gn dependency warning & rebase")

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

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

[commit-bot: I haz the power, 2016-06-14 02:09:52]

Description was changed from

==========
Incorporate BlobChannel into Blimp image encode/decode pipeline.

* Add "BlobChannelSenderHost" class which encapsulates calls to Mojo
  and manages a renderer-resident copy of the cache state.
  Access to the cache state enables the renderer to avoid
  performing redundant operations such as resampling/transcoding
  if the data has previously been processed by another renderer.
* Modify Mojo interface to transmit blob payloads as strings,
  which is consistent with the C++ BlobData typedef.
* Remove BlobChannel/BlobCache calls from WebPDecoder (now it only
  decodes WebP images.)
* Make ClientImageSerializationProcessor into a singleton.
* Add getter path for BlobChannel sender for Engine browser process.

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

to

==========
Incorporate BlobChannel into Blimp image encode/decode pipeline.

* Add "BlobChannelSenderHost" class which encapsulates calls to Mojo
  and manages a renderer-resident copy of the cache state.
  Access to the cache state enables the renderer to avoid
  performing redundant operations such as resampling/transcoding
  if the data has previously been processed by another renderer.
* Modify Mojo interface to transmit blob payloads as strings,
  which is consistent with the C++ BlobData typedef.
* Remove BlobChannel/BlobCache calls from WebPDecoder (now it only
  decodes WebP images.)
* Make ClientImageSerializationProcessor into a singleton.
* Add getter path for BlobChannel sender for Engine browser process.

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

[commit-bot: I haz the power, 2016-06-14 02:09:54]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-06-14 02:10:05]

CQ bit was unchecked

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

Description was changed from

==========
Incorporate BlobChannel into Blimp image encode/decode pipeline.

* Add "BlobChannelSenderHost" class which encapsulates calls to Mojo
  and manages a renderer-resident copy of the cache state.
  Access to the cache state enables the renderer to avoid
  performing redundant operations such as resampling/transcoding
  if the data has previously been processed by another renderer.
* Modify Mojo interface to transmit blob payloads as strings,
  which is consistent with the C++ BlobData typedef.
* Remove BlobChannel/BlobCache calls from WebPDecoder (now it only
  decodes WebP images.)
* Make ClientImageSerializationProcessor into a singleton.
* Add getter path for BlobChannel sender for Engine browser process.

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

to

==========
Incorporate BlobChannel into Blimp image encode/decode pipeline.

* Add "BlobChannelSenderHost" class which encapsulates calls to Mojo
  and manages a renderer-resident copy of the cache state.
  Access to the cache state enables the renderer to avoid
  performing redundant operations such as resampling/transcoding
  if the data has previously been processed by another renderer.
* Modify Mojo interface to transmit blob payloads as strings,
  which is consistent with the C++ BlobData typedef.
* Remove BlobChannel/BlobCache calls from WebPDecoder (now it only
  decodes WebP images.)
* Make ClientImageSerializationProcessor into a singleton.
* Add getter path for BlobChannel sender for Engine browser process.

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

Committed: https://crrev.com/27ea310df5b26c303575e1c7c401045e3f51f8af
Cr-Commit-Position: refs/heads/master@{#399639}
==========

[commit-bot: I haz the power, 2016-06-14 03:40:10]

Patchset 12 (id:??) landed as
https://crrev.com/27ea310df5b26c303575e1c7c401045e3f51f8af
Cr-Commit-Position: refs/heads/master@{#399639}