Register clipboard image blob in the browser process to copy data less.

Register clipboard image blob in the browser process to copy data less.

The original code encodes the PNG and sends the buffer over to the
renderer. The renderer then registers a blob with that buffer, which
transfers all the data back to the browser. Then the data is copied back
over to the renderer when the page tries to read the image/png
DataTransferItem.

Instead of sending the data back and forth so many times, just register
the blob directly in the browser process.

On a 1366x768 test image, there's no measurable difference: it takes
~0.125s to execute DataTransferItem::getAsFile() for an image.

On a 4000x4000 test image, there's a small difference: the old version
takes ~9.1s and the new version takes ~8.9s. It turns out memcpy is
pretty fast!

BUG=none

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

[dcheng, 2016-04-11 03:19:29]

Description was changed from

==========
Register clipboard image blob in the browser process to copy data less.

The original code encodes the PNG and sends the buffer over to the
renderer. The renderer then registers a blob with that buffer, which
transfers all the data back to the browser. Then the data is copied back
over to the renderer when the page tries to read the image/png
DataTransferItem.

Instead of sending the data back and forth so many times, just register
the blob directly in the browser process.

BUG=none
==========

to

==========
Register clipboard image blob in the browser process to copy data less.

The original code encodes the PNG and sends the buffer over to the
renderer. The renderer then registers a blob with that buffer, which
transfers all the data back to the browser. Then the data is copied back
over to the renderer when the page tries to read the image/png
DataTransferItem.

Instead of sending the data back and forth so many times, just register
the blob directly in the browser process.

On a 1366x768 test image, there's no measurable difference: it takes
~0.125s to execute DataTransferItem::getAsFile() for an image.

On a 4000x4000 test image, there's a small difference: the old version
takes ~9.1s and the new version takes ~8.9s. It turns out memcpy is
pretty fast!

BUG=none
==========

[dcheng, 2016-04-11 03:19:29]

dcheng@chromium.org changed reviewers:
+ jochen@chromium.org, kinuko@chromium.org

[dcheng, 2016-04-11 03:22:53]

dcheng@chromium.org changed reviewers:
+ tkent@chromium.org
- jochen@chromium.org

[dcheng, 2016-04-11 03:22:54]

I have some followup patches that reduce the main thread blocking time to ~3.5s
for a 4000x4000 image paste with some encoding tweaks. This is still
unacceptably long, but is much better than the ~9s today. At the end of the day,
it's hard to do much better with the current blob APIs: the requirement for
synchronously knowing size beforehand constrains the possible optimizations.

+kinuko for overall review, +tkent for //components/test_runner

https://codereview.chromium.org/1876653003/diff/40001/content/test/mock_webcl...
File content/test/mock_webclipboard_impl.cc (left):

https://codereview.chromium.org/1876653003/diff/40001/content/test/mock_webcl...
content/test/mock_webclipboard_impl.cc:105:
gfx::PNGCodec::Encode(static_cast<unsigned char*>(bitmap.getPixels()),
I don't really remember why I made it so complicated originally. I think the
simple version will work exactly the same.

https://codereview.chromium.org/1876653003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMockClipboard.h (right):

https://codereview.chromium.org/1876653003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMockClipboard.h:13: class WebMockClipboard
: public WebClipboard {
I'm not particularly thrilled about adding this class, but it seems the most
straightforward:
1) Without it, I need to figure out the unit test plumbing for blobs.
2) Even with the unit test plumbing, a lot of things only want the raw pixels
anyway and will just end up decoding the png. So adding this interface
simplifies those tests.

[dcheng, 2016-04-11 03:26:15]

cc dmurph as well, in case there are some blob apis that i missed that would
help make this work more async.

[tkent, 2016-04-11 03:27:27]

components/test_runner lgtm

[dmurph, 2016-04-11 06:39:13]

dmurph@chromium.org changed reviewers:
+ dmurph@chromium.org

[dmurph, 2016-04-11 06:39:14]

My main concern here (from the blob system) is the size of the data. We
currently still have the 500MB limit on blobs in the system. So large images
will just break here.

In the future (this quarter) I'll be creating disk-backed blob storage. The big
difference here is that the construction will have to be asynchronous, as we
might need to push stuff to disk before we hold onto that data. Can you, at
least, design the system in a way that would allow asynchronous construction?
Basically there will be a method like:

NotifyWhenBlobCanBeConstructed(size_t size, base::Closure callback)

(this won't be exactly right, but you might need to hold onto the data until I
ask for it).

One of the following will happen:
* We have space - it will be saved directly into memory immediately.
* We don't have space - we need to wait until memory is paged to disk.
* We don't have space and this image is > our memory limit - we save it
immediately to disk.

I believe this can be handled by the blob system, and this client example is
great for me to keep in mind. Most important question for you: Is this design
compatible with an async construction? I can definitely give you a handle back
immediately, but we may need to be saving the data in a callback for a bit.

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:220: // Give the
renderer a minute to pick up a reference to the blob before
I wish we had a better way of doing this :( I believe it's an issue when we send
blobs across renderer processes still. We would need some sort of 'ACK' system.

[dcheng, 2016-04-11 08:11:19]

On 2016/04/11 06:39:14, dmurph wrote:
> My main concern here (from the blob system) is the size of the data. We
> currently still have the 500MB limit on blobs in the system. So large images
> will just break here.

Yes, large images are definitely a problem. That being said, they were a problem
before this patch as well too: this patch is mainly focused on reducing the
badness of the system by eliminating unnecessary copies. I'm thinking about how
to address overly large images in a followup, but I don't have a good idea atm:
the current DataTransfer API doesn't allow for any way of signalling failures =(

Do we have any UMAs on how much space blob storage takes up / how often we hit
the limit?

> 
> In the future (this quarter) I'll be creating disk-backed blob storage. The
big
> difference here is that the construction will have to be asynchronous, as we
> might need to push stuff to disk before we hold onto that data. Can you, at
> least, design the system in a way that would allow asynchronous construction?
> Basically there will be a method like:
> 
> NotifyWhenBlobCanBeConstructed(size_t size, base::Closure callback)
> 
> (this won't be exactly right, but you might need to hold onto the data until I
> ask for it).
> 
> One of the following will happen:
> * We have space - it will be saved directly into memory immediately.
> * We don't have space - we need to wait until memory is paged to disk.
> * We don't have space and this image is > our memory limit - we save it
> immediately to disk.
> 
> I believe this can be handled by the blob system, and this client example is
> great for me to keep in mind. Most important question for you: Is this design
> compatible with an async construction? I can definitely give you a handle back
> immediately, but we may need to be saving the data in a callback for a bit.

As long as it's possible to return a blob handle to Blink without actually
waiting for the image encoding to finish, I should be able to accommodate the
new API.
Bonus request: if it's possible to return a handle without a pre-determine size,
that'd be even nicer, but I might be asking for too much...

> 
>
https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/clipboard_message_filter.cc (right):
> 
>
https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
> content/browser/renderer_host/clipboard_message_filter.cc:220: // Give the
> renderer a minute to pick up a reference to the blob before
> I wish we had a better way of doing this :( I believe it's an issue when we
send
> blobs across renderer processes still. We would need some sort of 'ACK'
system.

[kinuko, 2016-04-11 08:35:48]

Large image case would be problematic, but as Daniel noted this had existed
before this patch and this patch should be making the situation still better :)

The sync nature is required by the API, that's really unfortunate though.

A few more inline comments:

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:182: // TODO(dcheng):
Just use the worker pool.
Or just use
BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(SKIP_ON_SHUTDOWN)))
?

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:220: // Give the
renderer a minute to pick up a reference to the blob before
On 2016/04/11 06:39:14, dmurph wrote:
> I wish we had a better way of doing this :( I believe it's an issue when we
send
> blobs across renderer processes still. We would need some sort of 'ACK'
system.

:( couldn't we just add some ack IPC in this series of patch?

[dcheng, 2016-04-11 09:13:07]

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:182: // TODO(dcheng):
Just use the worker pool.
On 2016/04/11 08:35:47, kinuko wrote:
> Or just use
>
BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(SKIP_ON_SHUTDOWN)))
> ?

Done.

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:220: // Give the
renderer a minute to pick up a reference to the blob before
On 2016/04/11 08:35:47, kinuko wrote:
> On 2016/04/11 06:39:14, dmurph wrote:
> > I wish we had a better way of doing this :( I believe it's an issue when we
> send
> > blobs across renderer processes still. We would need some sort of 'ACK'
> system.
> 
> :( couldn't we just add some ack IPC in this series of patch?

I could add one, but we would still need a timeout to release the blob reference
if the renderer crashed. So I think it's slightly cleaner to just have the
timeout here. =/

[kinuko, 2016-04-11 09:52:02]

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:220: // Give the
renderer a minute to pick up a reference to the blob before
On 2016/04/11 09:13:07, dcheng wrote:
> On 2016/04/11 08:35:47, kinuko wrote:
> > On 2016/04/11 06:39:14, dmurph wrote:
> > > I wish we had a better way of doing this :( I believe it's an issue when
we
> > send
> > > blobs across renderer processes still. We would need some sort of 'ACK'
> > system.
> > 
> > :( couldn't we just add some ack IPC in this series of patch?
> 
> I could add one, but we would still need a timeout to release the blob
reference
> if the renderer crashed. So I think it's slightly cleaner to just have the
> timeout here. =/

We often keep these blob uuids in a per-host dispatcher host / message filter
and deref when the host's going away.  E.g.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/bl...

I think we could use the same technique here.

[dcheng, 2016-04-11 23:26:33]

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:220: // Give the
renderer a minute to pick up a reference to the blob before
On 2016/04/11 09:52:02, kinuko wrote:
> On 2016/04/11 09:13:07, dcheng wrote:
> > On 2016/04/11 08:35:47, kinuko wrote:
> > > On 2016/04/11 06:39:14, dmurph wrote:
> > > > I wish we had a better way of doing this :( I believe it's an issue when
> we
> > > send
> > > > blobs across renderer processes still. We would need some sort of 'ACK'
> > > system.
> > > 
> > > :( couldn't we just add some ack IPC in this series of patch?
> > 
> > I could add one, but we would still need a timeout to release the blob
> reference
> > if the renderer crashed. So I think it's slightly cleaner to just have the
> > timeout here. =/
> 
> We often keep these blob uuids in a per-host dispatcher host / message filter
> and deref when the host's going away.  E.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/bl...
> 
> I think we could use the same technique here.

Ah good point, I forgot that ClipboardMessageFilter is per-renderer.

That being said, it's not quite that simple. The "obvious" change here is to
make WebClipboard return a WebBlob. Then RendererClipboardDelegate can construct
a WebBlob (which refs the blob) before sending the ack IPC. Then we can return
this WebBlob through WebClipboard into Blink.

The problem is... WebClipboard lives in platform and WebBlob lives in web. It is
not trivial to move WebBlob to platform, since it depends on core.

Two alternatives:
1. Add an ack interface to WebClipboard. Seems pretty weird to do that though...
2. Expose BlobDataHandle in public/platform. Seems unfortunate to have both
WebBlobDataHandle and WebBlob though... they're very close in functionality,
except WebBlob also exposes the underlying script reference.

I'll probably try #2 for now, since I think #1 is worse. But I'm not too happy
about #2 either.

[dmurph, 2016-04-11 23:42:48]

On 2016/04/11 at 08:11:19, dcheng wrote:
> On 2016/04/11 06:39:14, dmurph wrote:
> > My main concern here (from the blob system) is the size of the data. We
> > currently still have the 500MB limit on blobs in the system. So large images
> > will just break here.
> 
> Yes, large images are definitely a problem. That being said, they were a
problem before this patch as well too: this patch is mainly focused on reducing
the badness of the system by eliminating unnecessary copies. I'm thinking about
how to address overly large images in a followup, but I don't have a good idea
atm: the current DataTransfer API doesn't allow for any way of signalling
failures =(
> 
> Do we have any UMAs on how much space blob storage takes up / how often we hit
the limit?

Yes.,
Storage.Blob.Broken, Storage.Blob.BrokenReason,
Storage.Blob.StorageSizeAfterAppend, Storage.Blob.StorageSizeBeforeAppend
https://uma.googleplex.com/p/chrome/histograms/?endDate=04-10-2016&dayCount=1...

Here's a speadsheet showing the storage distribution:
https://docs.google.com/spreadsheets/d/1ByKCjNmoBRfUvx6tyAQiBJGvJ_OK06eysZg71...

> > 
> > In the future (this quarter) I'll be creating disk-backed blob storage. The
big
> > difference here is that the construction will have to be asynchronous, as we
> > might need to push stuff to disk before we hold onto that data. Can you, at
> > least, design the system in a way that would allow asynchronous
construction?
> > Basically there will be a method like:
> > 
> > NotifyWhenBlobCanBeConstructed(size_t size, base::Closure callback)
> > 
> > (this won't be exactly right, but you might need to hold onto the data until
I
> > ask for it).
> > 
> > One of the following will happen:
> > * We have space - it will be saved directly into memory immediately.
> > * We don't have space - we need to wait until memory is paged to disk.
> > * We don't have space and this image is > our memory limit - we save it
> > immediately to disk.
> > 
> > I believe this can be handled by the blob system, and this client example is
> > great for me to keep in mind. Most important question for you: Is this
design
> > compatible with an async construction? I can definitely give you a handle
back
> > immediately, but we may need to be saving the data in a callback for a bit.
> 
> As long as it's possible to return a blob handle to Blink without actually
waiting for the image encoding to finish, I should be able to accommodate the
new API.
> Bonus request: if it's possible to return a handle without a pre-determine
size, that'd be even nicer, but I might be asking for too much...

Sorry, not possible, as we need to have the 'size' attribute on the blob object
available.

> > 
> >
https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
> > File content/browser/renderer_host/clipboard_message_filter.cc (right):
> > 
> >
https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
> > content/browser/renderer_host/clipboard_message_filter.cc:220: // Give the
> > renderer a minute to pick up a reference to the blob before
> > I wish we had a better way of doing this :( I believe it's an issue when we
send
> > blobs across renderer processes still. We would need some sort of 'ACK'
system.

Actually, since we're starting in the browser, we could probably follow the same
protocol as the renderer-created blob. I'm not sure what the API surface of this
would look like, but it might be useful to support this feature (basically say
"ConstructBlobForRenderer", which would leave the refcount at 1 and register it
for that renderer).

[dcheng, 2016-04-11 23:48:57]

On 2016/04/11 23:42:48, dmurph wrote:
> On 2016/04/11 at 08:11:19, dcheng wrote:
> > On 2016/04/11 06:39:14, dmurph wrote:
> > > My main concern here (from the blob system) is the size of the data. We
> > > currently still have the 500MB limit on blobs in the system. So large
images
> > > will just break here.
> > 
> > Yes, large images are definitely a problem. That being said, they were a
> problem before this patch as well too: this patch is mainly focused on
reducing
> the badness of the system by eliminating unnecessary copies. I'm thinking
about
> how to address overly large images in a followup, but I don't have a good idea
> atm: the current DataTransfer API doesn't allow for any way of signalling
> failures =(
> > 
> > Do we have any UMAs on how much space blob storage takes up / how often we
hit
> the limit?
> 
> Yes.,
> Storage.Blob.Broken, Storage.Blob.BrokenReason,
> Storage.Blob.StorageSizeAfterAppend, Storage.Blob.StorageSizeBeforeAppend
>
https://uma.googleplex.com/p/chrome/histograms/?endDate=04-10-2016&dayCount=1...
> 
> Here's a speadsheet showing the storage distribution:
>
https://docs.google.com/spreadsheets/d/1ByKCjNmoBRfUvx6tyAQiBJGvJ_OK06eysZg71...
> 
> > > 
> > > In the future (this quarter) I'll be creating disk-backed blob storage.
The
> big
> > > difference here is that the construction will have to be asynchronous, as
we
> > > might need to push stuff to disk before we hold onto that data. Can you,
at
> > > least, design the system in a way that would allow asynchronous
> construction?
> > > Basically there will be a method like:
> > > 
> > > NotifyWhenBlobCanBeConstructed(size_t size, base::Closure callback)
> > > 
> > > (this won't be exactly right, but you might need to hold onto the data
until
> I
> > > ask for it).
> > > 
> > > One of the following will happen:
> > > * We have space - it will be saved directly into memory immediately.
> > > * We don't have space - we need to wait until memory is paged to disk.
> > > * We don't have space and this image is > our memory limit - we save it
> > > immediately to disk.
> > > 
> > > I believe this can be handled by the blob system, and this client example
is
> > > great for me to keep in mind. Most important question for you: Is this
> design
> > > compatible with an async construction? I can definitely give you a handle
> back
> > > immediately, but we may need to be saving the data in a callback for a
bit.
> > 
> > As long as it's possible to return a blob handle to Blink without actually
> waiting for the image encoding to finish, I should be able to accommodate the
> new API.
> > Bonus request: if it's possible to return a handle without a pre-determine
> size, that'd be even nicer, but I might be asking for too much...
> 
> Sorry, not possible, as we need to have the 'size' attribute on the blob
object
> available.

Ah well. I figured as much.

> 
> > > 
> > >
>
https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
> > > File content/browser/renderer_host/clipboard_message_filter.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
> > > content/browser/renderer_host/clipboard_message_filter.cc:220: // Give the
> > > renderer a minute to pick up a reference to the blob before
> > > I wish we had a better way of doing this :( I believe it's an issue when
we
> send
> > > blobs across renderer processes still. We would need some sort of 'ACK'
> system.
> 
> Actually, since we're starting in the browser, we could probably follow the
same
> protocol as the renderer-created blob. I'm not sure what the API surface of
this
> would look like, but it might be useful to support this feature (basically say
> "ConstructBlobForRenderer", which would leave the refcount at 1 and register
it
> for that renderer).

This seems tricky: the corresponding renderer APIs would have to be updated to
have adoption semantics where appropriate.

[dmurph, 2016-04-12 00:10:23]

https://codereview.chromium.org/1876653003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/DataObjectItem.cpp (right):

https://codereview.chromium.org/1876653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/DataObjectItem.cpp:122: return
Blob::create(BlobDataHandle::create(blobInfo.uuid(), blobInfo.type(),
blobInfo.size()));
Hmmm.. Well if you went the register-blob-in-dispatcher-host-before-sending-it
approach, you would just call 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

right after creating the blob here. Doing it this (hacky) way would require
documentation on the browser saying "please decrement the refcount once from the
renderer once you create the handle there".

maybe that's not the best way though haha

[kinuko, 2016-04-12 05:27:28]

On 2016/04/11 23:26:33, dcheng wrote:
>
https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/clipboard_message_filter.cc (right):
> 
>
https://codereview.chromium.org/1876653003/diff/60001/content/browser/rendere...
> content/browser/renderer_host/clipboard_message_filter.cc:220: // Give the
> renderer a minute to pick up a reference to the blob before
> On 2016/04/11 09:52:02, kinuko wrote:
> > On 2016/04/11 09:13:07, dcheng wrote:
> > > On 2016/04/11 08:35:47, kinuko wrote:
> > > > On 2016/04/11 06:39:14, dmurph wrote:
> > > > > I wish we had a better way of doing this :( I believe it's an issue
when
> > we
> > > > send
> > > > > blobs across renderer processes still. We would need some sort of
'ACK'
> > > > system.
> > > > 
> > > > :( couldn't we just add some ack IPC in this series of patch?
> > > 
> > > I could add one, but we would still need a timeout to release the blob
> > reference
> > > if the renderer crashed. So I think it's slightly cleaner to just have the
> > > timeout here. =/
> > 
> > We often keep these blob uuids in a per-host dispatcher host / message
filter
> > and deref when the host's going away.  E.g.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/bl...
> > 
> > I think we could use the same technique here.
> 
> Ah good point, I forgot that ClipboardMessageFilter is per-renderer.
> 
> That being said, it's not quite that simple. The "obvious" change here is to
> make WebClipboard return a WebBlob. Then RendererClipboardDelegate can
construct
> a WebBlob (which refs the blob) before sending the ack IPC. Then we can return
> this WebBlob through WebClipboard into Blink.
> 
> The problem is... WebClipboard lives in platform and WebBlob lives in web. It
is
> not trivial to move WebBlob to platform, since it depends on core.
> 
> Two alternatives:
> 1. Add an ack interface to WebClipboard. Seems pretty weird to do that
though...
> 2. Expose BlobDataHandle in public/platform. Seems unfortunate to have both
> WebBlobDataHandle and WebBlob though... they're very close in functionality,
> except WebBlob also exposes the underlying script reference.
> 
> I'll probably try #2 for now, since I think #1 is worse. But I'm not too happy
> about #2 either.

We have WebBlobData in public/platform, we could probably add an exported method
that allows embedder/content to instantiate a new BlobData(Handle) at that
layer?

[dmurph, 2016-04-19 17:49:23]

Daniel, I think it's fair to say that this should work w/ the 1 minute timeout.
I think adding a TODO there and assigning it to me would be fair for adding this
new type of construction. I created a bug here:
crbug.com/604800

[dcheng, 2016-04-19 17:51:49]

On 2016/04/19 at 17:49:23, dmurph wrote:
> Daniel, I think it's fair to say that this should work w/ the 1 minute
timeout. I think adding a TODO there and assigning it to me would be fair for
adding this new type of construction. I created a bug here:
> crbug.com/604800

kinuko@, would you be OK with that? I haven't had time to add the
WebBlobDataHandle plumbing, and it feels a bit weird to add a one-off IPC in
clipboard for this, when it's really a more general issue.

[kinuko, 2016-04-20 06:00:15]

On 2016/04/19 17:51:49, dcheng wrote:
> On 2016/04/19 at 17:49:23, dmurph wrote:
> > Daniel, I think it's fair to say that this should work w/ the 1 minute
> timeout. I think adding a TODO there and assigning it to me would be fair for
> adding this new type of construction. I created a bug here:
> > crbug.com/604800
> 
> kinuko@, would you be OK with that? I haven't had time to add the
> WebBlobDataHandle plumbing, and it feels a bit weird to add a one-off IPC in
> clipboard for this, when it's really a more general issue.

Hm, ok.. if we're adding the TODO with a link to the issue Daniel (dmurph)
created I'm ok to land this as is.

[kinuko, 2016-04-20 06:06:16]

On 2016/04/20 06:00:15, kinuko wrote:
> On 2016/04/19 17:51:49, dcheng wrote:
> > On 2016/04/19 at 17:49:23, dmurph wrote:
> > > Daniel, I think it's fair to say that this should work w/ the 1 minute
> > timeout. I think adding a TODO there and assigning it to me would be fair
for
> > adding this new type of construction. I created a bug here:
> > > crbug.com/604800
> > 
> > kinuko@, would you be OK with that? I haven't had time to add the
> > WebBlobDataHandle plumbing, and it feels a bit weird to add a one-off IPC in
> > clipboard for this, when it's really a more general issue.
> 
> Hm, ok.. if we're adding the TODO with a link to the issue Daniel (dmurph)
> created I'm ok to land this as is.

lgtm assuming you're adding TODO before landing

[dcheng, 2016-04-20 19:26:24]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-04-20 19:26:25]

Added the TODO. I also removed a DCHECK_CURRENTLY_ON() that no longer makes
sense, since we just use the blocking pool now.

[dcheng, 2016-04-20 19:26:25]

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

[commit-bot: I haz the power, 2016-04-20 19:27:03]

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

[commit-bot: I haz the power, 2016-04-20 21:36:03]

Description was changed from

==========
Register clipboard image blob in the browser process to copy data less.

The original code encodes the PNG and sends the buffer over to the
renderer. The renderer then registers a blob with that buffer, which
transfers all the data back to the browser. Then the data is copied back
over to the renderer when the page tries to read the image/png
DataTransferItem.

Instead of sending the data back and forth so many times, just register
the blob directly in the browser process.

On a 1366x768 test image, there's no measurable difference: it takes
~0.125s to execute DataTransferItem::getAsFile() for an image.

On a 4000x4000 test image, there's a small difference: the old version
takes ~9.1s and the new version takes ~8.9s. It turns out memcpy is
pretty fast!

BUG=none
==========

to

==========
Register clipboard image blob in the browser process to copy data less.

The original code encodes the PNG and sends the buffer over to the
renderer. The renderer then registers a blob with that buffer, which
transfers all the data back to the browser. Then the data is copied back
over to the renderer when the page tries to read the image/png
DataTransferItem.

Instead of sending the data back and forth so many times, just register
the blob directly in the browser process.

On a 1366x768 test image, there's no measurable difference: it takes
~0.125s to execute DataTransferItem::getAsFile() for an image.

On a 4000x4000 test image, there's a small difference: the old version
takes ~9.1s and the new version takes ~8.9s. It turns out memcpy is
pretty fast!

BUG=none
==========

[commit-bot: I haz the power, 2016-04-20 21:36:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:26:42]

Description was changed from

==========
Register clipboard image blob in the browser process to copy data less.

The original code encodes the PNG and sends the buffer over to the
renderer. The renderer then registers a blob with that buffer, which
transfers all the data back to the browser. Then the data is copied back
over to the renderer when the page tries to read the image/png
DataTransferItem.

Instead of sending the data back and forth so many times, just register
the blob directly in the browser process.

On a 1366x768 test image, there's no measurable difference: it takes
~0.125s to execute DataTransferItem::getAsFile() for an image.

On a 4000x4000 test image, there's a small difference: the old version
takes ~9.1s and the new version takes ~8.9s. It turns out memcpy is
pretty fast!

BUG=none
==========

to

==========
Register clipboard image blob in the browser process to copy data less.

The original code encodes the PNG and sends the buffer over to the
renderer. The renderer then registers a blob with that buffer, which
transfers all the data back to the browser. Then the data is copied back
over to the renderer when the page tries to read the image/png
DataTransferItem.

Instead of sending the data back and forth so many times, just register
the blob directly in the browser process.

On a 1366x768 test image, there's no measurable difference: it takes
~0.125s to execute DataTransferItem::getAsFile() for an image.

On a 4000x4000 test image, there's a small difference: the old version
takes ~9.1s and the new version takes ~8.9s. It turns out memcpy is
pretty fast!

BUG=none

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

[commit-bot: I haz the power, 2016-04-22 19:26:44]

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