Hi,
On Mon, Dec 8, 2008 at 5:36 AM, Nigel Tao <[EMAIL PROTECTED]> wrote: > I'm generally happy with the approach you're taking. Like michaeln, this is a > lot nicer than I was expecting, and a better solution than what I would have > first tried to do myself. :) > > ======================================================================== > http://mondrian.corp.google.com/file/9284351///depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.cc?a=1 > File > //depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.cc > (snapshot 1) > ------------------------------------ > Line 554: // a completely bogus value. > On 6 Dec, michaeln wrote: >> The test on line 501 looks like its trying to accomplish the same thing, >> validate that a JSObject refers to a GearsNativeObject. Maybe we can reuse > that >> test at this site? > > Yeah, they are trying to accomplish the same thing. However, > JsContextWrapper::JsWrapperCaller is a static method and cannot access the > Gears-specific (as opposed to a Spidermonkey-specific) JsContextWrapper class > (and its shared_js_classes_ member variable). So I'm not sure if you could > take > the line-501 approach here. But you could change the line-501 code to take the > same approach as you've done here. That change might be better as a separate > ChangeList, though - perhaps add a TODO(nigeltao)? > > Yes, JsContextWrapper::JsWrapperCaller is static so it makes sense to change the line-501 code to take the same approach. I've added the TODO. > >> Also not I'm not sure what the distinction is between the >> JS_GET_CLASS macro and the JS_GetClass function? > > For Gears' purposes, the JS_GET_CLASS macro just expands to the JS_GetClass > function. The difference is if you compile Spidermonkey as single- or multi- > threaded (#ifdef JS_THREADSAFE). Gears does the latter. Changed to JS_GET_CLASS. > ======================================================================== > http://mondrian.corp.google.com/file/9284351///depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.h?a=1 > File > //depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.h > (snapshot 1) > ------------------------------------ > Line 143: static std::set<std::string> gears_module_names_; > The module names should be known at compile time. It might be possible, then, > to > create (and populate) this set during the JsContextWrapper constructor, and > then > you might not need the Mutex. Perhaps this can be another TODO to investigate. It's true that the list of Gears module names is known at compile time. However, I suspect that providing a nice way to build up such a list at compile time would be a slightly larger change. Added a TODO. Thanks, Andrei
