* Simplified LockLooper() and LockLooperWithTimeout(), got rid of not really

helpful but extensive comments in the code.
* Fixed possible wrong error values of LockLooperWithTimeout().
* AddFilter()/RemoveFilter() now check if the looper is locked in case this
  handler belongs to a looper - as SetFilterList() already did.


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@16911 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/kits/app/Handler.cpp

@@ -389,26 +389,11 @@ BHandler::NextHandler() const
 void
 BHandler::AddFilter(BMessageFilter *filter)
 {
-	// NOTE:  Although the documentation states that the handler must belong to
+	if (fLooper && !fLooper->IsLocked()) {
-	// a looper and the looper must be locked in order to use this method,
+		debugger("Owning Looper must be locked before calling SetFilterList");
-	// testing shows that this is not the case in the original implementation.
+		return;
-	// We may want to investigate enforcing these rules; it would be interesting
-	// to see how many apps out there have violated the dictates of the docs.
-	// For now, though, we'll play nicely.
-#if 0
-	if (!fLooper)
-	{
-		// TODO: error handling
-		return false;
 	}
 
-	if (!fLooper->IsLocked())
-	{
-		// TODO: error handling
-		return false;
-	}
-#endif
-
 	if (fLooper != NULL)
 		filter->SetLooper(fLooper);
 		
@@ -422,26 +407,11 @@ BHandler::AddFilter(BMessageFilter *filter)
 bool
 BHandler::RemoveFilter(BMessageFilter *filter)
 {
-	// NOTE:  Although the documentation states that the handler must belong to
+	if (fLooper && !fLooper->IsLocked()) {
-	// a looper and the looper must be locked in order to use this method,
+		debugger("Owning Looper must be locked before calling SetFilterList");
-	// testing shows that this is not the case in the original implementation.
-	// We may want to investigate enforcing these rules; it would be interesting
-	// to see how many apps out there have violated the dictates of the docs.
-	// For now, though, we'll play nicely.
-#if 0
-	if (!fLooper)
-	{
-		// TODO: error handling
 		return false;
 	}
 
-	if (!fLooper->IsLocked())
-	{
-		// TODO: error handling
-		return false;
-	}
-#endif
-
 	if (fFilters != NULL && fFilters->RemoveItem((void *)filter)) {
 		filter->SetLooper(NULL);
 		return true;
@@ -454,20 +424,6 @@ BHandler::RemoveFilter(BMessageFilter *filter)
 void
 BHandler::SetFilterList(BList* filters)
 {
-	/**
-		@note	Although the documentation states that the handler must belong to
-				a looper and the looper must be locked in order to use this method,
-				testing shows that this is not the case in the original implementation.
-	 */
-
-#if 0
-	if (!fLooper)
-	{
-		// TODO: error handling
-		return;
-	}
-#endif
-
 	if (fLooper && !fLooper->IsLocked()) {
 		debugger("Owning Looper must be locked before calling SetFilterList");
 		return;
@@ -509,31 +465,16 @@ BHandler::FilterList()
 bool
 BHandler::LockLooper()
 {
-	/**
-		@note	BeBook says that this function "retrieves the handler's looper and
-				unlocks it in a pseudo-atomic operation, thus avoiding a race
-				condition."  How "pseudo-atomic" would look completely escapes me,
-				so we'll go with the dumb version for now.  Maybe I should use a
-				benaphore?
-
-				BeBook mentions handling the case where the handler's looper
-				changes during this call.  I've attempted a "pseudo-atomic"
-				operation to check that.
-	 */
-
 	BLooper *looper = fLooper;
-	if (looper) {
+	// Locking the looper also makes sure that the looper is valid
-		bool result = looper->Lock();
+	if (looper != NULL && looper->Lock()) {
-
+		// Have we locked the right looper? That's as far as the
-		// Are we still assigned to the same looper?
+		// "pseudo-atomic" operation mentioned in the BeBook.
 		if (fLooper == looper)
-			return result;
+			return true;
 
-		if (result) {
+		// we locked the wrong looper, bail out
-			// Our looper is different, and the lock was successful on the old
+		looper->Unlock();
-			// one; undo the lock
-			looper->Unlock();
-		}
 	}
 
 	return false;
@@ -543,58 +484,30 @@ BHandler::LockLooper()
 status_t
 BHandler::LockLooperWithTimeout(bigtime_t timeout)
 {
-	/**
-		@note	BeBook says that this function "retrieves the handler's looper and
-				unlocks it in a pseudo-atomic operation, thus avoiding a race
-				condition."  How "pseudo-atomic" would look completely escapes me,
-				so we'll go with the dumb version for now.  Maybe I should use a
-				benaphore?
-
-				BeBook mentions handling the case where the handler's looper
-				changes during this call.  I've attempted a "pseudo-atomic"
-				operation to check for that.
-	 */
-
 	BLooper *looper = fLooper;
-	if (looper) {
+	if (looper == NULL)
-		status_t result = looper->LockWithTimeout(timeout);
+		return B_BAD_VALUE;
 
-		// Are we still assigned to the same looper?
+	status_t status = looper->LockWithTimeout(timeout);
-		if (fLooper == looper)
+	if (status != B_OK)
-			return result;
+		return status;
-
-		// Our looper changed during the lock attempt
-		if (result == B_OK) {
-			// The lock was successful on the old looper; undo the lock
-			looper->Unlock();
-		}
 
+	if (fLooper != looper) {
+		// we locked the wrong looper, bail out
+		looper->Unlock();
 		return B_MISMATCHED_VALUES;
 	}
 
-	return B_BAD_VALUE;
+	return B_OK;
 }
 
 
 void
 BHandler::UnlockLooper()
 {
-	/**
+	// The looper is locked at this point, and cannot change
-		@note	BeBook says that this function "retrieves the handler's looper and
+	if (fLooper != NULL)
-				unlocks it in a pseudo-atomic operation, thus avoiding a race
+		fLooper->Unlock();
-				condition."  How "pseudo-atomic" would look completely escapes me,
-				so we'll go with the dumb version for now.  Maybe I should use a
-				benaphore?
-
-				The solution I used for Lock() and LockWithTimeout() seems out of
-				place here; if our looper does change while attempting to unlock it,
-				re-Lock()ing the original looper just doesn't seem right.
-	 */
-
-	// TODO: implement correctly
-	BLooper *looper = fLooper;
-	if (looper)
-		looper->Unlock();
 }