Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 9091816
or point your web browser to
http://mondrian/9091816
to review the following code:
Change 9091816 by [EMAIL PROTECTED] on 2008/11/20 18:35:19 *pending*
Changes timeout parameter in CondVar, Event, TimedCallback and
TimedMessage to use int64, rather than int.
This is required to support the full 53 bit range of JavaScript integer
values for Geolocation timeout values.
However, it turns out that the implementation on Win32 limits the range
of the timeout parameter to that of uint32, so I'm not sure that this change is
worthwhile right now. However, it does set an API and we could change the
implementation in the future.
R=andreip
[EMAIL PROTECTED]
DELTA=24 (13 added, 0 deleted, 11 changed)
OCL=9091816
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/event.cc#5 edit
... //depot/googleclient/gears/opensource/gears/base/common/event.h#2 edit
... //depot/googleclient/gears/opensource/gears/base/common/mutex.h#8 edit
... //depot/googleclient/gears/opensource/gears/base/common/mutex_posix.cc#2
edit
... //depot/googleclient/gears/opensource/gears/base/common/mutex_win32.cc#2
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#63
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#29
edit
... //depot/googleclient/gears/opensource/gears/geolocation/timed_callback.cc#5
edit
... //depot/googleclient/gears/opensource/gears/geolocation/timed_callback.h#3
edit
24 delta lines: 13 added, 0 deleted, 11 changed
Also consider running:
g4 lint -c 9091816
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 9091816 by [EMAIL PROTECTED] on 2008/11/20 18:35:19 *pending*
Changes timeout parameter in CondVar, Event, TimedCallback and
TimedMessage to use int64, rather than int.
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/event.cc#5 edit
... //depot/googleclient/gears/opensource/gears/base/common/event.h#2 edit
... //depot/googleclient/gears/opensource/gears/base/common/mutex.h#8 edit
... //depot/googleclient/gears/opensource/gears/base/common/mutex_posix.cc#2
edit
... //depot/googleclient/gears/opensource/gears/base/common/mutex_win32.cc#2
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#63
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#29
edit
... //depot/googleclient/gears/opensource/gears/geolocation/timed_callback.cc#5
edit
... //depot/googleclient/gears/opensource/gears/geolocation/timed_callback.h#3
edit
==== //depot/googleclient/gears/opensource/gears/base/common/event.cc#5 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/base/common/event.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/event.cc 2008-11-20
18:35:13.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/event.cc 2008-11-20
18:35:20.000000000 +0000
@@ -53,7 +53,7 @@
signal_ = false;
}
-bool Event::WaitWithTimeout(int timeout_milliseconds) {
+bool Event::WaitWithTimeout(int64 timeout_milliseconds) {
MutexLock lock(&mutex_);
while (!signal_) {
Stopwatch stopwatch;
==== //depot/googleclient/gears/opensource/gears/base/common/event.h#2 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/base/common/event.h ====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/event.h 2008-11-20
18:35:14.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/event.h 2008-11-20
18:35:23.000000000 +0000
@@ -60,7 +60,7 @@
void Wait();
// Blocks until the event is signalled or until the specified time limit
// expires. Returns true if the event was signalled.
- bool WaitWithTimeout(int timeout_milliseconds);
+ bool WaitWithTimeout(int64 timeout_milliseconds);
private:
Mutex mutex_;
==== //depot/googleclient/gears/opensource/gears/base/common/mutex.h#8 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/base/common/mutex.h ====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/mutex.h 2008-11-20
18:35:14.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/mutex.h 2008-11-20
19:16:59.000000000 +0000
@@ -238,7 +238,10 @@
// - this call was interrupted by a signal.
// Returns false if signalled, true if timed out.
// If both conditions are true, it can return either true or false.
- bool WaitWithTimeout(Mutex *mutex, int milliseconds);
+ //
+ // Note that the current implementation limits the range of milliseconds to
+ // that of uint32.
+ bool WaitWithTimeout(Mutex *mutex, int64 milliseconds);
// Signal this CondVar to awaken all waiters.
// It is more efficient (but not necessary) to call SignalAll after
==== //depot/googleclient/gears/opensource/gears/base/common/mutex_posix.cc#2 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/base/common/mutex_posix.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/mutex_posix.cc
2008-11-20 18:35:14.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/mutex_posix.cc
2008-11-20 19:29:47.000000000 +0000
@@ -140,12 +140,16 @@
}
-bool CondVar::WaitWithTimeout(Mutex *mutex, int milliseconds) {
+bool CondVar::WaitWithTimeout(Mutex *mutex, int64 milliseconds) {
// We've been handed a time duration, but pthread_cond_timedwait requires
// an absolute time for its argument.
struct timeval current_time;
struct timespec timeout;
gettimeofday(¤t_time, NULL);
+ // timeval::tv_sec has type time_t, which is typically int32, so will
overflow
+ // in 2038. We limit milliseconds to uint32, which equates to 50 days, and is
+ // insignificant in comparison.
+ assert(milliseconds >= 0 && milliseconds <= kuint32max);
timeout.tv_sec = current_time.tv_sec + (milliseconds / 1000);
timeout.tv_nsec = (current_time.tv_usec * 1000 +
(milliseconds % 1000) * 1000000);
==== //depot/googleclient/gears/opensource/gears/base/common/mutex_win32.cc#2 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/base/common/mutex_win32.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/mutex_win32.cc
2008-11-20 18:35:14.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/mutex_win32.cc
2008-11-20 19:19:23.000000000 +0000
@@ -136,7 +136,7 @@
}
-bool CondVar::WaitWithTimeout(Mutex *mutex, int milliseconds) {
+bool CondVar::WaitWithTimeout(Mutex *mutex, int64 milliseconds) {
#ifdef DEBUG
assert(mutex->is_locked_);
#endif
@@ -149,7 +149,10 @@
event = current_event_;
}
mutex->Unlock();
- DWORD result = WaitForSingleObject(event->Handle(), milliseconds);
+ // The timeout parameter is a DWORD.
+ assert(milliseconds >= 0 && milliseconds <= kuint32max);
+ DWORD result = WaitForSingleObject(event->Handle(),
+ static_cast<DWORD>(milliseconds));
mutex->Lock();
return result == WAIT_TIMEOUT;
}
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#63
-
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-11-20 20:09:16.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-11-20 19:33:55.000000000 +0000
@@ -76,7 +76,7 @@
static const int kLastSingleRequestId = kint32min; // Single IDs negative
-TimedMessage::TimedMessage(int timeout_milliseconds,
+TimedMessage::TimedMessage(int64 timeout_milliseconds,
const std::string16 &message_type,
NotificationData *notification_data)
: message_type_(message_type) {
@@ -1193,6 +1193,9 @@
assert(!fix_info->success_callback_timer.get());
fix_info->pending_position = position;
+ assert(timeout_milliseconds >= 0);
+ // The timeout parameter of TimedMessage is limited in range to uint32, so
+ // this is safe.
fix_info->success_callback_timer.reset(new TimedMessage(
timeout_milliseconds,
kCallbackRequiredObserverTopic,
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#29 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/geolocation/geolocation.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-11-20 20:09:16.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-11-20 18:43:39.000000000 +0000
@@ -134,7 +134,7 @@
// when the timer expires.
class TimedMessage : public TimedCallback::ListenerInterface {
public:
- TimedMessage(int timeout_milliseconds,
+ TimedMessage(int64 timeout_milliseconds,
const std::string16 &message_type,
NotificationData *notification_data);
virtual ~TimedMessage() {}
====
//depot/googleclient/gears/opensource/gears/geolocation/timed_callback.cc#5 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/geolocation/timed_callback.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/timed_callback.cc
2008-11-20 18:35:14.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/timed_callback.cc
2008-11-20 18:35:45.000000000 +0000
@@ -26,7 +26,7 @@
#include "gears/geolocation/timed_callback.h"
TimedCallback::TimedCallback(ListenerInterface *listener,
- int timeout_milliseconds,
+ int64 timeout_milliseconds,
void *user_data)
: listener_(listener),
timeout_(timeout_milliseconds),
==== //depot/googleclient/gears/opensource/gears/geolocation/timed_callback.h#3
-
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/geolocation/timed_callback.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/timed_callback.h
2008-11-20 18:35:14.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/timed_callback.h
2008-11-20 18:35:51.000000000 +0000
@@ -41,7 +41,7 @@
};
TimedCallback(ListenerInterface *listener,
- int timeout_milliseconds,
+ int64 timeout_milliseconds,
void *user_data);
~TimedCallback();
@@ -50,7 +50,7 @@
void Run();
ListenerInterface *listener_;
- int timeout_;
+ int64 timeout_;
void *user_data_;
Event stop_event_;
