Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3153)

Unified Diff: base/threading/platform_thread_unittest.cc

Issue 1193303002: base/threading: restrict to set only current thread priority (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: two more android fix and review #2 Created 5 years, 6 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: base/threading/platform_thread_unittest.cc
diff --git a/base/threading/platform_thread_unittest.cc b/base/threading/platform_thread_unittest.cc
index c4b3d5d7ecc6092a89ca519800b55f92d3fd77da..89f0b225242c05229d2906776dc4539759bd482e 100644
--- a/base/threading/platform_thread_unittest.cc
+++ b/base/threading/platform_thread_unittest.cc
@@ -8,7 +8,10 @@
#include "base/threading/platform_thread.h"
#include "testing/gtest/include/gtest/gtest.h"
-#if defined(OS_WIN)
+#if defined(OS_POSIX)
+#include <sys/types.h>
+#include <unistd.h>
+#elif defined(OS_WIN)
#include <windows.h>
#endif
@@ -170,106 +173,47 @@ TEST(PlatformThreadTest, FunctionTimesTen) {
namespace {
const ThreadPriority kThreadPriorityTestValues[] = {
-// Disable non-normal priority toggling on POSIX as it appears to be broken
-// (http://crbug.com/468793). This is prefered to disabling the tests altogether
-// on POSIX as it at least provides coverage for running this code under
-// "normal" priority.
-#if !defined(OS_POSIX)
- ThreadPriority::DISPLAY,
ThreadPriority::REALTIME_AUDIO,
- // Keep BACKGROUND second to last to test backgrounding from other
- // priorities.
- ThreadPriority::BACKGROUND,
-#endif // !defined(OS_POSIX)
- // Keep NORMAL last to test unbackgrounding.
- ThreadPriority::NORMAL
gab 2015/06/23 17:01:32 The order here was intentional, please keep it thi
Takashi Toyoshima 2015/06/24 04:15:39 Yep, I changed this order because of the reason ga
gab 2015/06/26 15:57:13 But the code at the end of the test doesn't test t
Takashi Toyoshima 2015/06/29 05:48:51 Oh, that's right. I missed the point that the last
+ ThreadPriority::DISPLAY,
+ ThreadPriority::NORMAL,
+ ThreadPriority::BACKGROUND
};
} // namespace
-// Test changing another thread's priority.
-// NOTE: This test is partially disabled on POSIX, see note above and
-// http://crbug.com/468793.
-TEST(PlatformThreadTest, ThreadPriorityOtherThread) {
- PlatformThreadHandle current_handle(PlatformThread::CurrentHandle());
-
- // Confirm that the current thread's priority is as expected.
- EXPECT_EQ(ThreadPriority::NORMAL,
- PlatformThread::GetThreadPriority(current_handle));
-
- // Create a test thread.
- FunctionTestThread thread;
- PlatformThreadHandle handle;
- ASSERT_TRUE(PlatformThread::Create(0, &thread, &handle));
- thread.WaitForThreadStart();
- EXPECT_NE(thread.thread_id(), kInvalidThreadId);
- EXPECT_NE(thread.thread_id(), PlatformThread::CurrentId());
-
- // New threads should get normal priority by default.
- EXPECT_EQ(ThreadPriority::NORMAL, PlatformThread::GetThreadPriority(handle));
-
- // Toggle each supported priority on the test thread and confirm it only
- // affects it (and not the current thread).
- for (size_t i = 0; i < arraysize(kThreadPriorityTestValues); ++i) {
- SCOPED_TRACE(i);
-
- // Alter and verify the test thread's priority.
- PlatformThread::SetThreadPriority(handle, kThreadPriorityTestValues[i]);
- EXPECT_EQ(kThreadPriorityTestValues[i],
- PlatformThread::GetThreadPriority(handle));
-
- // Make sure the current thread was otherwise unaffected.
- EXPECT_EQ(ThreadPriority::NORMAL,
- PlatformThread::GetThreadPriority(current_handle));
- }
-
- thread.MarkForTermination();
- PlatformThread::Join(handle);
-}
+#if defined(OS_MACOSX)
+// PlatformThread::GetCurrentThreadPriority() is not implemented on OS X.
gab 2015/06/23 17:01:32 Nor on OS_ANDROID
Takashi Toyoshima 2015/06/24 04:15:39 Ah, you are right. But it's curious that Android b
gab 2015/06/25 14:49:00 Well actually Android should work for all but REAL
Takashi Toyoshima 2015/06/26 12:28:53 No, GetCurrentThreadPriorityForPlatform() has NOTR
gab 2015/06/26 15:57:13 No, the Android code has NOTIMPLEMENTED() which on
Takashi Toyoshima 2015/06/29 05:48:51 Oh, I confused them. Right, I'll enable the test f
+#define MAYBE_ThreadPriorityCurrentThread DISABLED_ThreadPriorityCurrentThread
+#else
+#define MAYBE_ThreadPriorityCurrentThread ThreadPriorityCurrentThread
+#endif
// Test changing the current thread's priority (which has different semantics on
// some platforms).
-// NOTE: This test is partially disabled on POSIX, see note above and
-// http://crbug.com/468793.
-TEST(PlatformThreadTest, ThreadPriorityCurrentThread) {
- PlatformThreadHandle current_handle(PlatformThread::CurrentHandle());
-
+TEST(PlatformThreadTest, MAYBE_ThreadPriorityCurrentThread) {
// Confirm that the current thread's priority is as expected.
EXPECT_EQ(ThreadPriority::NORMAL,
- PlatformThread::GetThreadPriority(current_handle));
-
- // Create a test thread for verification purposes only.
- FunctionTestThread thread;
- PlatformThreadHandle handle;
- ASSERT_TRUE(PlatformThread::Create(0, &thread, &handle));
- thread.WaitForThreadStart();
- EXPECT_NE(thread.thread_id(), kInvalidThreadId);
- EXPECT_NE(thread.thread_id(), PlatformThread::CurrentId());
-
- // Confirm that the new thread's priority is as expected.
- EXPECT_EQ(ThreadPriority::NORMAL, PlatformThread::GetThreadPriority(handle));
+ PlatformThread::GetCurrentThreadPriority());
// Toggle each supported priority on the current thread and confirm it only
// affects it (and not the test thread).
gab 2015/06/23 17:01:32 Fix this comment (there no longer is a test thread
Takashi Toyoshima 2015/06/24 04:15:39 Done.
for (size_t i = 0; i < arraysize(kThreadPriorityTestValues); ++i) {
SCOPED_TRACE(i);
+#if defined(OS_POSIX)
+ // Only root or a user with CAP_SYS_NICE permission can raise the thread
+ // priority.
gab 2015/06/23 17:01:32 Please document this in the SetCurrentThreadPriori
Takashi Toyoshima 2015/06/24 04:15:39 Done.
+ if (geteuid() != 0 && kThreadPriorityTestValues[i] > ThreadPriority::NORMAL)
gab 2015/06/23 17:01:32 Ah ok I now understand why you want to test backgr
Takashi Toyoshima 2015/06/24 04:15:39 Thanks you, that idea sounds nice.
+ continue;
+#endif
// Alter and verify the current thread's priority.
- PlatformThread::SetThreadPriority(current_handle,
- kThreadPriorityTestValues[i]);
+ PlatformThread::SetCurrentThreadPriority(kThreadPriorityTestValues[i]);
EXPECT_EQ(kThreadPriorityTestValues[i],
- PlatformThread::GetThreadPriority(current_handle));
-
- // Make sure the test thread was otherwise unaffected.
- EXPECT_EQ(ThreadPriority::NORMAL,
- PlatformThread::GetThreadPriority(handle));
+ PlatformThread::GetCurrentThreadPriority());
}
// Restore current thread priority for follow-up tests.
- PlatformThread::SetThreadPriority(current_handle, ThreadPriority::NORMAL);
-
- thread.MarkForTermination();
- PlatformThread::Join(handle);
+ PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL);
}
} // namespace base

Powered by Google App Engine
This is Rietveld 408576698