Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(73)

Issue 3043037: Adds IDBKeyPath parser / extractor, and provides a mechanism to call it sandboxed. (Closed)

Created:
9 years, 11 months ago by bulach
Modified:
9 years, 1 month ago
CC:
chromium-reviews, darin-cc_chromium.org, ben+cc_chromium.org, Paweł Hajdan Jr., brettw-cc_chromium.org
Visibility:
Public.

Description

Adds IDBKeyPath parser / extractor, and provides a mechanism to call it sandboxed. TEST=idbkeypathextractor_browsertests.cc Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56524

Patch Set 1 : Patch #

Total comments: 6

Patch Set 2 : Adds comments. #

Total comments: 8

Patch Set 3 : Moves to IDBBindingUtilities. #

Total comments: 30

Patch Set 4 : Comments. #

Patch Set 5 : Tests with invalid keys, and keeping the utility process running. #

Total comments: 12

Patch Set 6 : Patch #

Total comments: 6

Patch Set 7 : Comments #

Patch Set 8 : Fixes batch mode in utility_thread. #

Total comments: 21

Patch Set 9 : Brett's comment and common param_traits. #

Patch Set 10 : WebKit roll, rebase and try. #

Patch Set 11 : Fixes dependencies. #

Total comments: 21

Patch Set 12 : Makes MSVC happy. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+644 lines, -107 lines) Patch
A chrome/browser/idbbindingutilities_browsertest.cc View 3 4 5 6 7 8 9 10 11 1 chunk +269 lines, -0 lines 0 comments Download
M chrome/browser/utility_process_host.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +36 lines, -1 line 0 comments Download
M chrome/browser/utility_process_host.cc View 1 2 3 4 5 6 7 8 3 chunks +33 lines, -1 line 0 comments Download
M chrome/chrome_common.gypi View 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
A chrome/common/indexed_db_param_traits.h View 1 chunk +102 lines, -0 lines 0 comments Download
M chrome/common/render_messages.h View 9 10 11 3 chunks +1 line, -17 lines 0 comments Download
M chrome/common/render_messages.cc View 9 2 chunks +1 line, -82 lines 0 comments Download
M chrome/common/render_messages_internal.h View 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/utility_messages.h View 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/utility_messages_internal.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +25 lines, -0 lines 0 comments Download
M chrome/utility/utility_thread.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +20 lines, -0 lines 0 comments Download
M chrome/utility/utility_thread.cc View 1 2 3 4 5 6 7 8 9 10 11 9 chunks +57 lines, -6 lines 0 comments Download
A webkit/glue/idb_bindings.h View 9 10 11 1 chunk +25 lines, -0 lines 0 comments Download
A webkit/glue/idb_bindings.cc View 9 10 11 1 chunk +69 lines, -0 lines 0 comments Download
M webkit/glue/webkit_glue.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 26 (0 generated)
bulach
This change depends on https://bugs.webkit.org/show_bug.cgi?id=42976 and https://bugs.webkit.org/show_bug.cgi?id=43276. The idea is that we'll run the IDBKeyPath ...
9 years, 11 months ago (2010-07-30 23:21:04 UTC) #1
Paweł Hajdan Jr.
The way of running methods on the IO thread seems suspicious to me, and it's ...
9 years, 11 months ago (2010-07-30 23:25:39 UTC) #2
bulach
Hi Pawel, thanks for your comments! I'm not sure I understood, could you please clarify ...
9 years, 11 months ago (2010-08-02 09:25:33 UTC) #3
andreip3000
http://codereview.chromium.org/3043037/diff/5001/6008 File webkit/glue/webidbkeypathextractor.cc (right): http://codereview.chromium.org/3043037/diff/5001/6008#newcode14 webkit/glue/webidbkeypathextractor.cc:14: class LocalContext { Isn't this the same as v8::Context::Scope? ...
9 years, 11 months ago (2010-08-02 10:18:01 UTC) #4
Paweł Hajdan Jr.
http://codereview.chromium.org/3043037/diff/5001/6001 File chrome/browser/idbkeypathextractor_browsertest.cc (right): http://codereview.chromium.org/3043037/diff/5001/6001#newcode39 chrome/browser/idbkeypathextractor_browsertest.cc:39: if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) { On 2010/08/02 09:25:34, bulach wrote: > ...
9 years, 11 months ago (2010-08-02 17:41:56 UTC) #5
bulach
andrei: thanks for the comments! all addressed, and also it's inline with the new names ...
9 years, 11 months ago (2010-08-03 10:28:13 UTC) #6
jorlow
http://codereview.chromium.org/3043037/diff/17001/4004 File chrome/browser/utility_process_host.h (right): http://codereview.chromium.org/3043037/diff/17001/4004#newcode80 chrome/browser/utility_process_host.h:80: int id, const std::vector<IDBKey>& value) {} Why are these ...
9 years, 11 months ago (2010-08-03 14:31:59 UTC) #7
bulach
thanks jeremy! comments addressed, would you please take another look before I send to a ...
9 years, 11 months ago (2010-08-05 21:26:26 UTC) #8
jorlow
besides what i mentioned in person, looking pretty good
9 years, 11 months ago (2010-08-06 14:42:09 UTC) #9
bulach
+avayvod, brettw all: this change depends on https://bugs.webkit.org/show_bug.cgi?id=42976 https://bugs.webkit.org/show_bug.cgi?id=43276 but I'd appreciate your thoughts on ...
9 years, 11 months ago (2010-08-10 16:41:09 UTC) #10
jorlow
LGTM. My only concern is that whomever is using this needs to handle the case ...
9 years, 11 months ago (2010-08-10 16:53:05 UTC) #11
whywhat
http://codereview.chromium.org/3043037/diff/35001/36003 File chrome/browser/utility_process_host.h (right): http://codereview.chromium.org/3043037/diff/35001/36003#newcode15 chrome/browser/utility_process_host.h:15: #include "chrome/common/serialized_script_value.h" This is not in alphabetical order, should ...
9 years, 11 months ago (2010-08-10 17:20:54 UTC) #12
bulach
thanks Anton! I addressed your comments, and changed things a bit to reflect the latest ...
9 years, 11 months ago (2010-08-11 16:56:46 UTC) #13
whywhat
LGTM with a couple of additional comments. I'd ask Brett to take a quick look ...
9 years, 11 months ago (2010-08-11 17:19:25 UTC) #14
bulach
thanks, Anton! brett, would you mind taking a look, please? as Anton said, this is ...
9 years, 11 months ago (2010-08-11 19:21:33 UTC) #15
bulach
hmm, I just realized a flaw in the utility_thread side: all the methods are "ReleaseProcess()" ...
9 years, 11 months ago (2010-08-12 19:12:31 UTC) #16
bulach
uploaded a new patch fixing the utility_thread. On 2010/08/12 19:12:31, bulach wrote: > hmm, I ...
9 years, 11 months ago (2010-08-12 20:18:43 UTC) #17
brettw
Sorry I took so long... http://codereview.chromium.org/3043037/diff/66001/67002 File chrome/browser/utility_process_host.cc (right): http://codereview.chromium.org/3043037/diff/66001/67002#newcode93 chrome/browser/utility_process_host.cc:93: if (has_started_) return true; ...
9 years, 11 months ago (2010-08-13 05:44:11 UTC) #18
bulach
thanks, Brett! all comments addressed except one about adding a reference in the child thread ...
9 years, 11 months ago (2010-08-13 15:06:30 UTC) #19
brettw
LGTM http://codereview.chromium.org/3043037/diff/66001/67007 File chrome/utility/utility_thread.cc (right): http://codereview.chromium.org/3043037/diff/66001/67007#newcode289 chrome/utility/utility_thread.cc:289: ChildProcess::current()->ReleaseProcess(); I get it. Then what you have ...
9 years, 11 months ago (2010-08-13 15:48:14 UTC) #20
bulach
(ouch, the trybots caught my DEPS violation..). brett, darin: your insight here will be greatly ...
9 years, 10 months ago (2010-08-17 15:04:57 UTC) #21
jorlow
On 2010/08/17 15:04:57, bulach wrote: > (ouch, the trybots caught my DEPS violation..). > > ...
9 years, 10 months ago (2010-08-17 15:09:08 UTC) #22
bulach
On 2010/08/17 15:09:08, jorlow wrote: > On 2010/08/17 15:04:57, bulach wrote: > > (ouch, the ...
9 years, 10 months ago (2010-08-17 16:33:15 UTC) #23
brettw
Just some style nits. The conversions and layering looks OK to me. In some places, ...
9 years, 10 months ago (2010-08-17 17:12:19 UTC) #24
jorlow
http://codereview.chromium.org/3043037/diff/39004/82012 File webkit/glue/idb_bindings.cc (right): http://codereview.chromium.org/3043037/diff/39004/82012#newcode49 webkit/glue/idb_bindings.cc:49: const string16& idb_key_path, std::vector<WebIDBKey>* values) { On 2010/08/17 17:12:19, ...
9 years, 10 months ago (2010-08-17 17:32:38 UTC) #25
bulach
9 years, 10 months ago (2010-08-17 21:30:22 UTC) #26
many thanks for the detailed review, Brett! I'll submit iff^H^H^H when the bots
are green. :)

jokes apart:
the latest patch failed on windows.. for some bizarre reason MSVC tries to pull
in some webkit dependency in npchrome_frame due to the param trait being in
common.. :(
I created a indexed_db_param_traits.h, and now I'm only including it in
render_messages and utility_messages, let see if all the bots are happy now...

http://codereview.chromium.org/3043037/diff/39004/82003
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/39004/82003#newcode167
chrome/browser/utility_process_host.h:167: // True iff running in batch mode,
i.e., StartBatchMode() has been called
On 2010/08/17 17:12:19, brettw wrote:
> Nit you don't have to follow: I hate it when people write "iff". The "only if"
> part really doesn't add any extra meaning in the context of a comment in
source
> code. "when" or "if" works just as well and is easier to read.

heh! :) I deleted an "iff" above so thought would balance by adding a new one..
;)
on a more serious note, done s/iff/when/

http://codereview.chromium.org/3043037/diff/39004/82009
File chrome/common/utility_messages_internal.h (right):

http://codereview.chromium.org/3043037/diff/39004/82009#newcode17
chrome/common/utility_messages_internal.h:17: #include
"chrome/common/serialized_script_value.h"
On 2010/08/17 17:12:19, brettw wrote:
> Is this include necessary? It looks like you can forward declare it in this
> header.

Done.

http://codereview.chromium.org/3043037/diff/39004/82009#newcode60
chrome/common/utility_messages_internal.h:60: // Tells the utility process that
it's running in batch mode.
On 2010/08/17 17:12:19, brettw wrote:
> Check indenting for these two messages.

Done.

http://codereview.chromium.org/3043037/diff/39004/82010
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/39004/82010#newcode264
chrome/utility/utility_thread.cc:264: namespace {
On 2010/08/17 17:12:19, brettw wrote:
> Normally we put the anonymous namespace at the beginning of the file. Also be
> sure that there are blank lines around the beginning & end of the namespace,
and
> the ending one looks like this "}  // namespace"

Done.

http://codereview.chromium.org/3043037/diff/39004/82010#newcode274
chrome/utility/utility_thread.cc:274: int id, const
std::vector<SerializedScriptValue>& serialized_script_values,
On 2010/08/17 17:12:19, brettw wrote:
> I'd put "serialized_script_values" on the next line as I explain in the arg
> wrapping comment below.

Done.

http://codereview.chromium.org/3043037/diff/39004/82012
File webkit/glue/idb_bindings.cc (right):

http://codereview.chromium.org/3043037/diff/39004/82012#newcode45
webkit/glue/idb_bindings.cc:45: } // namespace
On 2010/08/17 17:12:19, brettw wrote:
> Two spaces before end-of-line comments.

Done.

http://codereview.chromium.org/3043037/diff/39004/82012#newcode49
webkit/glue/idb_bindings.cc:49: const string16& idb_key_path,
std::vector<WebIDBKey>* values) {
On 2010/08/17 17:12:19, brettw wrote:
> Generally prefer to put args on separate lines unless everything fits on one
or
> there's some logical grouping (like "x" and "y"). So I'd put "values" on the
> next line. Same in the header.

Done.

http://codereview.chromium.org/3043037/diff/39004/82012#newcode55
webkit/glue/idb_bindings.cc:55: int pos = 0;
On 2010/08/17 17:12:19, brettw wrote:
> It doesn't look like this is needed for anything. Can you just delete it?

Done.

http://codereview.chromium.org/3043037/diff/39004/82012#newcode57
webkit/glue/idb_bindings.cc:57: serialized_script_values.begin();
On 2010/08/17 17:12:19, brettw wrote:
> I'd indent this 4 spaces from the ( (so 3 more).

Done.

http://codereview.chromium.org/3043037/diff/39004/82013
File webkit/glue/idb_bindings.h (right):

http://codereview.chromium.org/3043037/diff/39004/82013#newcode20
webkit/glue/idb_bindings.h:20: const
std::vector<WebKit::WebSerializedScriptValue>& serialized_script_values,
On 2010/08/17 17:12:19, brettw wrote:
> I'd wrap the var name indented 4 more spaces from the "const" on the previous
> line.

Done.

Powered by Google App Engine
This is Rietveld 408576698