Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 8858858
or point your web browser to
http://mondrian/8858858
to review the following code:
Change 8858858 by [EMAIL PROTECTED] on 2008/11/04 19:54:03 *pending*
Allows users of CabUpdater to specify the GUID used in the request
query parameters.
R=andreip
[EMAIL PROTECTED]
DELTA=42 (14 added, 6 deleted, 22 changed)
OCL=8858858
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/ie/bho.cc#12 edit
...
//depot/googleclient/gears/opensource/gears/installer/common/cab_updater.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/cab_updater.h#1
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.cc#2
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.h#3
edit
42 delta lines: 14 added, 6 deleted, 22 changed
Also consider running:
g4 lint -c 8858858
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 8858858 by [EMAIL PROTECTED] on 2008/11/04 19:54:03 *pending*
Allows users of CabUpdater to specify the GUID used in the request
query parameters.
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/ie/bho.cc#12 edit
...
//depot/googleclient/gears/opensource/gears/installer/common/cab_updater.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/cab_updater.h#1
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.cc#2
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.h#3
edit
==== //depot/googleclient/gears/opensource/gears/base/ie/bho.cc#12 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/base/ie/bho.cc ====
# action=edit type=xtext
--- googleclient/gears/opensource/gears/base/ie/bho.cc 2008-11-05
12:50:45.000000000 +0000
+++ googleclient/gears/opensource/gears/base/ie/bho.cc 2008-11-05
12:08:23.000000000 +0000
@@ -36,6 +36,9 @@
#include "gears/installer/common/cab_updater.h"
#include "gears/localserver/ie/http_handler_ie.h"
+// TODO(steveblock): Fix this GUID. See bug 406.
+const char16* kGuid = L"%7Bc3fc95dBb-cd75-4f3d-a586-bcb7D004784c%7D";
+
HWND BrowserHelperObject::browser_window_ = NULL;
// static
@@ -62,7 +65,7 @@
site->get_HWND(reinterpret_cast<long*>(&browser_window_));
static CabUpdater updater;
- updater.Start(browser_window_);
+ updater.Start(browser_window_, kGuid);
}
return S_OK;
}
====
//depot/googleclient/gears/opensource/gears/installer/common/cab_updater.cc#1 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/installer/common/cab_updater.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/installer/common/cab_updater.cc
2008-11-05 12:50:45.000000000 +0000
+++ googleclient/gears/opensource/gears/installer/common/cab_updater.cc
2008-11-05 12:03:35.000000000 +0000
@@ -31,10 +31,6 @@
#include "gears/installer/common/installer_utils.h"
#include "gears/installer/common/resource.h"
-// TODO(andreip): When updating the Gears 'guid' field, consider switching
-// 'application' to a GUID that identifies the browser (to match Firefox
pings).
-// Also remind cprince to update the stats logic when 'guid' changes.
-const char16* kApplicationId = L"%7Bc3fc95dBb-cd75-4f3d-a586-bcb7D004784c%7D";
// The topic for the message system.
const char16* kTopic = STRING16(L"Cab Update Event");
@@ -65,12 +61,12 @@
}
}
-void CabUpdater::Start(HWND browser_window) {
+void CabUpdater::Start(HWND browser_window, std::string16 guid) {
browser_window_ = browser_window;
assert(browser_window_);
assert(checker_);
- if (checker_->Init(kFirstUpdatePeriod, kUpdatePeriod, kGracePeriod,
- kApplicationId, this)) {
+ if (checker_->Init(kFirstUpdatePeriod, kUpdatePeriod, kGracePeriod, guid,
+ this)) {
MessageService::GetInstance()->AddObserver(this, kTopic);
// We get stopped when the DLL unloads.
ThreadLocals::SetValue(kThreadLocalKey, this, &Stop);
====
//depot/googleclient/gears/opensource/gears/installer/common/cab_updater.h#1 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/installer/common/cab_updater.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/installer/common/cab_updater.h
2008-11-05 12:50:45.000000000 +0000
+++ googleclient/gears/opensource/gears/installer/common/cab_updater.h
2008-11-05 12:09:28.000000000 +0000
@@ -51,8 +51,9 @@
public:
CabUpdater();
virtual ~CabUpdater();
- // Starts the updater.
- void Start(HWND browser_window);
+
+ // Starts the updater. guid is the GUID used in the request query parameters.
+ void Start(HWND browser_window, std::string16 guid);
// MessageObserverInterface
====
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.cc#2
-
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-11-05 12:50:45.000000000 +0000
+++ googleclient/gears/opensource/gears/installer/common/periodic_checker.cc
2008-11-05 12:10:50.000000000 +0000
@@ -35,17 +35,15 @@
#include "genfiles/product_constants.h"
const char16* kUpgradeUrl = L"http://tools.google.com/service/update2/ff?";
- L"guid=%7Bc3fc95dBb-cd75-4f3d-a586-bcb7D004784c%7D"
- L"&version=" PRODUCT_VERSION_STRING
+ L"version=" PRODUCT_VERSION_STRING
L"&appversion=1.0"
L"&os=wince"
- L"&dist=google"
#ifdef OFFICIAL_BUILD
// Only pass 'dev=1' for non-official builds.
#else
L"&dev=1"
#endif
- L"&application=";
+ L"&dist=google";
// String constants for XML parsing
// Query language
@@ -74,7 +72,7 @@
class VersionFetchTask : public AsyncTask {
public:
// Factory method
- static VersionFetchTask *Create(const std::string16 &application_id,
+ static VersionFetchTask *Create(const std::string16 &guid,
HWND listener_window);
// Signals the worker thread to stop and asynchronously deletes the object.
@@ -87,7 +85,7 @@
private:
// Use Create to create a new instance and StopThreadAndDelete to destroy.
- VersionFetchTask(const std::string16 &application_id, HWND listener_window);
+ VersionFetchTask(const std::string16 &guid, HWND listener_window);
virtual ~VersionFetchTask() {}
// AsyncTask implementation
@@ -99,7 +97,7 @@
std::string16 url_;
std::string16 latest_version_;
- std::string16 application_id_;
+ std::string16 guid_;
HWND listener_window_;
Mutex is_processing_response_mutex_;
@@ -116,12 +114,12 @@
bool PeriodicChecker::Init(int32 first_period,
int32 normal_period,
int32 grace_period,
- const std::string16 &application_id,
+ const std::string16 &guid,
ListenerInterface *listener) {
first_period_ = first_period;
normal_period_ = normal_period;
grace_period_ = grace_period;
- application_id_ = application_id;
+ guid_ = guid;
listener_ = listener;
return (TRUE == stop_event_.Create(NULL, FALSE, FALSE, NULL)) &&
(TRUE == thread_complete_event_.Create(NULL, FALSE, FALSE, NULL));
@@ -241,7 +239,7 @@
// 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_);
+ task_ = VersionFetchTask::Create(guid_, window_);
}
handled = TRUE;
return TRUE;
@@ -301,10 +299,10 @@
// VersionFetchTask
// static
-VersionFetchTask *VersionFetchTask::Create(const std::string16 &application_id,
+VersionFetchTask *VersionFetchTask::Create(const std::string16 &guid,
HWND listener_window) {
VersionFetchTask *task =
- new VersionFetchTask(application_id, listener_window);
+ new VersionFetchTask(guid, listener_window);
if (!task) {
assert(false);
return NULL;
@@ -316,10 +314,10 @@
return task;
}
-VersionFetchTask::VersionFetchTask(const std::string16 &application_id,
+VersionFetchTask::VersionFetchTask(const std::string16 &guid,
HWND listener_window)
: AsyncTask(NULL),
- application_id_(application_id),
+ guid_(guid),
listener_window_(listener_window) {
assert(listener_window_);
}
@@ -343,7 +341,17 @@
void VersionFetchTask::Run() {
WebCacheDB::PayloadInfo payload;
scoped_refptr<BlobInterface> payload_data;
- std::string16 url = kUpgradeUrl + application_id_;
+
+ // We use the guid for both the guid and the application. Note that Omaha
does
+ // not use the application when forming its response.
+ //
+ // TODO(andreip): When updating the Gears 'guid' field, consider switching
+ // 'application' to a GUID that identifies the browser (to match Firefox
+ // pings). Also remind cprince to update the stats logic when 'guid' changes.
+ std::string16 url = kUpgradeUrl;
+ url += L"&guid=" + guid_;
+ url += L"&application=" + guid_;
+
bool result = false;
if (IsOnline()) {
result = HttpGet(url.c_str(),
====
//depot/googleclient/gears/opensource/gears/installer/common/periodic_checker.h#3
-
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-11-05 12:50:45.000000000 +0000
+++ googleclient/gears/opensource/gears/installer/common/periodic_checker.h
2008-11-05 12:06:24.000000000 +0000
@@ -53,7 +53,7 @@
// Also sets the application ID sent to the server for update checks and the
// listener for callbacks.
bool Init(int first_period, int normal_period, int grace_period,
- const std::string16 &application_id, ListenerInterface *listener);
+ const std::string16 &guid, ListenerInterface *listener);
// Starts the checker thread.
void Start();
// Stops the checker thread. Parmeter specifies whether this call should
block
@@ -100,8 +100,8 @@
int normal_period_;
// The time interval to wait after a connection up event.
int grace_period_;
- // The application ID to include in the server URL.
- std::string16 application_id_;
+ // The GUID to include in the request query parameters.
+ std::string16 guid_;
// The listener for callbacks
ListenerInterface *listener_;
// Events used for shutting down.
