The DOMWindow::*Storage functions currently return 0 in some cases. Currently, the v8 bindings will just wrap the NULL/0 with a Storage object. Given that 0 is returned when *Storage is "unavailable" it makes more sense to return Undefined. Pavel: Can you please look at the ScriptObjectQuarantine part, please?
Created attachment 32831 [details] v1 The DOMWindow::*Storage functions currently return 0 in some cases. Currently, the v8 bindings will just wrap the NULL/0 with a Storage object. Given that 0 is returned when *Storage is "unavailable" it makes more sense to return Undefined. Pavel: Can you please look at the ScriptObjectQuarantine part, please?
This change looks good to me, but I'd like Mads or Dimitri to OK it too.
In the ScriptObjectQuarantine part you need to make sure to enter the right context for creating the wrapper object. You have the frame and from there you can get the context that you have to enter before creating the wrapper. Just follow the pattern in the other methods. With that change, this looks good to me as well.
Actually, now I'm not so sure about the DOMWindow portion of this change. I ran 'alert(typeof window.localStorage)' and 'alert(window.localStorage)' in safari with DOMWindow::localStorage hard coded to return 0 and Safari returned 'Object' and 'null'. In other words, the way it is before this patch matches Safari. Still, if you do 'alert(typeof window.blahblahblah)' you get undefined in both Chromium and Safari. So it seems to me that if we want to pretend we know nothing about LocalStorage, we should be returning undefined. What do you guys think?
Created attachment 32837 [details] v2 Made the change Mads pointed out.
Created attachment 32878 [details] v2 minus DOMWindow changes This is the other possible option: Only change ScriptObjectQuarantine and leave DOMWindow as is. I'm leaning towards this patch, but both seem like fine options.
Comment on attachment 32878 [details] v2 minus DOMWindow changes I like this one better -- I was just going to suggest letting codegen do its thing.
Will the ScriptObjectQuarantine change work? You seem to be using node to get to the context, but you don't have a node parameter here. Instead, you have the actual frame. I think you can get the context by doing V8Proxy::context(frame).
Comment on attachment 32878 [details] v2 minus DOMWindow changes Whoops. Mads is right. Actually, you should just be able to use Frame* that's being passed.
Created attachment 32901 [details] rev 3 Made the change Mads suggested. Also a disclaimer that I should have made earlier: I know nothing about the Inspector, nothing about this code, and only some about the bindings in general. This stuff is definitely over my head. I'm only doing it because hitting the NOTIMPLEMENTED was blocking me from enabling DOM_STORAGE. The inspector has seemingly worked even after the previously buggy implementations, so please take a close look before you r+ it. That said, we really need to get this in before we can go much further in enabling DOM storage.
Comment on attachment 32901 [details] rev 3 ok.
Landed in r46074