Integrate the new DomStorage backend into DRT and test_shell.

Integrate the new dom_storage backend into chromium DRT and test_shell.
- test_shell_webkit_init is used in test_shell
- test_webview_delegate is used test_shell
- test_webkit_platform_support is used in crDRT
- webkit_support is used in crDRT (by way of WebViewHost.cpp upstream)

(I'll have to land a seperate patch to alter WebViewHost.cpp)

This is behind a compile time enable flag defined in dom_storage_types,
the flag is undefined in this CL. It will be enabled in a future CL
when chromium is also ready to switch over to the new backend across
the board.

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

[michaeln, 2012-02-23 05:19:19]

this isn't quite ready yet, but i think it's getting close

[benm (inactive), 2012-02-23 19:17:15]

Looking good - I guess that most of these comments are probably due to this
still being a work in progress...

http://codereview.chromium.org/9429029/diff/8001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_types.h (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_types.h:15: #define
ENABLE_NEW_DOM_STORAGE_BACKEND
Maybe a comment on this to say how the flag is used.

http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_p...
File webkit/support/test_webkit_platform_support.cc (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_p...
webkit/support/test_webkit_platform_support.cc:322:
TestWebKitPlatformSupport::createLocalStorageNamespace(
Just wondering, why does this class not define a method for getting
SessionStorage? I guess it picks it up from webview_delegate? But why the split?

http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_p...
File webkit/support/test_webkit_platform_support.h (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_p...
webkit/support/test_webkit_platform_support.h:134: SimpleDomStorageSystem
dom_storage_system_;
should this be guarded with the new #define?

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
File webkit/tools/test_shell/simple_dom_storage_system.cc (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:34: if (parent_.get())
!parent.get()

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:59: if (parent_.get())
!parent.get()

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:91: return new
AreaImpl(parent_, namespace_id_, GURL());
Should this be a GURL created from the origin parameter?

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:96: int new_id = -1;
how about making a kInvalidNamespaceId in dom_storage_types.h and using it here?

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:101: return new
NamespaceImpl(parent_, new_id);
Is it ok for -1 to end up as the id here?

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:165: context_(new
DomStorageContext(FilePath(), NULL, NULL)),
I guess the params here still need filling out.

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
File webkit/tools/test_shell/simple_dom_storage_system.h (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.h:25: class
SimpleDomStorageSystem {
Should this be named ...ForTesting?

[michaeln, 2012-02-25 19:20:31]

no new snapshot yet

http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_p...
File webkit/support/test_webkit_platform_support.cc (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_p...
webkit/support/test_webkit_platform_support.cc:322:
TestWebKitPlatformSupport::createLocalStorageNamespace(
On 2012/02/23 19:17:15, benm wrote:
> Just wondering, why does this class not define a method for getting
> SessionStorage? I guess it picks it up from webview_delegate? But why the
split?

Direct consequence of how webcore and ultimately the webkit API is put together,
this just implements that.

local storage namespaces are created w/o any 'context' whereas session storage
namespaces are contextual. The API reflects that by making the factory method
for local guys global and that for session guys an instance method of an
interface that implies the extra context.

http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_p...
File webkit/support/test_webkit_platform_support.h (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_p...
webkit/support/test_webkit_platform_support.h:134: SimpleDomStorageSystem
dom_storage_system_;
On 2012/02/23 19:17:15, benm wrote:
> should this be guarded with the new #define?

dont think its needed, we'll instance the class (its inexpensive to do so) and
just not use it for a brief period of time...

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
File webkit/tools/test_shell/simple_dom_storage_system.cc (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:165: context_(new
DomStorageContext(FilePath(), NULL, NULL)),
On 2012/02/23 19:17:15, benm wrote:
> I guess the params here still need filling out.

I think they may not need any more filling out than this (at least for now). We
can run test_shell/DRT without putting anything on disk (so much faster), and in
a completely single threaded mode (making the Simple class much simpler).

Some aspects of things won't get excersized in this test environment, but much
will. In particular the v8 bindings + webkit api + dom_storage logic minus task
posting and backing db stuff. This harness doesn't have to test "everything",
and actually can't since there's no IPC in the loop here. Other tests (unit,
automation, ui, etc) can cover parts that are missing coverage here.

When a renderer-side caching layer is added, definitely will want to revisit.

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
File webkit/tools/test_shell/simple_dom_storage_system.h (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.h:25: class
SimpleDomStorageSystem {
On 2012/02/23 19:17:15, benm wrote:
> Should this be named ...ForTesting?

There are several "Simple" classes named like this for the same purpose. It's
not crystal clear, but once you've seen one of them, when you run across
another, i hope thy set of them with similar names makes some sense... one of
the first of these was SimpleResourceLoaderBridge...

SimpleResourceLoaderBridge
SimpleSocketStreamBridge
SimpleClipboardImpl
SimpleWebCookieJarImpl
SimpleAppCacheSystem
SimpleFileWriter
SimpleFileSystem
SimpleDatabaseSystem

There are some other similar in nature classes called TestShellSomething or
TestWhatchamacallit (we're not completely consistent), but i think that naming
scheme is less clear since these classes aren't confined to just TestShell and
they aren't used for unit testing purposes (a common place for a TestFoo to be
used).

Since the 'storage' things use the Simple naming scheme (modulo the outlyer
TestShellWebBlobRegistryImpl), i think its better to be consistent with the
Simple naming convention.

[michaeln, 2012-02-28 01:14:54]

http://codereview.chromium.org/9429029/diff/8001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_types.h (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_types.h:15: #define
ENABLE_NEW_DOM_STORAGE_BACKEND
On 2012/02/23 19:17:15, benm wrote:
> Maybe a comment on this to say how the flag is used.

Done.

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
File webkit/tools/test_shell/simple_dom_storage_system.cc (right):

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:34: if (parent_.get())
On 2012/02/23 19:17:15, benm wrote:
> !parent.get()

Done.

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:59: if (parent_.get())
On 2012/02/23 19:17:15, benm wrote:
> !parent.get()

Done.

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:91: return new
AreaImpl(parent_, namespace_id_, GURL());
On 2012/02/23 19:17:15, benm wrote:
> Should this be a GURL created from the origin parameter?

Done.

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:96: int new_id = -1;
On 2012/02/23 19:17:15, benm wrote:
> how about making a kInvalidNamespaceId in dom_storage_types.h and using it
here?

Done in a constant in this file

http://codereview.chromium.org/9429029/diff/8001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_dom_storage_system.cc:101: return new
NamespaceImpl(parent_, new_id);
On 2012/02/23 19:17:15, benm wrote:
> Is it ok for -1 to end up as the id here?

Yes, should be harmless.

[michaeln, 2012-02-28 06:04:23]

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_webview_delegate.cc:335: WebStorageNamespace*
TestWebViewDelegate::createSessionStorageNamespace(
Oh well...

My assumption that this was utilized in chromiumDRT was wrong. this is only used
in test_shell. i'll have to create another code path for DRT to invoke the
SimplDomStorageSystem's CreateSessionStorageNamespace() factory method.

I missed the impl of WebViewClient::createSessionStorageNamespace(unsigned
quota) in this file... WebKit/Tools/DumpRenderTree/chromium/WebViewHost.cpp

I'll have to add something to webkit/support/webkit_support.h which is where DRT
calls out to invoke logic that's implemented in chromium.

Unfortunately, this'll be a two-sided patch to update WebViewHost.cpp in
webkit's svn repository.
- add an interface and impl to webkit_support (chromium svn)
- retrofit WebViewHost.cpp to use that new interface (webkit svn)

I can add that first part to this CL, but w/o having the upstream part done, i
can't run layout tests on the try bots (although i can run them on my local
system with WebViewHost changes made here). So long as its commited with the
ENABLE flag turned of, nothing will break.

[benm (inactive), 2012-02-28 16:46:01]

couple of nits but looking good

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_dom_storage_system.cc (right):

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:155: const WebURL& pageUrl,
Result& result, WebString& oldValue) {
should we set oldValue to a null string here to cover the cases when we bail out
early?

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:156: result =
ResultBlockedByQuota;
wdyt to setting to ResultOK here and setting to ResultBlockedByQuota below when
we fail the SetAreaItem? Bit easier to parse imo...

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:157: if (!Host())
Maybe we should have a different error code for when Host() is NULL?

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:180: return;
no need for a return

[michaeln, 2012-02-28 23:20:40]

Sweet, layout tests pass with this plugged in :) I'll upload a new snap shot
soon.

There's one expectation mismatch (not a test failure) around 'quota' stuff.

Apparently, webkit/safari has no limit on the amount of data that can be stuffed
into sessionStorage. This is not true in chrome (nor has it ever been). The new
backend imposes a limit (as it should for chrome). The thing is, the chromiumDRT
currently does what webkit/safari does... so no limit.

Interestingly...

In 2010 test expectations that match chrome's actual behavior were in the tree.
http://trac.webkit.org/changeset/59457

A couple of weeks ago, those expectations got deleted from the tree by a good
webkit doobie.
http://trac.webkit.org/changeset/107379

The newly missing expectation file caused a 'chrome' test that actually tries to
verify a limit is prpoerly imposed in chrome to fail, and get disabled.
http://code.google.com/p/chromium/issues/detail?id=113611.

Pretty funny really.

[michaeln, 2012-02-28 23:53:07]

Starting try jobs for Snapshot 8 which has the compile time flag disabled.

[michaeln, 2012-02-29 00:19:21]

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_dom_storage_system.cc (right):

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:155: const WebURL& pageUrl,
Result& result, WebString& oldValue) {
On 2012/02/28 16:46:01, benm wrote:
> should we set oldValue to a null string here to cover the cases when we bail
out
> early?

Done.

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:156: result =
ResultBlockedByQuota;
On 2012/02/28 16:46:01, benm wrote:
> wdyt to setting to ResultOK here and setting to ResultBlockedByQuota below
when
> we fail the SetAreaItem? Bit easier to parse imo...

I'd like to keep the setting of the two out args together (why would one be done
this way and the other another way). I can either preset them up top (as i'm
doing) so an early returns is just return; or set set them at each early return
site (w/o having preset up top).

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:157: if (!Host())
On 2012/02/28 16:46:01, benm wrote:
> Maybe we should have a different error code for when Host() is NULL?

Atm, there are only two possible values defined in the WebKit api: OK or
QUOTA_ERR. Agreed that a third error code would be a good addition to that api,
but i can't do that in this CL.

http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:180: return;
On 2012/02/28 16:46:01, benm wrote:
> no need for a return

Done.

[michaeln, 2012-02-29 01:43:28]

tony, ptal at the /webkit/support and /webkit/tools stuff

[tony, 2012-02-29 17:51:15]

webkit/support and webkit/tools LGTM

http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_dom_storage_system.cc (right):

http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:43: return
parent_.get()->context_.get();
Nit: Is the .get() after parent_ unnecessary?

http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:68: return
parent_.get()->host_.get();
Ditto.

http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:141: return
NullableString16(true);
Nit: You could probably just return a WebString() directly.

[michaeln, 2012-02-29 21:54:24]

fyi: here's the upstream patch to WebViewHost.cpp,
https://bugs.webkit.org/show_bug.cgi?id=79933

[benm (inactive), 2012-02-29 23:05:09]

lgtm!

http://codereview.chromium.org/9429029/diff/25009/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_types.h (right):

http://codereview.chromium.org/9429029/diff/25009/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_types.h:19: // but to not affect with the real
build until its ready.
nit: "affect the"

[michaeln, 2012-03-01 00:39:41]

http://codereview.chromium.org/9429029/diff/25009/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_types.h (right):

http://codereview.chromium.org/9429029/diff/25009/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_types.h:19: // but to not affect with the real
build until its ready.
On 2012/02/29 23:05:10, benm wrote:
> nit: "affect the"

Done.

http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_dom_storage_system.cc (right):

http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:43: return
parent_.get()->context_.get();
On 2012/02/29 17:51:15, tony wrote:
> Nit: Is the .get() after parent_ unnecessary?

Done.

http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:68: return
parent_.get()->host_.get();
On 2012/02/29 17:51:15, tony wrote:
> Ditto.

Done.

http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:141: return
NullableString16(true);
On 2012/02/29 17:51:15, tony wrote:
> Nit: You could probably just return a WebString() directly.

That's true, but i'm gunning for extra clarity to the reader that the returned
value should be understood as 'null'.