======================================================================== http://mondrian.corp.google.com/file/9135600///depot/googleclient/gears/opensource/gears/base/npapi/module.cc?a=3 File //depot/googleclient/gears/opensource/gears/base/npapi/module.cc (snapshot 3) ------------------------------------ Line 54: #endif // BROWSER_OPERA Two spaces before comment ======================================================================== http://mondrian.corp.google.com/file/9135600///depot/googleclient/gears/opensource/gears/base/npapi/module.h?a=1 File //depot/googleclient/gears/opensource/gears/base/npapi/module.h (snapshot 1) ------------------------------------ Line 41: #ifdef BROWSER_OPERA Should this be protected with BROWSER_OPERA && OS_WINCE for future-proofing? ======================================================================== http://mondrian.corp.google.com/file/9135600///depot/googleclient/gears/opensource/gears/base/opera_mobile/opera_utils.cc?a=3 File //depot/googleclient/gears/opensource/gears/base/opera_mobile/opera_utils.cc (snapshot 3) ------------------------------------ Line 35: #include "third_party/opera_mobile/opera_callback_api.h" Within each block, includes should be in alphabetical order. ------------------------------------ Line 38: static ThreadId g_main_thread; Can these be static (private) members of OperaUtils? ------------------------------------ Line 105: scoped_refptr<GearsFactoryImpl> factory; Member variables have a trailing underscore. ------------------------------------ Line 134: SettingsDialog::Run(NULL); Is there any need for this method to be part of OperaUtils? ======================================================================== http://mondrian.corp.google.com/file/9135600///depot/googleclient/gears/opensource/gears/base/opera_mobile/opera_utils.h?a=1 File //depot/googleclient/gears/opensource/gears/base/opera_mobile/opera_utils.h (snapshot 1) ------------------------------------ Line 24: // ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. Can you add some general comments to this header and to those in third_party/opera_mobile about how the interfacing with the browser API works. ------------------------------------ Line 32: class OperaWorkerThread; I think we should be consistent in all of these class names and use 'Opera' rather than 'Op'. (Names of cross-browser classes such as OPHttpRequest should remain unchanged though). ------------------------------------ Line 39: static OpGearsAPI* GetGearsApi(); >From the perspective of Gears, 'GetBrowserApiForGears' or similar makes more sense. ------------------------------------ Line 40: static ThreadId GetMainThreadId(); Rename to 'GetBrowserThreadId' to make clear behaviour when called from a worker? ------------------------------------ Line 43: }; All classes should use DISALLOW_EVIL_CONSTRUCTORS; ======================================================================== http://mondrian.corp.google.com/file/9135600///depot/googleclient/gears/opensource/gears/base/opera_mobile/opera_worker.h?a=3 File //depot/googleclient/gears/opensource/gears/base/opera_mobile/opera_worker.h (snapshot 3) ------------------------------------ Line 33: class OperaWorkerThread { Pure interface classes such as this should be named XXXInterface. ======================================================================== http://mondrian.corp.google.com/file/9135600///depot/googleclient/gears/opensource/third_party/opera_mobile/opera_callback_api.h?a=5 File //depot/googleclient/gears/opensource/third_party/opera_mobile/opera_callback_api.h (snapshot 5) ------------------------------------ Line 12: typedef void (*OP_Initialize_func)(OpGearsAPI *, OperaCallbacks *); This type isn't used. ------------------------------------ Line 13: typedef OperaWorkerThread* (*CreateWorker_func)(); Types should be camel-caps, eg CreateWorkerFunction ------------------------------------ Line 17: OpLocalServerInterface **opera_local_server_instance); Can these be scoped to OperaCallbacks? ------------------------------------ Line 19: class OperaCallbacks { Use struct for a data-only type. ------------------------------------ Line 24: LocalServerInitialize_func LocalServerInitialize; Variables are named in lowercase, with words separated by underscores. ------------------------------------ Line 52: { No line break before '{'. ------------------------------------ Line 54: REDIRECT_CANCEL The Gears pattern is to name enumerated values after the enumerated type, eg REDIRECT_STATUS_OK ========================================================================
-- To respond, reply to this email or visit http://mondrian.corp.google.com/9135600
