Bug 27327 – [Chromium] Clean up v8 bindings a bit

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

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?

[Jeremy Orlow]

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?

[Darin Fisher (:fishd, Google)]

This change looks good to me, but I'd like Mads or Dimitri to OK it too.

[Mads Ager]

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.

[Jeremy Orlow]

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?

[Jeremy Orlow]

Created attachment 32837 [details]
v2

Made the change Mads pointed out.

[Jeremy Orlow]

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.

[Dimitri Glazkov (Google)]

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.

[Mads Ager]

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).

[Dimitri Glazkov (Google)]

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.

[Jeremy Orlow]

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.

[Dimitri Glazkov (Google)]

Comment on attachment 32901 [details]
rev 3

ok.

[Jeremy Orlow]

Landed in r46074