IndexedDB: Fix prefetching; cache was always being bypassed.

IndexedDB: Fix prefetching; cache was always being bypassed.

Regression introduced in r119183 when "Null" and "Invalid" were disambiguated.

Tests will require refactoring; tracked as crbug.com/118033

BUG=117920
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126719

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127509

[jsbell, 2012-03-13 00:14:34]

Oops, my bad.

Thoughts on a test to ensure cache is getting used?

[michaeln, 2012-03-13 00:44:12]

lgtm

[michaeln, 2012-03-13 00:47:42]

unittests can work well for test at this grain size?

[jsbell, 2012-03-13 17:42:20]

On 2012/03/13 00:47:42, michaeln wrote:
> unittests can work well for test at this grain size?

New patch w/ unit tests added. It required sprinkling "is this a real cursor?"
checks inside the proxy; feedback welcome.

[michaeln, 2012-03-13 18:39:48]

http://codereview.chromium.org/9691021/diff/1002/content/common/indexed_db/pr...
File content/common/indexed_db/proxy_webidbcursor_impl_unittest.cc (right):

http://codereview.chromium.org/9691021/diff/1002/content/common/indexed_db/pr...
content/common/indexed_db/proxy_webidbcursor_impl_unittest.cc:24:
RendererWebIDBCursorImpl cursor(kInvalidIDBCursorId);
I was wondering how the kInvalidIDBCursorId value was used since the impl never
sets it's cursor_id_ field to that value. looks like its used so the impl avoids
calling the 'dispatcher' when conducting the PrefetchTest.

Hmmm... feels dirty to modify the impl class in this way. I think the class was
just not designed with unittests in mind. The dependency on the 'dispatcher'
makes it difficult to compose them. Not sure you should add this test in this
way at this time? I'd be ok with the fix w/o a test.

If RendererWebIDBCursor utilized an interface to perform the methods asked of
the 'dispatcher', that interface could be mocked out for unittests which focus
on the core logic of that class (which looks likes is pretty much the prefetch
stuff), and unittests could also verify that the core logic calls out via the
'dispatcher' as it should. All that sounds like a battle for another day to me
though.

[jsbell, 2012-03-13 18:47:33]

On 2012/03/13 18:39:48, michaeln wrote:
> Hmmm... feels dirty to modify the impl class in this way. I think the class
was
> just not designed with unittests in mind. The dependency on the 'dispatcher'
> makes it difficult to compose them. Not sure you should add this test in this
> way at this time? I'd be ok with the fix w/o a test.

Agreed that it's too messy, and we need to abstract out the dispatcher. I'll
roll back to the earlier patch (no test) and file a new crbug for better tests
here.

[commit-bot: I haz the power, 2012-03-13 19:00:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/9691021/6

[commit-bot: I haz the power, 2012-03-14 01:30:03]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android, revision is 126428, job name
was 9691021-6 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[michaeln, 2012-03-14 01:37:55]

On 2012/03/14 01:30:03, I haz the power (commit-bot) wrote:
> The commit queue went berserk

haha... nice automated message ;)

[commit-bot: I haz the power, 2012-03-14 18:32:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/9691021/6

[commit-bot: I haz the power, 2012-03-14 20:26:02]

Change committed as 126719

[jsbell, 2012-03-17 00:37:23]

The initial trivial fix was reverted because it exposed a memory leak. We must
have suppressed the leak at some point, then removed the suppression after the
regression was introduced.

I was able to verify the leak and the fix with valgrind. Review kthxbai?

[michaeln, 2012-03-17 02:44:46]

http://codereview.chromium.org/9691021/diff/13001/content/common/indexed_db/p...
File content/common/indexed_db/proxy_webidbcursor_impl.cc (right):

http://codereview.chromium.org/9691021/diff/13001/content/common/indexed_db/p...
content/common/indexed_db/proxy_webidbcursor_impl.cc:69:
scoped_ptr<WebIDBCallbacks> callbacks(callbacks_ptr);
lgtm

[commit-bot: I haz the power, 2012-03-19 16:08:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/9691021/13001

[commit-bot: I haz the power, 2012-03-19 18:36:47]

Change committed as 127509