Bug 28942

Summary:

[V8] DOM Storage bindings: Event Handler should create StorageEvents

Product:

WebKit

Reporter:

Ben Murdoch <benm>

Component:

WebCore Misc.

Assignee:

Ben Murdoch <benm>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, jorlow

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Proposed patch	eric: review-
Rejigged Changelog	none

Ben Murdoch

Reported 2009-09-03 03:46:07 PDT

On Android we're failing a set of DOM storage layout tests when running with V8. It seems that the event handling code in V8DOMWrapper.cpp does not generate Storage Events and the code in the custom storage binding (V8StorageCustom.cpp) ignores the length attribute in the setter but not in the getter.

Attachments
Proposed patch (2.07 KB, patch) 2009-09-03 04:53 PDT, Ben Murdoch	eric: review-	Details Formatted Diff Diff
Rejigged Changelog (2.97 KB, patch) 2009-09-04 02:55 PDT, Ben Murdoch	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Ben Murdoch

Comment 1 2009-09-03 04:53:32 PDT

Created attachment 38981 [details] Proposed patch

Jeremy Orlow

Comment 2 2009-09-03 07:44:26 PDT

In what cases do we do the wrong thing for length? IIRC, I tested these scenarios pretty carefully and found that it wasn't necessary, but that was a while ago. If there are any, could you please add a layout test to verify the behavior? If not, I think we should leave it as is.

Jeremy Orlow

Comment 3 2009-09-03 08:09:51 PDT

Nm. Apparently we're failing one that we're excluding right now for chrome and this fixes it. :-)

Ben Murdoch

Comment 4 2009-09-03 08:18:13 PDT

(In reply to comment #3) > Nm. Apparently we're failing one that we're excluding right now for chrome and > this fixes it. :-) Yes -- just to follow up it's the storage/domstorage/localstorage/complex-keys.html test I was seeing fail. The problem is that in the binding for name/indexed property setters, we silently ignore cases when trying to set the length property but we do create a new property named 'length' if it is set through the setItem function. Now in that layout test after calling setItem("length", 0) on line 92, we query the length through a named and indexed access and as we do not special case length in the indexed/named getter of the bindings we end up returning the value 0 (as in the property we added through setItem), rather than actual number of items in the object as the layout test result expects.

Eric Seidel (no email)

Comment 5 2009-09-04 00:23:58 PDT

Comment on attachment 38981 [details] Proposed patch The ChnageLog should list the tests that this changes/causes v8 to pass. If there are no tests, then this change needs tests. :(

Ben Murdoch

Comment 6 2009-09-04 02:55:35 PDT

Created attachment 39048 [details] Rejigged Changelog Added tests that now pass to the changelog.

Eric Seidel (no email)

Comment 7 2009-09-04 03:15:34 PDT

Comment on attachment 39048 [details] Rejigged Changelog LGTM. I can't remember if you're a committer, assuming not, cq+.

Ben Murdoch

Comment 8 2009-09-04 03:17:54 PDT

(In reply to comment #7) > (From update of attachment 39048 [details]) > LGTM. I can't remember if you're a committer, assuming not, cq+. Thanks - I am a committer, should be fine to let the cq handle this one though.

Eric Seidel (no email)

Comment 9 2009-09-04 03:28:40 PDT

Comment on attachment 39048 [details] Rejigged Changelog Clearing flags on attachment: 39048 Committed r48047: <http://trac.webkit.org/changeset/48047>

Eric Seidel (no email)

Comment 10 2009-09-04 03:28:44 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.