Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1193303002:
base/threading: restrict to set only current thread priority (Closed)
|
Created:
5 years, 6 months ago by Takashi Toyoshima Modified:
5 years, 5 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, jam, enne (OOO), Nico Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbase/threading: restrict to set only current thread priority
On some platforms, e.g., Linux, changing other thread's priority is not
permitted by sandbox mechanisms for a security reason.
Since there are no real use cases that really need to change another thread's
priority for now, I'll replace the base API with the one that change the
current thread priority to avoid confusion.
BUG=468793, 505474
TEST=base_unittests
TEST=sudo base_unittests --gtest_filter='Thread*'
TEST=git cl try
Committed: https://crrev.com/7c0e19558a837263c1c447a237b699f1fd17c7ee
Cr-Commit-Position: refs/heads/master@{#338661}
Patch Set 1 #Patch Set 2 : android and mac fix #
Total comments: 4
Patch Set 3 : two more android fix and review #2 #
Total comments: 29
Patch Set 4 : gab's review (though it sill contains parts that are split to others) #Patch Set 5 : rebase (win and linux changes are removed) #Patch Set 6 : rebase (hide compositor change) #Patch Set 7 : s/CurrentHandle().id()/CurrentId()/ #Patch Set 8 : (rebase for review) #
Total comments: 14
Patch Set 9 : #21, git cl format, fix win build #
Total comments: 6
Patch Set 10 : fix nits (#24) #
Total comments: 2
Patch Set 11 : rebase, review #29 #
Total comments: 2
Patch Set 12 : rebase only #Patch Set 13 : run the last test in a separate thread #
Total comments: 8
Patch Set 14 : review #48 #Dependent Patchsets: Messages
Total messages: 54 (17 generated)
thestig@chromium.org changed reviewers: + thestig@chromium.org
looking good https://codereview.chromium.org/1193303002/diff/20001/base/threading/platform... File base/threading/platform_thread_internal_posix.h (right): https://codereview.chromium.org/1193303002/diff/20001/base/threading/platform... base/threading/platform_thread_internal_posix.h:34: // Returns true if there is a platform-specific ThreadPriority set on |handle| still mentions |handle| https://codereview.chromium.org/1193303002/diff/20001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/20001/base/threading/platform... base/threading/platform_thread_unittest.cc:188: #if !defined(OS_MACOSX) I would just skip the test entirely on Mac.
https://codereview.chromium.org/1193303002/diff/20001/base/threading/platform... File base/threading/platform_thread_internal_posix.h (right): https://codereview.chromium.org/1193303002/diff/20001/base/threading/platform... base/threading/platform_thread_internal_posix.h:34: // Returns true if there is a platform-specific ThreadPriority set on |handle| On 2015/06/23 05:20:08, Lei Zhang wrote: > still mentions |handle| Done. https://codereview.chromium.org/1193303002/diff/20001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/20001/base/threading/platform... base/threading/platform_thread_unittest.cc:188: #if !defined(OS_MACOSX) On 2015/06/23 05:20:08, Lei Zhang wrote: > I would just skip the test entirely on Mac. Done.
toyoshim@chromium.org changed reviewers: + kbr@chromium.org
+kbr@ for content/renderer/gpu since I'm applying non-mechanical changes on compositor_output_surface.cc.
gab@chromium.org changed reviewers: + gab@chromium.org
lg https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread.h (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread.h:194: // Toggles the current thread's priority at runtime. Should we add an explanation here with link to a bug/discussion for anyone who wonders why this API doesn't support setting priority of other threads? https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_internal_posix.h (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_internal_posix.h:34: // Returns true if there is a platform-specific ThreadPriority set on current nit: s/on current thread/on the current thread/ https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_linux.cc:50: pthread_setschedparam(pthread_self(), SCHED_OTHER, &kResetPrio); I think we should do a separate CL for actual logic changes which shouldn't be mixed amongst mechanical API changes. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_posix.cc:253: << PlatformThread::CurrentHandle().id() If I'm not mistaken this is the only use of PlatformHandle::id() after this CL; if so, and especially if the only use case is logging now, we should remove it and log PlatformThread::CurrentId() instead. I never liked the fact that the id() property as it's not cross-platform (not set on Windows) and am excited to get rid of it if we can :-). (this could in this CL or as a follow-up CL with a TODO in this one, I'm fine either way) https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (left): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: ThreadPriority::NORMAL The order here was intentional, please keep it this way (with comments) rather than having it match the enum's declaration order). https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: // PlatformThread::GetCurrentThreadPriority() is not implemented on OS X. Nor on OS_ANDROID https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:199: // affects it (and not the test thread). Fix this comment (there no longer is a test thread). https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:204: // priority. Please document this in the SetCurrentThreadPriority() API; i.e. a thread may not be able to raise its priority back up after lowering it. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:205: if (geteuid() != 0 && kThreadPriorityTestValues[i] > ThreadPriority::NORMAL) Ah ok I now understand why you want to test backgrounding last on Linux (can't bring priority back up after)... hmmm... perhaps we need different orders on different platforms...? Perhaps the enum should be: AUDIO, DISPLAY, BACKGROUND, NORMAL, BACKGROUND and here this code would check > CurrentPriority() instead of explicitly > NORMAL? To skip unbackgrounding? https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_win.cc:198: SetCurrentThreadPriority(priority); Hmm, this is incorrect no? This code runs on the thread creating the thread of interest, not on that thread itself. I think we may need something like StartWithOptions in the Windows implementation as well (as a precursor CL). https://codereview.chromium.org/1193303002/diff/40001/content/renderer/gpu/co... File content/renderer/gpu/compositor_output_surface.h (right): https://codereview.chromium.org/1193303002/diff/40001/content/renderer/gpu/co... content/renderer/gpu/compositor_output_surface.h:109: scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner_; Please keep logic changes to another CL here as well.
https://codereview.chromium.org/1193303002/diff/40001/content/renderer/gpu/co... File content/renderer/gpu/compositor_output_surface.h (right): https://codereview.chromium.org/1193303002/diff/40001/content/renderer/gpu/co... content/renderer/gpu/compositor_output_surface.h:109: scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner_; On 2015/06/23 17:01:32, gab wrote: > Please keep logic changes to another CL here as well. I agree with gab's assessment.
kbr@chromium.org changed reviewers: + enne@chromium.org
Also, +enne, who's a better reviewer for compositor_output_surface than me.
Here is a separated CL for the CompositorOutputSurface. https://codereview.chromium.org/1205783002/ I added enne@ for the CL as a reviewer. Please jump in if you are also interested in.
Another separated CL for a bug sticking to READLTIME_AUDIO. https://codereview.chromium.org/1205703005/
Thank you gab. I uploaded a new patch set that fix your points. But it is not rebased yet. So it still contains parts that are split to other CLs. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread.h (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread.h:194: // Toggles the current thread's priority at runtime. On 2015/06/23 17:01:32, gab wrote: > Should we add an explanation here with link to a bug/discussion for anyone who > wonders why this API doesn't support setting priority of other threads? Done. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_internal_posix.h (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_internal_posix.h:34: // Returns true if there is a platform-specific ThreadPriority set on current On 2015/06/23 17:01:32, gab wrote: > nit: s/on current thread/on the current thread/ Done. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_linux.cc:50: pthread_setschedparam(pthread_self(), SCHED_OTHER, &kResetPrio); On 2015/06/23 17:01:32, gab wrote: > I think we should do a separate CL for actual logic changes which shouldn't be > mixed amongst mechanical API changes. Done. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_posix.cc:253: << PlatformThread::CurrentHandle().id() Thread::thread_id() still calls it, so it isn't as simple as I do it in this patch. But it is needed for one of my goal, https://codereview.chromium.org/1150563002/. I tried to achieve it without removing it, but it caused tricky issues. So, I'm also happy if I can remove it. I'd add TODO in the header file. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (left): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: ThreadPriority::NORMAL Yep, I changed this order because of the reason gap@ also mentioned in the following comment. Also, NORMAL is not needed to be the last since we already have a code to set it back to NORMAL at the end of the test. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: // PlatformThread::GetCurrentThreadPriority() is not implemented on OS X. Ah, you are right. But it's curious that Android build bots pass the test. Anyway, I'll add OS_ANDROID here. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:199: // affects it (and not the test thread). On 2015/06/23 17:01:32, gab wrote: > Fix this comment (there no longer is a test thread). Done. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:204: // priority. On 2015/06/23 17:01:32, gab wrote: > Please document this in the SetCurrentThreadPriority() API; i.e. a thread may > not be able to raise its priority back up after lowering it. Done. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:205: if (geteuid() != 0 && kThreadPriorityTestValues[i] > ThreadPriority::NORMAL) Thanks you, that idea sounds nice. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_win.cc:198: SetCurrentThreadPriority(priority); Oops, thank you for catching this. Yes, definitely I should fix it in a separate CL. Thanks! https://codereview.chromium.org/1193303002/diff/40001/content/renderer/gpu/co... File content/renderer/gpu/compositor_output_surface.h (right): https://codereview.chromium.org/1193303002/diff/40001/content/renderer/gpu/co... content/renderer/gpu/compositor_output_surface.h:109: scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner_; On 2015/06/23 17:01:32, gab wrote: > Please keep logic changes to another CL here as well. Done.
FYI, here is a split CL for Windows https://codereview.chromium.org/1199313004/
Patchset #6 (id:100001) has been deleted
Thanks, please ping after rebasing and I'll do another round then. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: // PlatformThread::GetCurrentThreadPriority() is not implemented on OS X. On 2015/06/24 04:15:39, Takashi Toyoshima (chromium) wrote: > Ah, you are right. But it's curious that Android build bots pass the test. > Anyway, I'll add OS_ANDROID here. Well actually Android should work for all but REALTIME_AUDIO (still very surprising that it passed that one though?). Can we add a selective ifdef above rather than disable the entire test?
content/gpu and content/renderer/gpu lgtm
gab@, now child CLs were submitted and this main CL was rebased to be ready for another review cycle. PTAL. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: // PlatformThread::GetCurrentThreadPriority() is not implemented on OS X. No, GetCurrentThreadPriorityForPlatform() has NOTREACHED() internally, and it is called first in GetCurrentThreadPriority(). So, it should crash with NOTREACHED() in debug build as OS X does. Another option is to enable it only in release build?
toyoshim@chromium.org changed reviewers: + jam@chromium.org
+jam especially for content/browser where no one reviewed.
lg, mostly nits + desire to get tests for Android for all priority levels but REALTIME_AUDIO (since comments below). Cheers, Gab https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (left): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: ThreadPriority::NORMAL On 2015/06/24 04:15:39, Takashi Toyoshima (chromium) wrote: > Yep, I changed this order because of the reason gap@ also mentioned in the > following comment. > > Also, NORMAL is not needed to be the last since we already have a code to set it > back to NORMAL at the end of the test. But the code at the end of the test doesn't test the result whereas this was explicitly testing unbackgrounding -- the latest patch set with the redundant BACKGROUND,NORMAL,BACKGROUND is okay I think. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: // PlatformThread::GetCurrentThreadPriority() is not implemented on OS X. On 2015/06/26 12:28:53, Takashi Toyoshima (chromium) wrote: > No, GetCurrentThreadPriorityForPlatform() has NOTREACHED() internally, and it is > called first in GetCurrentThreadPriority(). So, it should crash with > NOTREACHED() in debug build as OS X does. > Another option is to enable it only in release build? No, the Android code has NOTIMPLEMENTED() which only results in a log message not in a crash like NOTREACHED(). https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... File base/threading/platform_thread.h (left): https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread.h:179: // the thread is set based on |priority|. Can be used in place of Create() s/Can be used in place of/Prefer this to/ https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_posix.cc:63: } nit: can remove {} now. https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_posix.cc:268: &platform_specific_priority)) { nit: indent 4 spaces more (git cl format?) https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_unittest.cc:176: // Basically, the order should be higher to lower to cover as much cases Remove "Basically" I'd say, i.e.: // The order should be (...) https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_unittest.cc:211: PlatformThread::GetCurrentThreadPriority(); inline this in the conditional below maybe? (otherwise you end up with a local scope variable only on POSIX -- i.e., if someone is changing this code on Windows and adds a variable with the same name outside the ifdef, it won't fail to compile until they hit a Linux trybot). https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_unittest.cc:212: if (geteuid() != 0 && kThreadPriorityTestValues[i] > current_priority) According to http://stackoverflow.com/questions/4159910/check-if-user-is-root-in-c checking for root is perhaps incorrect/overkill (i.e. the user could be non-root but have CAP_SYS_NICE?). Is it worth checking for this permission rather than checking for root? Also, please check for this once outside the loop instead of every time inside, i.e.: const bool bumping_priority_allowed = #if defined(OS_POSIX) IsCapSysNicePermissionSet(); #else true; #endif https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_win.cc:60: } nit: can remove {}
Patchset #9 (id:170001) has been deleted
https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (left): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: ThreadPriority::NORMAL Oh, that's right. I missed the point that the last NORMAL is for testing unbackgrounding. https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:185: // PlatformThread::GetCurrentThreadPriority() is not implemented on OS X. Oh, I confused them. Right, I'll enable the test for Android with an exception for REALTIME. Thanks! https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... File base/threading/platform_thread.h (left): https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread.h:179: // the thread is set based on |priority|. Can be used in place of Create() I removed this part because Create() && SetCurrentThreadPriority() is not alternative any more since SetCurrentThreadPriority() should be called in the created thread. https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_posix.cc:63: } On 2015/06/26 15:57:13, gab wrote: > nit: can remove {} now. Done. https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_posix.cc:268: &platform_specific_priority)) { On 2015/06/26 15:57:13, gab wrote: > nit: indent 4 spaces more (git cl format?) Done. https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_unittest.cc:176: // Basically, the order should be higher to lower to cover as much cases On 2015/06/26 15:57:13, gab wrote: > Remove "Basically" I'd say, i.e.: > > // The order should be (...) Done. https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_unittest.cc:211: PlatformThread::GetCurrentThreadPriority(); On 2015/06/26 15:57:13, gab wrote: > inline this in the conditional below maybe? (otherwise you end up with a local > scope variable only on POSIX -- i.e., if someone is changing this code on > Windows and adds a variable with the same name outside the ifdef, it won't fail > to compile until they hit a Linux trybot). Done. https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_unittest.cc:212: if (geteuid() != 0 && kThreadPriorityTestValues[i] > current_priority) To check the capability, we need libcap.so and the code works only on Linux. I think depending on libcap.so only for this check is over engineering. I updated code and comments to explain details. https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1193303002/diff/150001/base/threading/platfor... base/threading/platform_thread_win.cc:60: } On 2015/06/26 15:57:13, gab wrote: > nit: can remove {} Done.
lgtm w/ nits, please add to the CL description why this change is required (for reference if anyone ever wonders why this is this way -- the referenced bug doesn't quite explain that I think) Thanks, Gab https://codereview.chromium.org/1193303002/diff/160013/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/160013/base/threading/platfor... base/threading/platform_thread_unittest.cc:180: // REALTIME_AUDIO case. Should we file a bug for this and try to get someone on Android to take a look at it? It's really only missing a Java hook similar to the one used to set priority I assume. https://codereview.chromium.org/1193303002/diff/160013/base/threading/platfor... base/threading/platform_thread_unittest.cc:194: // libcap.so will be needed to check the capability. s/will/would/ https://codereview.chromium.org/1193303002/diff/160013/base/threading/platfor... base/threading/platform_thread_unittest.cc:224: continue; nit: {} since conditional is multiline
Thanks, gab. I'll wait for the final OWNERS stamp from jam@ https://codereview.chromium.org/1193303002/diff/160013/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/160013/base/threading/platfor... base/threading/platform_thread_unittest.cc:180: // REALTIME_AUDIO case. On 2015/06/29 13:04:00, gab wrote: > Should we file a bug for this and try to get someone on Android to take a look > at it? It's really only missing a Java hook similar to the one used to set > priority I assume. Done. https://codereview.chromium.org/1193303002/diff/160013/base/threading/platfor... base/threading/platform_thread_unittest.cc:194: // libcap.so will be needed to check the capability. On 2015/06/29 13:04:00, gab wrote: > s/will/would/ Done. https://codereview.chromium.org/1193303002/diff/160013/base/threading/platfor... base/threading/platform_thread_unittest.cc:224: continue; On 2015/06/29 13:04:00, gab wrote: > nit: {} since conditional is multiline Done.
toyoshim@chromium.org changed reviewers: + kinuko@chromium.org
hum, jam@ seems to be in vacation. kinuko@ can you give me a stamp for content/browser ? Lei already said LG for base/, so I think it's ok to land this with TBR once I get a stamp for content/browser.
content/ changes lgtm. It'd be still good to get clear l/g/t/m for base/ changes though.
one nit https://codereview.chromium.org/1193303002/diff/200001/base/threading/platfor... File base/threading/platform_thread.h (right): https://codereview.chromium.org/1193303002/diff/200001/base/threading/platfor... base/threading/platform_thread.h:92: // TODO(toyoshim): Remove id() and use PlatformThread::CurrentId() instead. nit: would be nice to have a bug number (crbug link) here too
https://codereview.chromium.org/1193303002/diff/200001/base/threading/platfor... File base/threading/platform_thread.h (right): https://codereview.chromium.org/1193303002/diff/200001/base/threading/platfor... base/threading/platform_thread.h:92: // TODO(toyoshim): Remove id() and use PlatformThread::CurrentId() instead. Done. I'd share the same bug ID here since I already have a patch to remove this TODO here; https://codereview.chromium.org/1207823004/
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, gab@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/1193303002/#ps220001 (title: "rebase, review #29")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1193303002/220001
On 2015/07/02 07:43:09, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1193303002/220001 I recommend you get explicit l/g/t/m for base change.
The CQ bit was unchecked by kinuko@chromium.org
On 2015/07/02 07:48:37, kinuko wrote: > On 2015/07/02 07:43:09, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1193303002/220001 > > I recommend you get explicit l/g/t/m for base change. (Not sure what Lei meant but 'looking good' does not exactly equal to l/g/t/m)
toyoshim@chromium.org changed reviewers: + thakis@chromium.org
ok, let me add Nico for base since Lei has been OOO.
Lei Zhang: ping for the remaining base/ OWNERS review.
toyoshim@chromium.org changed reviewers: - enne@chromium.org, jam@chromium.org, thakis@chromium.org
thakis@chromium.org changed reviewers: + thakis@chromium.org
Sorry, feel free to ping me much sooner if I don't respond. https://codereview.chromium.org/1193303002/diff/220001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/220001/base/threading/platfor... base/threading/platform_thread_unittest.cc:234: PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); Is this not a bump in practice? (From background to normal) According to the new comment in platform_thread.h, will this not work on linux, and keep the test thread in background priority for the remaining tests?
Thank you Nico. I'll try creating a new patch set to run the test in a dedicated thread.
oops, my comment was not published. https://codereview.chromium.org/1193303002/diff/220001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/220001/base/threading/platfor... base/threading/platform_thread_unittest.cc:234: PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); That's true. If this could be a serious issue, I'll change this test to run in a dedicated thread.
Patchset #12 (id:240001) has been deleted
On 2015/07/14 05:21:41, Takashi Toyoshima (chromium) wrote: > Thank you Nico. I'll try creating a new patch set to run the test in a dedicated > thread. Ok, I'll wait for the next patch set. Everything else looks good.
Nico, and Lei: PTAL Patch Set 13.
lgtm with some comments. https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... base/threading/platform_thread_unittest.cc:82: // Grabs |thread_id_|, runs tests, signals |termination_ready_|, and then s/runs tests/runs an optional test on that thread/ ? https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... base/threading/platform_thread_unittest.cc:116: virtual void RunTest() {} Add a brief comment about running an optional test on the newly created thread. https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... base/threading/platform_thread_unittest.cc:116: virtual void RunTest() {} Make this private since derived classes should only be able to implement it but not call it? https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... base/threading/platform_thread_unittest.cc:218: void RunTest() override { Also private.
Thanks! https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... base/threading/platform_thread_unittest.cc:82: // Grabs |thread_id_|, runs tests, signals |termination_ready_|, and then On 2015/07/14 07:54:20, Lei Zhang wrote: > s/runs tests/runs an optional test on that thread/ ? Done. https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... base/threading/platform_thread_unittest.cc:116: virtual void RunTest() {} On 2015/07/14 07:54:20, Lei Zhang wrote: > Add a brief comment about running an optional test on the newly created thread. Done. https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... base/threading/platform_thread_unittest.cc:116: virtual void RunTest() {} On 2015/07/14 07:54:20, Lei Zhang wrote: > Make this private since derived classes should only be able to implement it but > not call it? Done. https://codereview.chromium.org/1193303002/diff/280001/base/threading/platfor... base/threading/platform_thread_unittest.cc:218: void RunTest() override { On 2015/07/14 07:54:20, Lei Zhang wrote: > Also private. Done.
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, gab@chromium.org, kinuko@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1193303002/#ps300001 (title: "review #48")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1193303002/300001
Message was sent while issue was closed.
Committed patchset #14 (id:300001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/7c0e19558a837263c1c447a237b699f1fd17c7ee Cr-Commit-Position: refs/heads/master@{#338661} |
