I have only two significant comments. The others are just style nits. - This makes use of an overloaded version of NewJsRunner() that hasn't been submitted yet. I'm happy to submit with this call commented out and a TODO. - I'm not sure about casting from NPP to JsCallContext.
I'll follow up with an updated patch that fixes all of my comments. If you're happy with this, I'll submit on your behalf - there's no need to send a new patch. ======================================================================== http://mondrian.corp.google.com/file/9350686///depot/googleclient/gears/opensource/gears/base/opera/opera_utils.cc?a=1 File //depot/googleclient/gears/opensource/gears/base/opera/opera_utils.cc (snapshot 1) ------------------------------------ Line 31: #include "third_party/opera/opera_worker_interface.h" Includes in alphabetical order. ------------------------------------ Line 41: : public OperaWorkerThreadInterface { This fits on one line. ------------------------------------ Line 58: const unsigned short* url) { One argument per line if they don't all fit on one line. ------------------------------------ Line 60: js_runner_ = NewJsRunner(instance, global_object); This overload of NewJsRunner() has not yet been submitted. ------------------------------------ Line 69: BrowserUtils::GetPageBrowsingContext((NPP)instance, &browsing_context); Is the cast to NPP required? ------------------------------------ Line 78: if (!CreateModule<GearsFactoryImpl>(module_env, (JsCallContext*)instance, &factory_)) { return NULL; } Line too long. Is it safe to cast from NPP to JsCallContext*? As far as I'm aware, the types are unrelated. In any case, use reinterpret_cast to make this explicit. ------------------------------------ Line 83: NPObject* fo = NPVARIANT_TO_OBJECT(factory_.get()->GetWrapperToken()); Use a more explicit variable name ------------------------------------ Line 102: }; Use DISALLOW_EVIL_CONSTRUCTORS ======================================================================== http://mondrian.corp.google.com/file/9350686///depot/googleclient/gears/opensource/gears/base/opera/opera_utils.h?a=1 File //depot/googleclient/gears/opensource/gears/base/opera/opera_utils.h (snapshot 1) ------------------------------------ Line 46: static OperaGearsAPI* GetBrowserApiForGears(); Be consistent about placing * on right or left. Prefer right. ======================================================================== -- To respond, reply to this email or visit http://mondrian.corp.google.com/9350686
