======================================================================== http://mondrian.corp.google.com/file/8450696///depot/googleclient/gears/opensource/gears/base/common/html_event_monitor.cc?a=1 File //depot/googleclient/gears/opensource/gears/base/common/html_event_monitor.cc (snapshot 1) ------------------------------------ Line 33: Is the deleted line intentional? ======================================================================== http://mondrian.corp.google.com/file/8450696///depot/googleclient/gears/opensource/gears/base/common/html_event_monitor.h?a=1 File //depot/googleclient/gears/opensource/gears/base/common/html_event_monitor.h (snapshot 1) ------------------------------------ Line 35: // attachEvent(). This comment needs updating. ------------------------------------ Line 36: #include <webvw.h> Is this for IPIEHTMLWindow2? ======================================================================== http://mondrian.corp.google.com/file/8450696///depot/googleclient/gears/opensource/gears/base/common/html_event_monitor_iemobile.cc?a=4 File //depot/googleclient/gears/opensource/gears/base/common/html_event_monitor_iemobile.cc (snapshot 4) ------------------------------------ Line 1: // Copyright 2007, Google Inc. 2008 ------------------------------------ Line 3: // Redistribution and use in source and binary forms, with or without Trailing whitespace - and below in this comment block. ------------------------------------ Line 28: #ifdef WINCE Do we need these guards? ------------------------------------ Line 37: class HtmlEventMonitorHook Add a comment explaining that on WinCE, HTMLEventMonitorHook not only passes events from the browser (in this case IPIEHTMLWindow2) to the HTMLEventMonitor, but it also handles additional fake unload events from the factory. ------------------------------------ Line 73: void *user_param) : function_(function), user_param_(user_param) { Weird line breaks. ------------------------------------ Line 94: bool HtmlEventMonitor::Start(IPIEHTMLWindow2 *event_source) { All of this is specific to unload handling. Shouldn't you test event_name_? ------------------------------------ Line 104: if FAILED(hr) { return true; } When would get_onunload fail? Shouln't we return false here? Currently the method only returns true. ======================================================================== http://mondrian.corp.google.com/file/8450696///depot/googleclient/gears/opensource/gears/base/common/wince_compatibility.h?a=5 File //depot/googleclient/gears/opensource/gears/base/common/wince_compatibility.h (snapshot 5) ------------------------------------ Line 126: // concrete implementation of UnloadEventSourceInterface in order to receive Is UnloadEventSourceInterface still used? ======================================================================== http://mondrian.corp.google.com/file/8450696///depot/googleclient/gears/opensource/gears/base/common/wince_compatibility.h?a=2 File //depot/googleclient/gears/opensource/gears/base/common/wince_compatibility.h (snapshot 2) ------------------------------------ Line 136: // with its PrivateSendUnloadEvent, can detect unload events. detect?! ======================================================================== http://mondrian.corp.google.com/file/8450696///depot/googleclient/gears/opensource/gears/base/common/wince_compatibility.h?a=5 File //depot/googleclient/gears/opensource/gears/base/common/wince_compatibility.h (snapshot 5) ------------------------------------ Line 142: private: Trailing whitespace. ------------------------------------ Line 143: static UnloadEventHandlerInterface *s_handler_; Variable naming. ------------------------------------ Line 145: UnloadEventSource(); Methods before member variables. ========================================================================
-- To respond, reply to this email or visit http://mondrian.corp.google.com/8450696
