======================================================================== 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: On Thu Oct 2 02:48:33 2008 PDT, steveblock wrote: > Is the deleted line intentional?
Umm, yes, I though it's weird to have two line breaks between the string const and the constructor below. Or should there actually be two lines between them? ======================================================================== 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(). On Mon Oct 6 04:58:49 2008 PDT, steveblock wrote: > This comment needs updating. Done. ------------------------------------ Line 36: #include <webvw.h> On Thu Oct 2 02:49:08 2008 PDT, steveblock wrote: > Is this for IPIEHTMLWindow2? Yes. ======================================================================== 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. On Mon Oct 6 04:26:25 2008 PDT, steveblock wrote: > 2008 I like 2007, it was a good year :) Fixed. ------------------------------------ Line 3: // Redistribution and use in source and binary forms, with or without On Mon Oct 6 04:53:36 2008 PDT, steveblock wrote: > Trailing whitespace - and below in this comment block. Ah yes, copy&pasted from the ie-specific file...Fixed. ------------------------------------ Line 28: #ifdef WINCE On Mon Oct 6 04:27:12 2008 PDT, steveblock wrote: > Do we need these guards? We've used them everywhere else. Even if the header has the same guard, I'm not sure we want to have it included by all builds. The build is already slow as it is. ------------------------------------ Line 37: class HtmlEventMonitorHook On Mon Oct 6 04:56:39 2008 PDT, steveblock wrote: > 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. Done. ------------------------------------ Line 73: void *user_param) : function_(function), user_param_(user_param) { On Mon Oct 6 04:28:07 2008 PDT, steveblock wrote: > Weird line breaks. Done. ------------------------------------ Line 94: bool HtmlEventMonitor::Start(IPIEHTMLWindow2 *event_source) { On Mon Oct 6 04:57:38 2008 PDT, steveblock wrote: > All of this is specific to unload handling. Shouldn't you test event_name_? True. Done. ------------------------------------ Line 104: if FAILED(hr) { return true; } On Mon Oct 6 04:57:50 2008 PDT, steveblock wrote: > When would get_onunload fail? Shouln't we return false here? Currently the > method only returns true. It seems to fail randomly. We return true since we still have the "fake event" monitoring already set up. ======================================================================== 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 On Mon Oct 6 04:33:39 2008 PDT, steveblock wrote: > Is UnloadEventSourceInterface still used? Nope, fixed. ------------------------------------ Line 142: private: On Mon Oct 6 04:34:00 2008 PDT, steveblock wrote: > Trailing whitespace. Done. ------------------------------------ Line 143: static UnloadEventHandlerInterface *s_handler_; On Mon Oct 6 04:35:28 2008 PDT, steveblock wrote: > Variable naming. Umm, what's wrong with it? ------------------------------------ Line 145: UnloadEventSource(); On Mon Oct 6 04:34:43 2008 PDT, steveblock wrote: > Methods before member variables. Done. Note that I intentionally grouped it with DISALLOW_ since I wanted to make it clear to the reader that this class is not meant to be instantiated. ======================================================================== -- To respond, reply to this email or visit http://mondrian.corp.google.com/8450696
