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

Issue 9429029: Integrate the new DomStorage backend into DRT and test_shell. (Closed)

Created:
8 years, 8 months ago by michaeln
Modified:
8 years, 8 months ago
Reviewers:
tony, benm (inactive)
CC:
chromium-reviews, darin-cc_chromium.org, pam+watch_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 20

Patch Set 5 : #

Total comments: 9

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Total comments: 8

Patch Set 10 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+330 lines, -11 lines) Patch
M webkit/dom_storage/dom_storage_context.h View 1 2 3 4 5 6 7 8 9 2 chunks +7 lines, -8 lines 0 comments Download
M webkit/dom_storage/dom_storage_host.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M webkit/dom_storage/dom_storage_session.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M webkit/dom_storage/dom_storage_types.h View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -0 lines 0 comments Download
M webkit/support/test_webkit_platform_support.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download
M webkit/support/test_webkit_platform_support.cc View 1 2 3 4 5 6 7 8 9 2 chunks +9 lines, -1 line 0 comments Download
M webkit/support/webkit_support.h View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -0 lines 0 comments Download
M webkit/support/webkit_support.cc View 1 2 3 4 5 6 7 8 9 3 chunks +11 lines, -0 lines 0 comments Download
M webkit/support/webkit_support.gypi View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -0 lines 0 comments Download
A webkit/tools/test_shell/simple_dom_storage_system.h View 1 2 3 4 5 1 chunk +56 lines, -0 lines 0 comments Download
A webkit/tools/test_shell/simple_dom_storage_system.cc View 1 2 3 4 5 6 7 8 9 1 chunk +213 lines, -0 lines 0 comments Download
M webkit/tools/test_shell/test_shell_webkit_init.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download
M webkit/tools/test_shell/test_shell_webkit_init.cc View 1 2 3 4 5 6 7 8 9 2 chunks +9 lines, -1 line 0 comments Download
M webkit/tools/test_shell/test_webview_delegate.cc View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
michaeln
this isn't quite ready yet, but i think it's getting close
8 years, 8 months ago (2012-02-23 05:19:19 UTC) #1
benm (inactive)
Looking good - I guess that most of these comments are probably due to this ...
8 years, 8 months ago (2012-02-23 19:17:15 UTC) #2
michaeln
no new snapshot yet http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_platform_support.cc File webkit/support/test_webkit_platform_support.cc (right): http://codereview.chromium.org/9429029/diff/8001/webkit/support/test_webkit_platform_support.cc#newcode322 webkit/support/test_webkit_platform_support.cc:322: TestWebKitPlatformSupport::createLocalStorageNamespace( On 2012/02/23 19:17:15, benm ...
8 years, 8 months ago (2012-02-25 19:20:31 UTC) #3
michaeln
http://codereview.chromium.org/9429029/diff/8001/webkit/dom_storage/dom_storage_types.h File webkit/dom_storage/dom_storage_types.h (right): http://codereview.chromium.org/9429029/diff/8001/webkit/dom_storage/dom_storage_types.h#newcode15 webkit/dom_storage/dom_storage_types.h:15: #define ENABLE_NEW_DOM_STORAGE_BACKEND On 2012/02/23 19:17:15, benm wrote: > Maybe ...
8 years, 8 months ago (2012-02-28 01:14:54 UTC) #4
michaeln
http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/test_webview_delegate.cc File webkit/tools/test_shell/test_webview_delegate.cc (right): http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/test_webview_delegate.cc#newcode335 webkit/tools/test_shell/test_webview_delegate.cc:335: WebStorageNamespace* TestWebViewDelegate::createSessionStorageNamespace( Oh well... My assumption that this was ...
8 years, 8 months ago (2012-02-28 06:04:23 UTC) #5
benm (inactive)
couple of nits but looking good http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/simple_dom_storage_system.cc File webkit/tools/test_shell/simple_dom_storage_system.cc (right): http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/simple_dom_storage_system.cc#newcode155 webkit/tools/test_shell/simple_dom_storage_system.cc:155: const WebURL& pageUrl, ...
8 years, 8 months ago (2012-02-28 16:46:01 UTC) #6
michaeln
Sweet, layout tests pass with this plugged in :) I'll upload a new snap shot ...
8 years, 8 months ago (2012-02-28 23:20:40 UTC) #7
michaeln
Starting try jobs for Snapshot 8 which has the compile time flag disabled.
8 years, 8 months ago (2012-02-28 23:53:07 UTC) #8
michaeln
http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/simple_dom_storage_system.cc File webkit/tools/test_shell/simple_dom_storage_system.cc (right): http://codereview.chromium.org/9429029/diff/17001/webkit/tools/test_shell/simple_dom_storage_system.cc#newcode155 webkit/tools/test_shell/simple_dom_storage_system.cc:155: const WebURL& pageUrl, Result& result, WebString& oldValue) { On ...
8 years, 8 months ago (2012-02-29 00:19:21 UTC) #9
michaeln
tony, ptal at the /webkit/support and /webkit/tools stuff
8 years, 8 months ago (2012-02-29 01:43:28 UTC) #10
tony
webkit/support and webkit/tools LGTM http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/simple_dom_storage_system.cc File webkit/tools/test_shell/simple_dom_storage_system.cc (right): http://codereview.chromium.org/9429029/diff/25009/webkit/tools/test_shell/simple_dom_storage_system.cc#newcode43 webkit/tools/test_shell/simple_dom_storage_system.cc:43: return parent_.get()->context_.get(); Nit: Is the ...
8 years, 8 months ago (2012-02-29 17:51:15 UTC) #11
michaeln
fyi: here's the upstream patch to WebViewHost.cpp, https://bugs.webkit.org/show_bug.cgi?id=79933
8 years, 8 months ago (2012-02-29 21:54:24 UTC) #12
benm (inactive)
lgtm! http://codereview.chromium.org/9429029/diff/25009/webkit/dom_storage/dom_storage_types.h File webkit/dom_storage/dom_storage_types.h (right): http://codereview.chromium.org/9429029/diff/25009/webkit/dom_storage/dom_storage_types.h#newcode19 webkit/dom_storage/dom_storage_types.h:19: // but to not affect with the real ...
8 years, 8 months ago (2012-02-29 23:05:09 UTC) #13
michaeln
8 years, 8 months ago (2012-03-01 00:39:41 UTC) #14
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'.

Powered by Google App Engine
This is Rietveld 408576698