New IPC message parameters for IDB/Blob support.

New IPC message parameters for IDB/Blob support.

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

[jsbell, 2014-04-08 00:03:38]

+ tsepez@ for IPC messages review

(I haven't had a chance to look at this yet, but tsepez will need to weigh in
eventually)

[Tom Sepez, 2014-04-08 17:35:20]

Can you tell me how file_path and file_name are going to be used?  Thanks.

[ericu, 2014-04-08 20:36:57]

On 2014/04/08 17:35:20, Tom Sepez wrote:
> Can you tell me how file_path and file_name are going to be used?  Thanks.

Sure; the master CLs that will use these message are linked below.

On the browser->renderer messages, they're used to create a File object that
points into the IDB directory where the data for said File is stored.  That
happens in SerializedScriptValue.cpp, in readFileIndexHelper, in
https://codereview.chromium.org/18590006/.  That should be pretty innocuous,
since we already expose such paths to the renderer in every other File object.

The renderer->browser message (IndexedDBHostMsg_DatabasePut_Params) is used to
pass a File to the browser in order to read it and store it in the database.  As
of 5 minutes ago [uh...oops], we don't trust the renderer's path, and validate
read permissions for the file in indexed_db_dispatcher_host.cc in
https://codereview.chromium.org/18023022/.

[Tom Sepez, 2014-04-08 21:16:18]

LGTM given the check in indexed_db_dispatcher_host.cc

[cmumford, 2014-04-08 21:47:14]

On 2014/04/08 21:16:18, Tom Sepez wrote:
> LGTM given the check in indexed_db_dispatcher_host.cc

lgtm with nits

[cmumford, 2014-04-08 21:47:26]

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

https://codereview.chromium.org/227693008/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:511: if (p.value.size())
Nit: was here before, but size() is O(n), but empty() is O(1)

https://codereview.chromium.org/227693008/diff/1/content/common/indexed_db/in...
File content/common/indexed_db/indexed_db_messages.h (right):

https://codereview.chromium.org/227693008/diff/1/content/common/indexed_db/in...
content/common/indexed_db/indexed_db_messages.h:18: #include "url/gurl.h"
Is gurl.h needed?

https://codereview.chromium.org/227693008/diff/1/content/common/indexed_db/in...
content/common/indexed_db/indexed_db_messages.h:293:
IPC_STRUCT_MEMBER(content::IndexedDBKey, primary_key)
The other messages only use primary_key when there is another key(s) member.
Might this be better named as "key"?

[ericu, 2014-04-08 22:07:10]

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

https://codereview.chromium.org/227693008/diff/1/content/child/indexed_db/ind...
content/child/indexed_db/indexed_db_dispatcher.cc:511: if (p.value.size())
On 2014/04/08 21:47:26, cmumford wrote:
> Nit: was here before, but size() is O(n), but empty() is O(1)

As discussed in person, size() is likely O(1), but empty() better anyway.

https://codereview.chromium.org/227693008/diff/1/content/common/indexed_db/in...
File content/common/indexed_db/indexed_db_messages.h (right):

https://codereview.chromium.org/227693008/diff/1/content/common/indexed_db/in...
content/common/indexed_db/indexed_db_messages.h:18: #include "url/gurl.h"
On 2014/04/08 21:47:26, cmumford wrote:
> Is gurl.h needed?

No, that's old.  Removed.

https://codereview.chromium.org/227693008/diff/1/content/common/indexed_db/in...
content/common/indexed_db/indexed_db_messages.h:293:
IPC_STRUCT_MEMBER(content::IndexedDBKey, primary_key)
On 2014/04/08 21:47:26, cmumford wrote:
> The other messages only use primary_key when there is another key(s) member.
> Might this be better named as "key"?

I used primary_key as that was the name of the parameter that got rolled in [see
OnSuccessValueWithKey].  I don't see an example of a parameter object for an
onsuccess method that uses key and has no primary_key, so this seems consistent
to me too.

The closest I see to a counterexample is key vs. index_key in
IndexedDBHostMsg_DatabasePut_Params, but then
IndexedDBHostMsg_DatabaseSetIndexKeys_Params uses primary_key vs. index_keys, so
maybe that one should be fixed to be primary_key.

[jsbell, 2014-04-08 22:55:22]

lgtm (other than primary_key discussion...)

The key vs. primary_key convention comes from the spec, where 'key' is used
everywhere in a context sensitive way except for cursors where 'key' and
'primaryKey' are both exposed, and (implementation detail) indexing (where we
supply both to populate the index).

The SuccessWithKey case is also an impl detail; it's definitely a primary key,
since it's the case where the pk was autogenerated and is being sent back along
side the serialized value so it can be injected in the renderer.

So... I'm fine with it being primary_key or key. :)

[ericu, 2014-04-08 23:01:15]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-08 23:01:50]

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

[commit-bot: I haz the power, 2014-04-09 00:32:40]

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

[commit-bot: I haz the power, 2014-04-09 00:56:33]

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

[commit-bot: I haz the power, 2014-04-09 01:07:43]

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

[commit-bot: I haz the power, 2014-04-09 01:27:22]

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

[commit-bot: I haz the power, 2014-04-09 01:38:03]

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

[commit-bot: I haz the power, 2014-04-09 01:49:15]

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

[commit-bot: I haz the power, 2014-04-09 01:59:36]

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

[commit-bot: I haz the power, 2014-04-09 02:17:36]

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

[commit-bot: I haz the power, 2014-04-09 04:04:17]

Change committed as 262608