The chromium-side backchannel plumbing for blobs in IDB.

The chromium-side backchannel plumbing for blobs in IDB.
This requires https://codereview.chromium.org/235933013/.

BUG=108012
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266677

[jsbell, 2014-04-17 18:37:59]

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
File content/child/indexed_db/indexed_db_dispatcher.cc (right):

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:523: static void
PrepareWebValueAndBlobInfo(
It seems like it would be nice to propagate the IndexedDBValue (bits+blobs)
pattern throughout Chromium/IPC/Blink API/WebCore to reduce the number of
parameters everywhere and provide more encapsulation. I suppose that could be
done later.

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:527:
blink::WebVector<WebBlobInfo>* web_blob_info) {
Parameter order here looks odd - web_value, value, blob_info, web_blob_info.

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:529: if (!value.empty()) {
Make this an early exit, to reduce indentation?

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:674:
DCHECK_EQ(p.values.size(), p.blob_or_file_infos.size());
Bleah - sending same-sized vectors through is an antipattern. We should really
define a struct and pass a vector of those through.

(I think I started on a patch for that at one point, but don't have it any
more.)

It's browser->renderer so it's not a security issue, at least.)

[ericu, 2014-04-17 21:56:01]

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
File content/child/indexed_db/indexed_db_dispatcher.cc (right):

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:523: static void
PrepareWebValueAndBlobInfo(
On 2014/04/17 18:37:59, jsbell wrote:
> It seems like it would be nice to propagate the IndexedDBValue (bits+blobs)
> pattern throughout Chromium/IPC/Blink API/WebCore to reduce the number of
> parameters everywhere and provide more encapsulation. I suppose that could be
> done later.

Agreed; will add a TODO to my notes.

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:527:
blink::WebVector<WebBlobInfo>* web_blob_info) {
On 2014/04/17 18:37:59, jsbell wrote:
> Parameter order here looks odd - web_value, value, blob_info, web_blob_info.

Huh.  Yeah, that's terrible.  Fixed to value, blob_info, *web_value,
*web_blob_info.

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:529: if (!value.empty()) {
On 2014/04/17 18:37:59, jsbell wrote:
> Make this an early exit, to reduce indentation?

Done.

https://codereview.chromium.org/240003010/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:674:
DCHECK_EQ(p.values.size(), p.blob_or_file_infos.size());
On 2014/04/17 18:37:59, jsbell wrote:
> Bleah - sending same-sized vectors through is an antipattern. We should really
> define a struct and pass a vector of those through.
> 
> (I think I started on a patch for that at one point, but don't have it any
> more.)
> 
> It's browser->renderer so it's not a security issue, at least.)

Added a TODO to indexed_db_messages.h for this message.

[jsbell, 2014-04-17 22:05:55]

lgtm

https://codereview.chromium.org/240003010/diff/20001/content/common/indexed_d...
File content/common/indexed_db/indexed_db_messages.h (right):

https://codereview.chromium.org/240003010/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_messages.h:274: // TODO(ericu): Put each
value/vector<blob_info> together in a wrapper object,
We'd want to tackle all 4 vectors, not just value/vector<blob_info>.

I found my old patch and it was using a Tuple3 but I thought a dedicated struct
was a better idea at that point and abandoned it.

(If you add a TODO you'll need to get tsepez to sign off or TBR. Maybe just file
a bug?)

[ericu, 2014-04-17 22:08:08]

https://codereview.chromium.org/240003010/diff/20001/content/common/indexed_d...
File content/common/indexed_db/indexed_db_messages.h (right):

https://codereview.chromium.org/240003010/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_messages.h:274: // TODO(ericu): Put each
value/vector<blob_info> together in a wrapper object,
On 2014/04/17 22:05:56, jsbell wrote:
> We'd want to tackle all 4 vectors, not just value/vector<blob_info>.

Oh right--of course.

> (If you add a TODO you'll need to get tsepez to sign off or TBR. Maybe just
file
> a bug?)

Will do.

[cmumford, 2014-04-17 22:26:52]

lgtm - with one question.

https://codereview.chromium.org/240003010/diff/40001/content/child/indexed_db...
File content/child/indexed_db/indexed_db_dispatcher.cc (right):

https://codereview.chromium.org/240003010/diff/40001/content/child/indexed_db...
content/child/indexed_db/indexed_db_dispatcher.cc:533:
blink::WebVector<WebBlobInfo> local_blob_info(blob_info.size());
Why create a local array and then swap at the end? Why not just construct
directly into web_blob_info?

[ericu, 2014-04-17 22:33:11]

https://codereview.chromium.org/240003010/diff/40001/content/child/indexed_db...
File content/child/indexed_db/indexed_db_dispatcher.cc (right):

https://codereview.chromium.org/240003010/diff/40001/content/child/indexed_db...
content/child/indexed_db/indexed_db_dispatcher.cc:533:
blink::WebVector<WebBlobInfo> local_blob_info(blob_info.size());
On 2014/04/17 22:26:53, cmumford wrote:
> Why create a local array and then swap at the end? Why not just construct
> directly into web_blob_info?

Outside of the constructor, you can't set the size of a WebVector without
overwriting the whole thing.  You can't even append to it.  It's a very simple
class, just used as a wrapper to pass things through the API boundary.

I could see if folks were OK with making WebVector just a little bit more
complicated [adding a resize to it], but this seemed simple enough, so I thought
I'd just use the local+swap.

[ericu, 2014-04-25 21:25:41]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:00:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/240003010/60001

[commit-bot: I haz the power, 2014-04-25 23:35:56]

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

[commit-bot: I haz the power, 2014-04-25 23:35:57]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[ericu, 2014-04-28 16:38:35]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-28 16:39:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/240003010/60001

[commit-bot: I haz the power, 2014-04-28 22:38:18]

Change committed as 266677