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.
Created attachment 38981 [details] Proposed patch
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.
Nm. Apparently we're failing one that we're excluding right now for chrome and this fixes it. :-)
(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.
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. :(
Created attachment 39048 [details] Rejigged Changelog Added tests that now pass to the changelog.
Comment on attachment 39048 [details] Rejigged Changelog LGTM. I can't remember if you're a committer, assuming not, cq+.
(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.
Comment on attachment 39048 [details] Rejigged Changelog Clearing flags on attachment: 39048 Committed r48047: <http://trac.webkit.org/changeset/48047>
All reviewed patches have been landed. Closing bug.