Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 8762257
or point your web browser to
http://mondrian/8762257
to review the following code:
Change 8762257 by [EMAIL PROTECTED] on 2008/10/28 15:06:53 *pending*
Makes periodic_checker browser-independent.
- Uses IsNetworkAlive in place of ActiveXUtils::IsOnline.
ActiveXUtils::IsOnline uses IsNetworkAlive as well as browser-specific code to
detect IE's offline mode on Win32. Since offline mode is not present in IE on
WinCE, ActiveXUtils::IsOnline offers no advantage over IsNetworkAlive.
- Uses a callback interface, rather than a windows message to get the
result of the VersionFetchTask.
R=andreip
[EMAIL PROTECTED]
DELTA=175 (89 added, 57 deleted, 29 changed)
OCL=8762257
Affected files ...
...
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.h#2
edit
175 delta lines: 89 added, 57 deleted, 29 changed
Also consider running:
g4 lint -c 8762257
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 8762257 by [EMAIL PROTECTED] on 2008/10/28 15:06:53 *pending*
Makes periodic_checker browser-independent.
- Uses IsNetworkAlive in place of ActiveXUtils::IsOnline.
ActiveXUtils::IsOnline uses IsNetworkAlive as well as browser-specific code to
detect IE's offline mode on Win32. Since offline mode is not present in IE on
WinCE, ActiveXUtils::IsOnline offers no advantage over IsNetworkAlive.
- Uses a callback interface, rather than a windows message to get the
result of the VersionFetchTask.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.h#2
edit
====
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.cc#1
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/installer/common/periodic_checker.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/installer/common/periodic_checker.cc
2008-10-31 14:02:16.000000000 +0000
+++ googleclient/gears/opensource/gears/installer/common/periodic_checker.cc
2008-10-31 13:40:18.000000000 +0000
@@ -25,11 +25,10 @@
#include "gears/installer/common/periodic_checker.h"
-#include "gears/base/ie/activex_utils.h"
+#include "gears/base/common/wince_compatibility.h" // For IsNetworkAlive
#include "gears/blob/blob_interface.h"
#include "gears/blob/blob_utils.h"
#include "gears/installer/common/resource.h"
-#include "gears/localserver/common/async_task.h"
#include "gears/localserver/common/http_constants.h"
#include "genfiles/product_constants.h"
@@ -63,43 +62,8 @@
const char16* kUpdateLinkQuery = STRING16(L"//em:updateLink[1]");
-// An implementation of AsyncTaks that makes a request to the upgrade URL and
-// parses the response to extract the latest version number and download URL.
-// Note that redirects are not followed.
-class VersionFetchTask : public AsyncTask {
- public:
- // Factory method
- static VersionFetchTask *Create(const std::string16 &application_id,
- HWND listener_window,
- int listener_message_base);
-
- // Signals the worker thread to stop and asynchronously deletes the object.
- // No further messages will be sent to the listener once a call to this
method
- // completes.
- void StopThreadAndDelete();
-
- const char16* Url() const;
- const char16* LatestVersion() const;
-
- private:
- // Use Create to create a new instance and StopThreadAndDelete to destroy.
- VersionFetchTask(const std::string16 &application_id);
- virtual ~VersionFetchTask() {}
-
- // AsyncTask implementation
- virtual void Run();
-
- // Parses XML and extracts the Gears version number and download URL.
- bool ExtractVersionAndDownloadUrl(const std::string16& xml);
-
- std::string16 url_;
- std::string16 latest_version_;
-
- std::string16 application_id_;
-
- DISALLOW_EVIL_CONSTRUCTORS(VersionFetchTask);
-};
-
+// Local function to determine if we're online
+static bool IsOnline();
// static
PeriodicChecker* PeriodicChecker::CreateChecker() {
@@ -223,10 +187,10 @@
thread_complete_event_.Set();
}
-LRESULT PeriodicChecker::OnTimer(UINT message,
- WPARAM w,
- LPARAM l,
- BOOL& handled) {
+LRESULT PeriodicChecker::OnTimer(UINT /* message */,
+ WPARAM /* unused1 */,
+ LPARAM /* unused2 */,
+ BOOL &handled) {
if (!normal_timer_event_) {
normal_timer_event_ = true;
ResetTimer(normal_period_);
@@ -234,18 +198,16 @@
// Depending on how the periodic checker is configured, this can fire
// while a task is running. We ignore such events.
if (!task_) {
- task_ = VersionFetchTask::Create(application_id_,
- window_,
- kUpdateTaskMessageBase);
+ task_ = VersionFetchTask::Create(application_id_, this);
}
handled = TRUE;
return TRUE;
}
-LRESULT PeriodicChecker::OnComplete(UINT message,
- WPARAM success,
- LPARAM task,
- BOOL& handled) {
+LRESULT PeriodicChecker::OnFetchTaskComplete(UINT /* message */,
+ WPARAM success,
+ LPARAM /* unused */,
+ BOOL &handled) {
assert(task_);
if (success) {
// We check for differences between the version strings. If there is a
@@ -261,9 +223,7 @@
// If it appears that we don't have a connection, wait for one to appear.
// Otherwise, the error must have been on the network, so we'll try again
// when the timer next expires.
- // TODO(steveblock): Use a browser-independent method to get connection
- // state.
- if (!ActiveXUtils::IsOnline()) {
+ if (!IsOnline()) {
ConnMgrRegisterForStatusChangeNotification(TRUE, window_);
}
}
@@ -276,11 +236,17 @@
return TRUE;
}
+// VersionFetchTask::TaskListenerInterface implementation.
+void PeriodicChecker::ResponseAvailable(bool success, VersionFetchTask *task) {
+ assert(task == task_);
+ // Marshall to the window message loop.
+ ::PostMessage(window_, WM_FETCH_TASK_COMPLETE, success, NULL);
+}
+
void PeriodicChecker::OnConnectionStatusChanged() {
// Unregister from connection events. We'll register again
// if the update check fails.
- // TODO(steveblock): Use a browser-independent method to get connection
state.
- if (ActiveXUtils::IsOnline()) {
+ if (IsOnline()) {
ConnMgrRegisterForStatusChangeNotification(FALSE, window_);
// Schedule a check after the grace period.
normal_timer_event_ = false;
@@ -299,12 +265,9 @@
// VersionFetchTask
// static
-VersionFetchTask *VersionFetchTask::Create(
- const std::string16 &application_id,
- HWND listener_window,
- int listener_message_base) {
- VersionFetchTask *task = new VersionFetchTask(application_id);
- task->SetListenerWindow(listener_window, listener_message_base);
+VersionFetchTask *VersionFetchTask::Create(const std::string16 &application_id,
+ TaskListenerInterface *listener) {
+ VersionFetchTask *task = new VersionFetchTask(application_id, listener);
if (!task) {
assert(false);
return NULL;
@@ -316,14 +279,18 @@
return task;
}
-VersionFetchTask::VersionFetchTask(const std::string16 &application_id)
+VersionFetchTask::VersionFetchTask(const std::string16 &application_id,
+ TaskListenerInterface *listener)
: AsyncTask(NULL),
- application_id_(application_id) {
+ application_id_(application_id),
+ listener_(listener) {
+ assert(listener_);
}
void VersionFetchTask::StopThreadAndDelete() {
- SetListenerWindow(NULL, 0);
+ is_processing_response_mutex_.Lock();
Abort();
+ is_processing_response_mutex_.Unlock();
DeleteWhenDone();
}
@@ -339,20 +306,29 @@
void VersionFetchTask::Run() {
WebCacheDB::PayloadInfo payload;
scoped_refptr<BlobInterface> payload_data;
+ std::string16 url = kUpgradeUrl + application_id_;
+ bool result = false;
+ if (IsOnline()) {
+ result = HttpGet(url.c_str(),
+ true,
+ NULL,
+ NULL,
+ NULL,
+ &payload,
+ &payload_data,
+ NULL, // was_redirected
+ NULL, // full_redirect_url
+ NULL); // error_msg
+ }
+
+ MutexLock lock(&is_processing_response_mutex_);
+ // is_aborted_ may be true even if HttpPost succeeded.
+ if (is_aborted_) {
+ return;
+ }
+
bool success = false;
- std::string16 url = kUpgradeUrl + application_id_;
- // TODO(steveblock): Use a browser-independent method to get connection
state.
- if (ActiveXUtils::IsOnline() &&
- AsyncTask::HttpGet(url.c_str(),
- true,
- NULL,
- NULL,
- NULL,
- &payload,
- &payload_data,
- NULL, // was_redirected
- NULL, // full_redirect_url
- NULL)) { // error_msg
+ if (result) {
std::string16 xml;
const std::string16 charset;
// payload.data can be empty in case of a 30x response.
@@ -368,7 +344,7 @@
success = true;
}
}
- NotifyListener(PeriodicChecker::kVersionFetchTaskMessageId, success);
+ listener_->ResponseAvailable(success, this);
}
// Internal
@@ -443,3 +419,9 @@
url_ = update_link;
return true;
}
+
+// static
+bool IsOnline() {
+ DWORD network_alive_flags_out = 0;
+ return IsNetworkAlive(&network_alive_flags_out) == TRUE;
+}
====
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.h#2
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/installer/common/periodic_checker.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/installer/common/periodic_checker.h
2008-10-31 14:02:16.000000000 +0000
+++ googleclient/gears/opensource/gears/installer/common/periodic_checker.h
2008-10-31 13:40:09.000000000 +0000
@@ -34,13 +34,60 @@
#define GEARS_INSTALLER_IEMOBILE_PERIODIC_CHECKER_H__
#include "gears/base/common/atl_headers_win32.h"
+#include "gears/base/common/mutex.h"
#include <gears/base/common/string16.h>
#include "gears/base/common/wince_compatibility.h"
+#include "gears/localserver/common/async_task.h"
-// An implemnentation detail of PeriodicChecker.
-class VersionFetchTask;
+// An implementation of AsyncTaks that makes a request to the upgrade URL and
+// parses the response to extract the latest version number and download URL.
+// Note that redirects are not followed.
+class VersionFetchTask : public AsyncTask {
+ public:
+ class TaskListenerInterface {
+ public:
+ virtual void ResponseAvailable(bool success, VersionFetchTask *task) = 0;
+ };
-class PeriodicChecker : public CWindowImpl<PeriodicChecker> {
+ // Factory method
+ static VersionFetchTask *Create(const std::string16 &application_id,
+ TaskListenerInterface *listener);
+
+ // Signals the worker thread to stop and asynchronously deletes the object.
+ // No further messages will be sent to the listener once a call to this
method
+ // completes.
+ void StopThreadAndDelete();
+
+ const char16* Url() const;
+ const char16* LatestVersion() const;
+
+ private:
+ // Use Create to create a new instance and StopThreadAndDelete to destroy.
+ VersionFetchTask(const std::string16 &application_id,
+ TaskListenerInterface *listener);
+ virtual ~VersionFetchTask() {}
+
+ // AsyncTask implementation
+ virtual void Run();
+
+ // Parses XML and extracts the Gears version number and download URL.
+ bool ExtractVersionAndDownloadUrl(const std::string16& xml);
+
+ std::string16 url_;
+ std::string16 latest_version_;
+
+ std::string16 application_id_;
+ TaskListenerInterface *listener_;
+
+ Mutex is_processing_response_mutex_;
+
+ DISALLOW_EVIL_CONSTRUCTORS(VersionFetchTask);
+};
+
+
+class PeriodicChecker
+ : public CWindowImpl<PeriodicChecker>,
+ public VersionFetchTask::TaskListenerInterface {
public:
class ListenerInterface {
public:
@@ -67,22 +114,25 @@
private:
// TODO(andreip): aggregate all Gears WM_USER messages in the same
// header to avoid overlap.
- static const int kUpdateTaskMessageBase = WM_USER;
static const int WM_FETCH_TASK_COMPLETE =
- kUpdateTaskMessageBase + kVersionFetchTaskMessageId;
+ WM_USER + kVersionFetchTaskMessageId;
// CWindowImpl
BEGIN_MSG_MAP(PeriodicChecker)
MESSAGE_HANDLER(WM_TIMER, OnTimer)
- MESSAGE_HANDLER(WM_FETCH_TASK_COMPLETE, OnComplete)
+ MESSAGE_HANDLER(WM_FETCH_TASK_COMPLETE, OnFetchTaskComplete)
END_MSG_MAP()
// Internal
PeriodicChecker();
// Message handlers.
- LRESULT OnTimer(UINT message, WPARAM w, LPARAM l, BOOL& handled);
- LRESULT OnComplete(UINT message, WPARAM success, LPARAM task, BOOL& handled);
+ LRESULT OnTimer(UINT message, WPARAM unused1, LPARAM unused2, BOOL &handled);
+ LRESULT OnFetchTaskComplete(UINT message, WPARAM success, LPARAM unused,
+ BOOL &handled);
+
+ // VersionFetchTask::TaskListenerInterface implementation.
+ virtual void ResponseAvailable(bool success, VersionFetchTask *task);
// This message handler is different because it needs to be registered at
// runtime with RegisterWindowMessage (it's comming from the connection
