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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Murdoch]

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.

[Ben Murdoch]

Created attachment 38981 [details]
Proposed patch

[Jeremy Orlow]

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]

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

[Ben Murdoch]

(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 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]

Created attachment 39048 [details]
Rejigged Changelog

Added tests that now pass to the changelog.

[Eric Seidel (no email)]

Comment on attachment 39048 [details]
Rejigged Changelog

LGTM.  I can't remember if you're a committer, assuming not, cq+.

[Ben Murdoch]

(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 on attachment 39048 [details]
Rejigged Changelog

Clearing flags on attachment: 39048

Committed r48047: <http://trac.webkit.org/changeset/48047>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.