Bug 26356 - Need v8 bindings for DOM Storage
Summary: Need v8 bindings for DOM Storage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-12 14:35 PDT by Jeremy Orlow
Modified: 2009-06-15 21:00 PDT (History)
2 users (show)

See Also:


Attachments
v1 (6.54 KB, patch)
2009-06-12 14:48 PDT, Jeremy Orlow
dglazkov: review-
Details | Formatted Diff | Diff
v2 (5.95 KB, patch)
2009-06-12 15:17 PDT, Jeremy Orlow
dglazkov: review-
Details | Formatted Diff | Diff
v3 (5.95 KB, patch)
2009-06-12 15:24 PDT, Jeremy Orlow
levin: review+
Details | Formatted Diff | Diff
part 2 - v1 (1.55 KB, patch)
2009-06-15 20:47 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
part2 - v2 (1.54 KB, patch)
2009-06-15 20:51 PDT, Jeremy Orlow
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2009-06-12 14:35:25 PDT
Need v8 bindings for DOM Storage (for Chromium and Android).
Comment 1 Jeremy Orlow 2009-06-12 14:42:42 PDT
Chromium side being tracked at http://crbug.com/14006
Comment 2 Jeremy Orlow 2009-06-12 14:48:49 PDT
Created attachment 31214 [details]
v1

Create custom bindings for v8.  The rest of these files are still forked (so the review is happening on the chromium review site).  These bindings have been tested on a hacked up Chromium instance (also running --single-process) and Android.
Comment 3 Dimitri Glazkov (Google) 2009-06-12 15:01:44 PDT
Comment on attachment 31214 [details]
v1

> +2009-06-12  jorlow  <jorlow@chromium.org>

Need to set your environment variables: http://webkit.org/coding/contributing.html

> +        * bindings/v8/custom/V8StorageCustom.cpp: Added.

When you add a file, 

> +        (WebCore::V8Custom::v8StorageNamedPropertyEnumerator):
> +        (WebCore::storageGetter):
> +        (WebCore::INDEXED_PROPERTY_GETTER):
> +        (WebCore::NAMED_PROPERTY_GETTER):
> +        (WebCore::storageSetter):
> +        (WebCore::INDEXED_PROPERTY_SETTER):
> +        (WebCore::NAMED_PROPERTY_SETTER):
> +        (WebCore::storageDeleter):
> +        (WebCore::INDEXED_PROPERTY_DELETER):
> +        (WebCore::NAMED_PROPERTY_DELETER):

... there's no need to leave all the members listed in ChangeLog.

> +#include "config.h"
> +
> +#include "v8_binding.h"
V8Binding.h

> +#include "v8_custom.h"
V8CustomBinding.h

> +#include "V8Proxy.h"
> +
> +#include "Storage.h"

headers are out of order.

> +static v8::Handle<v8::Value> storageSetter(v8::Local<v8::String> nameV8, v8::Local<v8::Value> valueV8, const v8::AccessorInfo& info)

The convention is prefixes, rather than suffixes: v8Name, v8Value

> +
> +NAMED_PROPERTY_DELETER(Storage) {

Brace on new line.

> +    INC_STATS("DOM.Storage.NamedPropertyDeleter");
> +    return storageDeleter(name, info);
> +}
Comment 4 Jeremy Orlow 2009-06-12 15:17:35 PDT
Created attachment 31215 [details]
v2

Fixed Dimitri's issues.
Comment 5 Dimitri Glazkov (Google) 2009-06-12 15:20:32 PDT
Comment on attachment 31215 [details]
v2

> +    String name = ToWebCoreString(v8Name);
> +    String value = ToWebCoreString(v8Value);

toWebCoreString, sorry -- should've noticed earlier.

> +    String name = ToWebCoreString(v8Name);

ditto.
Comment 6 Jeremy Orlow 2009-06-12 15:24:09 PDT
Created attachment 31216 [details]
v3
Comment 7 David Levin 2009-06-12 15:46:20 PDT
Comment on attachment 31216 [details]
v3

         ASSERT(ec == 0);
should be (avoid ==, != comparisons to 0 in WK code).
         ASSERT(!ec);
but this can be done on landing.
Comment 8 David Levin 2009-06-12 15:48:13 PDT
Assigned to levin for landing.
Comment 9 Dmitry Titov 2009-06-12 16:21:53 PDT
Landed: http://trac.webkit.org/changeset/44636
Comment 10 Jeremy Orlow 2009-06-15 20:33:01 PDT
Things moved around a bit since I started this patch.  Need to upstream another file.
Comment 11 Jeremy Orlow 2009-06-15 20:47:06 PDT
Created attachment 31329 [details]
part 2 - v1

Define the functions in V8CustomBindings.h.  "Forgot" this in my earlier patch since it wasn't fully upstreamed when I wrote the patch.
Comment 12 Jeremy Orlow 2009-06-15 20:51:37 PDT
Created attachment 31330 [details]
part2 - v2

Forgot to set my name.  :-)
Comment 13 Dimitri Glazkov (Google) 2009-06-15 20:52:45 PDT
Comment on attachment 31330 [details]
part2 - v2

I'll land.
Comment 14 Dimitri Glazkov (Google) 2009-06-15 21:00:38 PDT
Fix landed as http://trac.webkit.org/changeset/44706.