************************* G4 reminder *************************
These new files:
c:\MyDocs\Gears1\googleclient\gears\opensource\gears\geolocation\access_token_manager.cc
are missing unit tests.
***************************************************************
Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 8513195
or point your web browser to
http://mondrian/8513195
to review the following code:
Change 8513195 by [EMAIL PROTECTED] on 2008/10/07 15:25:57 *pending*
Adds access token management for reverse geocoding.
R=andreip
[EMAIL PROTECTED]
DELTA=250 (163 added, 81 deleted, 6 changed)
OCL=8513195
Affected files ...
... //depot/googleclient/gears/opensource/gears/Makefile#189 edit
...
//depot/googleclient/gears/opensource/gears/geolocation/access_token_manager.cc#1
add
...
//depot/googleclient/gears/opensource/gears/geolocation/access_token_manager.h#1
add
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_provider.cc#25
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/reverse_geocoder.cc#1
edit
250 delta lines: 163 added, 81 deleted, 6 changed
Also consider running:
g4 lint -c 8513195
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 8513195 by [EMAIL PROTECTED] on 2008/10/07 15:25:57 *pending*
Adds access token management for reverse geocoding.
Affected files ...
... //depot/googleclient/gears/opensource/gears/Makefile#189 edit
...
//depot/googleclient/gears/opensource/gears/geolocation/access_token_manager.cc#1
add
...
//depot/googleclient/gears/opensource/gears/geolocation/access_token_manager.h#1
add
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_provider.cc#25
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/reverse_geocoder.cc#1
edit
==== //depot/googleclient/gears/opensource/gears/Makefile#189 -
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/Makefile ====
# action=edit type=text
--- googleclient/gears/opensource/gears/Makefile 2008-10-07
16:08:06.000000000 +0100
+++ googleclient/gears/opensource/gears/Makefile 2008-10-07
15:30:04.000000000 +0100
@@ -2396,6 +2396,7 @@
$(NULL)
$(BROWSER)_CPPSRCS += \
+ access_token_manager.cc \
empty_device_data_provider.cc \
geolocation.cc \
geolocation_db.cc \
====
//depot/googleclient/gears/opensource/gears/geolocation/access_token_manager.cc#1
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/access_token_manager.cc
====
# action=add type=text
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/access_token_manager.cc
2008-10-07 16:07:29.000000000 +0100
@@ -0,0 +1,86 @@
+// Copyright 2008, Google Inc.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+//
+// 1. Redistributions of source code must retain the above copyright notice,
+// this list of conditions and the following disclaimer.
+// 2. Redistributions in binary form must reproduce the above copyright
notice,
+// this list of conditions and the following disclaimer in the
documentation
+// and/or other materials provided with the distribution.
+// 3. Neither the name of Google Inc. nor the names of its contributors may be
+// used to endorse or promote products derived from this software without
+// specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR IMPLIED
+// WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO
+// EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
+// OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+// OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+// ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#include "gears/geolocation/access_token_manager.h"
+
+#include "gears/geolocation/geolocation_db.h"
+
+// static
+AccessTokenManager AccessTokenManager::instance_;
+
+// static
+AccessTokenManager *AccessTokenManager::GetInstance() {
+ return &instance_;
+}
+
+void AccessTokenManager::Register(const std::string16 &url) {
+ MutexLock lock(&access_tokens_mutex_);
+ user_count_.Ref();
+ // If we don't have a token for this URL in the map, try to get one from the
+ // database.
+ if (access_tokens_.find(url) == access_tokens_.end()) {
+ GeolocationDB *db = GeolocationDB::GetDB();
+ std::string16 access_token;
+ if (db && db->RetrieveAccessToken(url, &access_token)) {
+ // Empty tokens should never be stored in the DB.
+ assert(!access_token.empty());
+ access_tokens_[url] = access_token;
+ }
+ }
+}
+
+void AccessTokenManager::Unregister() {
+ MutexLock lock(&access_tokens_mutex_);
+ // If this is the last user, write the tokens to the database.
+ if (user_count_.Unref()) {
+ GeolocationDB *db = GeolocationDB::GetDB();
+ if (db) {
+ for (AccessTokenMap::const_iterator iter = access_tokens_.begin();
+ iter != access_tokens_.end();
+ iter++) {
+ if (!iter->second.empty()) {
+ db->StoreAccessToken(iter->first, iter->second);
+ }
+ }
+ }
+ }
+}
+
+void AccessTokenManager::GetToken(const std::string16 &url,
+ std::string16 *access_token) {
+ assert(access_token);
+ MutexLock lock(&access_tokens_mutex_);
+ AccessTokenMap::const_iterator iter = access_tokens_.find(url);
+ if (iter == access_tokens_.end()) {
+ access_token->clear();
+ } else {
+ *access_token = iter->second;
+ }
+}
+
+void AccessTokenManager::SetToken(const std::string16 &url, const
std::string16 &access_token) {
+ MutexLock lock(&access_tokens_mutex_);
+ access_tokens_[url] = access_token;
+}
====
//depot/googleclient/gears/opensource/gears/geolocation/access_token_manager.h#1
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/access_token_manager.h
====
# action=add type=text
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/access_token_manager.h
2008-10-07 15:35:20.000000000 +0100
@@ -0,0 +1,64 @@
+// Copyright 2008, Google Inc.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+//
+// 1. Redistributions of source code must retain the above copyright notice,
+// this list of conditions and the following disclaimer.
+// 2. Redistributions in binary form must reproduce the above copyright
notice,
+// this list of conditions and the following disclaimer in the
documentation
+// and/or other materials provided with the distribution.
+// 3. Neither the name of Google Inc. nor the names of its contributors may be
+// used to endorse or promote products derived from this software without
+// specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR IMPLIED
+// WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO
+// EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
+// OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+// OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+// ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+//
+// NetworkLocationRequest objects are specific to host, server URL and
language.
+// The access token should be specific to a server URL only. This class manages
+// sharing the access token between multiple NetworkLocationRequest objects,
and
+// reading and writing the value to the database for storage between sessions.
+
+#ifndef GEARS_GEOLOCATION_ACCESS_TOKEN_MANAGER_H__
+#define GEARS_GEOLOCATION_ACCESS_TOKEN_MANAGER_H__
+
+#include "gears/base/common/common.h"
+#include <map>
+#include "gears/base/common/mutex.h"
+#include "gears/base/common/scoped_refptr.h" // For RefCount
+
+class AccessTokenManager {
+ public:
+ static AccessTokenManager *GetInstance();
+ void Register(const std::string16 &url);
+ void Unregister();
+ // Returns the empty string if no token exists.
+ void GetToken(const std::string16 &url, std::string16 *access_token);
+ void SetToken(const std::string16 &url, const std::string16 &access_token);
+
+ private:
+ AccessTokenManager() {}
+ ~AccessTokenManager() {}
+
+ RefCount user_count_;
+
+ // A map from server URL to access token.
+ typedef std::map<std::string16, std::string16> AccessTokenMap;
+ AccessTokenMap access_tokens_;
+ Mutex access_tokens_mutex_;
+
+ static AccessTokenManager instance_;
+
+ DISALLOW_EVIL_CONSTRUCTORS(AccessTokenManager);
+};
+
+#endif // GEARS_GEOLOCATION_ACCESS_TOKEN_MANAGER_H__
====
//depot/googleclient/gears/opensource/gears/geolocation/network_location_provider.cc#25
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/network_location_provider.cc
====
# action=edit type=text
---
googleclient/gears/opensource/gears/geolocation/network_location_provider.cc
2008-10-07 16:08:06.000000000 +0100
+++
googleclient/gears/opensource/gears/geolocation/network_location_provider.cc
2008-10-07 15:36:35.000000000 +0100
@@ -25,9 +25,8 @@
#include "gears/geolocation/network_location_provider.h"
-#include "gears/base/common/event.h"
#include "gears/base/common/stopwatch.h" // For GetCurrentTimeMillis
-#include "gears/geolocation/geolocation_db.h"
+#include "gears/geolocation/access_token_manager.h"
// The maximum period of time we'll wait for a complete set of device data
// before sending the request.
@@ -36,86 +35,6 @@
static const int kBaselineMinimumRequestInterval = 1000 * 5; // 5 seconds
// The upper limit of the minimum period between network requests.
static const int kMinimumRequestIntervalLimit = 1000 * 60 * 60 * 3; // 3 hours
-
-
-// NetworkLocationRequest objects are specific to host, server URL and
language.
-// The access token should be specific to a server URL only. This class manages
-// sharing the access token between multiple NetworkLocationRequest objects,
and
-// reading and writing the value to the database for storage between sessions.
-class AccessTokenManager {
- public:
- static AccessTokenManager *GetInstance() {
- return &instance_;
- }
-
- void Register(const std::string16 &url) {
- MutexLock lock(&access_tokens_mutex_);
- user_count_.Ref();
- // If we don't have a token for this URL in the map, try to get one from
the
- // database.
- if (access_tokens_.find(url) == access_tokens_.end()) {
- GeolocationDB *db = GeolocationDB::GetDB();
- std::string16 access_token;
- if (db && db->RetrieveAccessToken(url, &access_token)) {
- // Empty tokens should never be stored in the DB.
- assert(!access_token.empty());
- access_tokens_[url] = access_token;
- }
- }
- }
-
- void Unregister() {
- MutexLock lock(&access_tokens_mutex_);
- // If this is the last user, write the tokens to the database.
- if (user_count_.Unref()) {
- GeolocationDB *db = GeolocationDB::GetDB();
- if (db) {
- for (AccessTokenMap::const_iterator iter = access_tokens_.begin();
- iter != access_tokens_.end();
- iter++) {
- if (!iter->second.empty()) {
- db->StoreAccessToken(iter->first, iter->second);
- }
- }
- }
- }
- }
-
- // Returns the empty string if no token exists.
- void GetToken(const std::string16 &url, std::string16 *access_token) {
- assert(access_token);
- MutexLock lock(&access_tokens_mutex_);
- AccessTokenMap::const_iterator iter = access_tokens_.find(url);
- if (iter == access_tokens_.end()) {
- access_token->clear();
- } else {
- *access_token = iter->second;
- }
- }
-
- void SetToken(const std::string16 &url, const std::string16 &access_token) {
- MutexLock lock(&access_tokens_mutex_);
- access_tokens_[url] = access_token;
- }
-
- private:
- AccessTokenManager() {}
- ~AccessTokenManager() {}
-
- RefCount user_count_;
-
- // A map from server URL to access token.
- typedef std::map<std::string16, std::string16> AccessTokenMap;
- AccessTokenMap access_tokens_;
- Mutex access_tokens_mutex_;
-
- static AccessTokenManager instance_;
-
- DISALLOW_EVIL_CONSTRUCTORS(AccessTokenManager);
-};
-
-// static
-AccessTokenManager AccessTokenManager::instance_;
// The BackoffManager class is used to implement exponential back-off for
====
//depot/googleclient/gears/opensource/gears/geolocation/reverse_geocoder.cc#1 -
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/reverse_geocoder.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/reverse_geocoder.cc
2008-10-07 16:08:06.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/reverse_geocoder.cc
2008-10-07 16:05:32.000000000 +0100
@@ -27,6 +27,7 @@
#include "gears/base/common/event.h"
#include "gears/base/common/stopwatch.h" // For GetCurrentTimeMillis
+#include "gears/geolocation/access_token_manager.h"
ReverseGeocoder::ReverseGeocoder(const std::string16 &url,
@@ -39,12 +40,16 @@
listener_(listener),
request_(NULL) {
assert(listener_);
+
+ AccessTokenManager::GetInstance()->Register(url_);
}
ReverseGeocoder::~ReverseGeocoder() {
if (request_) {
request_->StopThreadAndDelete();
}
+
+ AccessTokenManager::GetInstance()->Unregister();
}
bool ReverseGeocoder::MakeRequest(const Position &position) {
@@ -57,8 +62,9 @@
RadioData radio_data;
WifiData wifi_data;
- // TODO(steveblock): Correctly handle the access token.
- return request_->MakeRequest(STRING16(L""), // access_token
+ std::string16 access_token;
+ AccessTokenManager::GetInstance()->GetToken(url_, &access_token);
+ return request_->MakeRequest(access_token,
radio_data,
wifi_data,
true, // request_address
@@ -72,8 +78,14 @@
void ReverseGeocoder::LocationResponseAvailable(
const Position &position,
bool /* server_error */,
- const std::string16 & /* access_token */) {
- // TODO(steveblock): Correctly handle the access token and exponential
- // back-off in case of server error.
+ const std::string16 &access_token) {
+ // TODO(steveblock): Correctly handle exponential back-off in case of server
+ // error.
+
+ // Record access_token if it's set.
+ if (!access_token.empty()) {
+ AccessTokenManager::GetInstance()->SetToken(url_, access_token);
+ }
+
listener_->ReverseGeocodeAvailable(position);
}
