Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11968044:
Fix login visual hitch on chromebook (Closed)
|
Created:
7 years, 11 months ago by bshe Modified:
7 years, 11 months ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix login visual hitch on chromebook
There are two parts contribute to the visual hitch:
1. Wallpaper is loaded on FILE thread which is blocked by other
task on FILE thread.
2. Wallpaper loading is postponded because of checking untrusted values.
To address them, this CL move wallpaper loading operation to sequenced
worker thread and remove the untrusted value check.
tbr=davemoore
BUG=168297
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178870
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add settings observer and using unnamed sequence thread #
Total comments: 19
Patch Set 3 : #
Total comments: 21
Patch Set 4 : #
Total comments: 1
Patch Set 5 : Fix some unit and browser tests #
Total comments: 9
Patch Set 6 : Add issue # #Patch Set 7 : #Patch Set 8 : rebase #Messages
Total messages: 41 (0 generated)
Hi Nikita. Could you please take a look at this CL? Thanks! https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/wallpaper_manager.cc (left): https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/wallpaper_manager.cc:662: } This seems safe to remove if we are fine to change wallpaper to default if needed. We already had the setting observer in existing_user_controller. http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo...
https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/wallpaper_manager.cc (left): https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/wallpaper_manager.cc:662: } On 2013/01/17 17:09:41, bshe wrote: > This seems safe to remove if we are fine to change wallpaper to default if > needed. We already had the setting observer in existing_user_controller. > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo... That's fine to omit this check on initial show but you should also subsribe for that prefs change. Example: http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/webui/chr... http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/webui/chr...
For prefs change, I think existing_user_controller already observe it. And we can rely on the code in there to set wallpaper appropriately. http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo... and http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo... So we probably dont need to observe it here. Or maybe I misunderstand the code? On 2013/01/18 13:09:41, Nikita Kostylev wrote: > https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... > File chrome/browser/chromeos/login/wallpaper_manager.cc (left): > > https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... > chrome/browser/chromeos/login/wallpaper_manager.cc:662: } > On 2013/01/17 17:09:41, bshe wrote: > > This seems safe to remove if we are fine to change wallpaper to default if > > needed. We already had the setting observer in existing_user_controller. > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo... > > That's fine to omit this check on initial show but you should also subsribe for > that prefs change. > > Example: > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/webui/chr... > > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/webui/chr...
On 2013/01/18 15:43:30, bshe wrote: > For prefs change, I think existing_user_controller already observe it. And we > can rely on the code in there to set wallpaper appropriately. > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo... > and > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo... > > So we probably dont need to observe it here. Or maybe I misunderstand the code? I don't think so. I think that you may end up with http://code.google.com/p/chromium/issues/detail?id=162760 again. Fix was exactly about adding this very code that you're now deleting. https://codereview.chromium.org/11299304/diff/25001/chrome/browser/chromeos/l...
On 2013/01/18 16:24:49, Nikita Kostylev wrote: > On 2013/01/18 15:43:30, bshe wrote: > > For prefs change, I think existing_user_controller already observe it. And we > > can rely on the code in there to set wallpaper appropriately. > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo... > > and > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo... > > > > So we probably dont need to observe it here. Or maybe I misunderstand the > code? > > I don't think so. I think that you may end up with > http://code.google.com/p/chromium/issues/detail?id=162760 again. > > Fix was exactly about adding this very code that you're now deleting. > > https://codereview.chromium.org/11299304/diff/25001/chrome/browser/chromeos/l... But at the same time this code in ExistingUserController _is_ supposed to take care on prefs/policy change. if (type == chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED || type == chrome::NOTIFICATION_POLICY_USER_LIST_CHANGED) { if (host_ != NULL) { // Signed settings or user list changed. Notify views and update them. UpdateLoginDisplay(chromeos::UserManager::Get()->GetUsers()); return; }
+Joao
https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/user_image_loader.cc:42: GetNamedSequenceToken(kUserImageLoaderTokenName); This does mean that there would no 2 image loading operation at a time. // No two tasks with the same token will run at the same time. I think we should not have such restriction. Imagine login screen with 18 users and each of them with custom image. https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/user_image_loader.h (right): https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/user_image_loader.h:45: static const char* kUserImageLoaderTokenName; static const char kUserImageLoaderTokenName[]
@Nikita: not entirely sure what I should be looking at here; a couple of comments inline. https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/user_image_loader.cc:42: GetNamedSequenceToken(kUserImageLoaderTokenName); On 2013/01/18 16:40:13, Nikita Kostylev wrote: > This does mean that there would no 2 image loading operation at a time. > > // No two tasks with the same token will run at the same time. > > I think we should not have such restriction. Imagine login screen with 18 users > and each of them with custom image. > AFAICT the ImageDecoder spawns a utility process to decode the image, and multiple processes can be spawned at the same time. If so then using a non-sequenced task runner won't make the loading faster, but will complicated this class because OnImageDecoded() will be invoked concurrently, and mutates image_info_map_. https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/user_image_loader.cc:74: image_decoder->Start(kUserImageLoaderTokenName); The current task_runner could be passed instead of the token. https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/user_image_loader.h (right): https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/user_image_loader.h:45: static const char* kUserImageLoaderTokenName; This could also be hidden in the .cc file, in an anonymous namespace.
On 2013/01/18 17:24:58, Joao da Silva wrote: > @Nikita: not entirely sure what I should be looking at here; a couple of > comments inline. Joao, sorry I forgot to give you some context on that. What would be a recommended way to listen for preferences changes. There is notification chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED and also settings observer: CrosSettings::Get()->AddSettingsObserver(kAccountsPrefAllowNewUser, this);
Biao, AFAIK ExistingUserController would refresh sign in UI in case of these notifications (chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED, chrome::NOTIFICATION_POLICY_USER_LIST_CHANGED) but that wouldn't trigger wallpaper change.
https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/user_image_loader.cc:42: GetNamedSequenceToken(kUserImageLoaderTokenName); On 2013/01/18 17:24:58, Joao da Silva wrote: > On 2013/01/18 16:40:13, Nikita Kostylev wrote: > > This does mean that there would no 2 image loading operation at a time. > > > > // No two tasks with the same token will run at the same time. > > > > I think we should not have such restriction. Imagine login screen with 18 > users > > and each of them with custom image. > > > > AFAICT the ImageDecoder spawns a utility process to decode the image, and > multiple processes can be spawned at the same time. If so then using a > non-sequenced task runner won't make the loading faster, but will complicated > this class because OnImageDecoded() will be invoked concurrently, and mutates > image_info_map_. My point was that first it will load image using this token sequence_token_. That means that all images will be loaded one by one. For user images we'll use single UserImageLoader instance so I agree that there would be issues while accessing image_info_map_. It should be lock protected then.
Julian is the original author of CrosSettings, adding him. @Julian: What would be a recommended way to listen for preferences changes. There is notification chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED and also settings observer: CrosSettings::Get()->AddSettingsObserver(kAccountsPrefAllowNewUser, this);
On 2013/01/18 17:46:02, Joao da Silva wrote: > Julian is the original author of CrosSettings, adding him. > > @Julian: What would be a recommended way to listen for preferences changes. > There is notification chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED and also > settings observer: > > CrosSettings::Get()->AddSettingsObserver(kAccountsPrefAllowNewUser, this); Thanks for all the suggestions. I have uploaded another patch. In that patch, I have addressed the following: 1. Add a SettingObserver for preference changes in wallpaper_manager to listen for "Show User name..." option change. 2. Use unnamed sequence thread to load all images and lock protect image_info_map_ Could you please take another look? Thanks!
I'm not a big fan of locks :-) Please consider the comments inline. https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:52: base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN); If this is using a sequenced task runner then it won't execute tasks in parallel. Just post directly to the blocking pool. https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:86: const ImageInfo& image_info = info_it->second; info_it may be invalid at this point, because another thread may have just erased it or another iterator from the map. Locking access to image_info_map_ adds complexity and is very easy to break in the future. I'd suggest the following instead: - Start() posts to the blocking pool directly, to have parallel tasks; - OnImageDecoded() can then enter from several threads; just make it post again to UI: void UserImageLoader::OnImageDecoded(decoder, decoded_image) { BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, base::Bind(&UserImageLoader::ProcessDecodedImage, this, decoder, decoded_image)); } And add a new ProcessDecodedImage() method that always runs on UI and does what OnImageDecoded() is doing now. Same thing for OnDecodeImageFailed. https://codereview.chromium.org/11968044/diff/13001/chrome/browser/image_deco... File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/image_deco... chrome/browser/image_decoder.cc:40: const base::SequencedWorkerPool::SequenceToken& sequence_token) { Why not have a single Start() that gets a TaskRunner? The 1st version is invoked as base::TaskRunner* task_runner = BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE); decoder->Start(task_runner); and the 2nd: base::TaskRunner* task_runner = BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(sequence_token); decoder->Start(task_runner); CalledOnValidThreadOrWorkerThread() then just becomes: task_runner_->RunsTasksOnCurrentThread() https://codereview.chromium.org/11968044/diff/13001/chrome/browser/image_deco... chrome/browser/image_decoder.cc:45: base::Bind(&ImageDecoder::DecodeImageInSandbox, this, image_data_)); nit: indent (4 spaces)
I added my comments on the policy retrieval as well as a small nit. I also second Joao's concern on the use of locks for the logic. Please consider doing what he has proposed. https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:274: kAccountsPrefShowUserNamesOnSignIn) Multi-line conditions needs { } around the body. https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:670: bool result = CrosSettings::Get()->GetBoolean( Without the condition you removed on |PrepareTrustedValues| you can't not be sure whether the value you get here is actually coming from the trusted policy store or from a possibly expired cache. If this logic only controls which image to show as a background it is not critical enough to care but if you make any more grave decisions than that please consider reinstating the logic you had here before.
Thanks for review! Just post my reply on some comments that need more discussion. I am working on other comments. https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:52: base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN); pool->GetSequenceToken will return a different token each time. I thought sequenced task with different token can execute paralle. But I maybe wrong? Are you suggesting to use GetTaskRunnerWithShutdownBehavior here? On 2013/01/21 08:07:39, Joao da Silva wrote: > If this is using a sequenced task runner then it won't execute tasks in > parallel. Just post directly to the blocking pool. https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:86: const ImageInfo& image_info = info_it->second; If post back to UI, I worry this may be blocked on some tasks running on UI at start up. And the whole point of this CL is to unblock the operation as much as possible especially at start up. I remember I tried to execute the image_decoder->Start from UI thread and measure the time spend on each phrase. The time it takes from decoding finished on uitility process to start execute this function is around 100ms(cant remember the exact number but it is a large number). So we must wait some tasks to finish on UI thread before execute this. I will double check with your suggested approach and get some concrete measurement on real device. On 2013/01/21 08:07:39, Joao da Silva wrote: > info_it may be invalid at this point, because another thread may have just > erased it or another iterator from the map. > > Locking access to image_info_map_ adds complexity and is very easy to break in > the future. I'd suggest the following instead: > > - Start() posts to the blocking pool directly, to have parallel tasks; > - OnImageDecoded() can then enter from several threads; just make it post again > to UI: > > void UserImageLoader::OnImageDecoded(decoder, decoded_image) { > BrowserThread::PostTask( > BrowserThread::UI, FROM_HERE, > base::Bind(&UserImageLoader::ProcessDecodedImage, this, decoder, > decoded_image)); > } > > And add a new ProcessDecodedImage() method that always runs on UI and does what > OnImageDecoded() is doing now. > > Same thing for OnDecodeImageFailed.
https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:52: base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN); On 2013/01/21 16:04:40, bshe wrote: > pool->GetSequenceToken will return a different token each time. I thought > sequenced task with different token can execute paralle. But I maybe wrong? Are > you suggesting to use GetTaskRunnerWithShutdownBehavior here? > Yes, you're right. Either call GetTaskRunnerWithShutdownBehavior(), or just use the blocking pool directly (it's a TaskRunner too). > On 2013/01/21 08:07:39, Joao da Silva wrote: > > If this is using a sequenced task runner then it won't execute tasks in > > parallel. Just post directly to the blocking pool. > https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:86: const ImageInfo& image_info = info_it->second; On 2013/01/21 16:04:40, bshe wrote: > If post back to UI, I worry this may be blocked on some tasks running on UI at > start up. And the whole point of this CL is to unblock the operation as much as > possible especially at start up. > > I remember I tried to execute the image_decoder->Start from UI thread and > measure the time spend on each phrase. The time it takes from decoding finished > on uitility process to start execute this function is around 100ms(cant remember > the exact number but it is a large number). So we must wait some tasks to finish > on UI thread before execute this. I will double check with your suggested > approach and get some concrete measurement on real device. > I'd be very surprised if UI is stuck at any moment, but you may be right. I won't stop you if you prefer the locks :-) we just try to avoid them as much as possible in chrome. > On 2013/01/21 08:07:39, Joao da Silva wrote: > > info_it may be invalid at this point, because another thread may have just > > erased it or another iterator from the map. > > > > Locking access to image_info_map_ adds complexity and is very easy to break in > > the future. I'd suggest the following instead: > > > > - Start() posts to the blocking pool directly, to have parallel tasks; > > - OnImageDecoded() can then enter from several threads; just make it post > again > > to UI: > > > > void UserImageLoader::OnImageDecoded(decoder, decoded_image) { > > BrowserThread::PostTask( > > BrowserThread::UI, FROM_HERE, > > base::Bind(&UserImageLoader::ProcessDecodedImage, this, decoder, > > decoded_image)); > > } > > > > And add a new ProcessDecodedImage() method that always runs on UI and does > what > > OnImageDecoded() is doing now. > > > > Same thing for OnDecodeImageFailed. >
This patch should have address everything except the SequencedTaskRunner and TaskRunner issue. Please see comments for detail. Also, I have measured this patch on lumpy and link. On lumpy, wallpaper is loaded around 400ms to 500ms earlier. And I dont see the login hitch (tried a few times). On link, the speed up is not obvious. At most 100ms from my experiment. And if we use a large wallpaper (like the aurora one), we are seeing a white screen first, then user pod, then wallpaper fade in. But this is because that wallpaper is really large(12mb) and it tooks more than 1s decode it in the utility process. With small wallpapers, I dont see the login hitch at all on link. https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:52: base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN); It looks like UtilityProcessHost::Create require a SequencedTaskRunner as parameter. If we passing a TaskRunner to image_decoder, we will have to dynamic_cast TaskRunner to SequencedTaskRunner, which is really bad. I cant think of a good way to solve this. So I kept the SequencedTaskRunner in this patch. Any suggestion is welcome. At the same time, the performance with sequence task runner seems acceptable. I have tested them on lumpy. And with this patch, wallpaper finished loading around 400-500ms earlier than before. On 2013/01/21 16:46:58, Joao da Silva wrote: > On 2013/01/21 16:04:40, bshe wrote: > > pool->GetSequenceToken will return a different token each time. I thought > > sequenced task with different token can execute paralle. But I maybe wrong? > Are > > you suggesting to use GetTaskRunnerWithShutdownBehavior here? > > > > Yes, you're right. Either call GetTaskRunnerWithShutdownBehavior(), or just use > the blocking pool directly (it's a TaskRunner too). > > > On 2013/01/21 08:07:39, Joao da Silva wrote: > > > If this is using a sequenced task runner then it won't execute tasks in > > > parallel. Just post directly to the blocking pool. > > > https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:86: const ImageInfo& image_info = info_it->second; Thanks. I will try to use lock to unblock the task as much as possible. I have modified the lock block. It should be safe now. On 2013/01/21 16:46:58, Joao da Silva wrote: > On 2013/01/21 16:04:40, bshe wrote: > > If post back to UI, I worry this may be blocked on some tasks running on UI at > > start up. And the whole point of this CL is to unblock the operation as much > as > > possible especially at start up. > > > > I remember I tried to execute the image_decoder->Start from UI thread and > > measure the time spend on each phrase. The time it takes from decoding > finished > > on uitility process to start execute this function is around 100ms(cant > remember > > the exact number but it is a large number). So we must wait some tasks to > finish > > on UI thread before execute this. I will double check with your suggested > > approach and get some concrete measurement on real device. > > > > I'd be very surprised if UI is stuck at any moment, but you may be right. I > won't stop you if you prefer the locks :-) we just try to avoid them as much as > possible in chrome. > > > On 2013/01/21 08:07:39, Joao da Silva wrote: > > > info_it may be invalid at this point, because another thread may have just > > > erased it or another iterator from the map. > > > > > > Locking access to image_info_map_ adds complexity and is very easy to break > in > > > the future. I'd suggest the following instead: > > > > > > - Start() posts to the blocking pool directly, to have parallel tasks; > > > - OnImageDecoded() can then enter from several threads; just make it post > > again > > > to UI: > > > > > > void UserImageLoader::OnImageDecoded(decoder, decoded_image) { > > > BrowserThread::PostTask( > > > BrowserThread::UI, FROM_HERE, > > > base::Bind(&UserImageLoader::ProcessDecodedImage, this, decoder, > > > decoded_image)); > > > } > > > > > > And add a new ProcessDecodedImage() method that always runs on UI and does > > what > > > OnImageDecoded() is doing now. > > > > > > Same thing for OnDecodeImageFailed. > > > https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:274: kAccountsPrefShowUserNamesOnSignIn) On 2013/01/21 14:55:24, pastarmovj wrote: > Multi-line conditions needs { } around the body. Done. https://codereview.chromium.org/11968044/diff/13001/chrome/browser/image_deco... File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/image_deco... chrome/browser/image_decoder.cc:40: const base::SequencedWorkerPool::SequenceToken& sequence_token) { Good idea! We only have one Start function now. On 2013/01/21 08:07:39, Joao da Silva wrote: > Why not have a single Start() that gets a TaskRunner? > > The 1st version is invoked as > > base::TaskRunner* task_runner = > BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE); > decoder->Start(task_runner); > > and the 2nd: > > base::TaskRunner* task_runner = > BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(sequence_token); > decoder->Start(task_runner); > > > CalledOnValidThreadOrWorkerThread() then just becomes: > task_runner_->RunsTasksOnCurrentThread() https://codereview.chromium.org/11968044/diff/13001/chrome/browser/image_deco... chrome/browser/image_decoder.cc:45: base::Bind(&ImageDecoder::DecodeImageInSandbox, this, image_data_)); This is an argument of function PostTask, same as BrowserThread::IO. I remember the arguments should be aligned in this case. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Or maybe I understood it wrong? On 2013/01/21 08:07:39, Joao da Silva wrote: > nit: indent (4 spaces) https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (left): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:282: In order to add CrosSettingsObserver in ctor of WallpaperManager, removing this is needed. Otherwise, wallpaperManager is created before CrosSettings. The following code was needed because WallpaperManager was created before PowerManagerClient. It is not the case now. So I removed this function in WallpaperManager and add all observers in the ctor of wallpaper manager.
+owners for new modified files: background for owners: I have changed the signature of ImageDecoder::Start, changes in these files are just add argument. +xiyuan for chrome/browser/ui/app_list/search_builder.cc +davemoore for chrome/browser/profiles/profile_downloader.cc
https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:52: base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN); Actually, the sequence task runner is running parallel according to chrome tracing. Since I am getting a new sequence token each time. These tasks can be ran at the same time. Please see the screenshot I got here: https://code.google.com/p/chromium/issues/detail?id=168297#c17 In the picture: thread 1889 is the task to load a custom user profile image; thread 1909 loads exactly the same image for a different user; and thread 1924 loads a wallpaper. On 2013/01/21 22:53:07, bshe wrote: > It looks like UtilityProcessHost::Create require a SequencedTaskRunner as > parameter. If we passing a TaskRunner to image_decoder, we will have to > dynamic_cast TaskRunner to SequencedTaskRunner, which is really bad. > > I cant think of a good way to solve this. So I kept the SequencedTaskRunner in > this patch. Any suggestion is welcome. > > At the same time, the performance with sequence task runner seems acceptable. I > have tested them on lumpy. And with this patch, wallpaper finished loading > around 400-500ms earlier than before. > > On 2013/01/21 16:46:58, Joao da Silva wrote: > > On 2013/01/21 16:04:40, bshe wrote: > > > pool->GetSequenceToken will return a different token each time. I thought > > > sequenced task with different token can execute paralle. But I maybe wrong? > > Are > > > you suggesting to use GetTaskRunnerWithShutdownBehavior here? > > > > > > > Yes, you're right. Either call GetTaskRunnerWithShutdownBehavior(), or just > use > > the blocking pool directly (it's a TaskRunner too). > > > > > On 2013/01/21 08:07:39, Joao da Silva wrote: > > > > If this is using a sequenced task runner then it won't execute tasks in > > > > parallel. Just post directly to the blocking pool. > > > > > >
Looks mostly good. There's still an unlocked write to image_info_map_ (see, locks are bad :-) ), and the lifetime of WallPaperManager is a bit strange. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:63: task_runner->RunsTasksOnCurrentThread(); DCHECK(task_runner->RunsTasksOnCurrentThread()); https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:70: image_info_map_.insert(std::make_pair(image_decoder.get(), image_info)); Access to image_info_map_ must be locked https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.h (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.h:33: typedef base::SequencedWorkerPool::SequenceToken SequenceToken; This can be moved to the .cc file. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:194: delete WallpaperManager::Get(); This is quite strange. Why not make WallpaperManager a Singleton? (see MediaPlayer::GetInstance() for an example: chrome/browser/chromeos/media/media_player.cc) https://codereview.chromium.org/11968044/diff/26001/chrome/browser/image_deco... File chrome/browser/image_decoder.h (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/image_deco... chrome/browser/image_decoder.h:13: #include "content/public/browser/browser_thread.h" Not needed https://codereview.chromium.org/11968044/diff/26001/chrome/browser/image_deco... chrome/browser/image_decoder.h:47: void Start(scoped_refptr<base::SequencedTaskRunner> task_runner); #include "base/memory/ref_counted.h"
https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.h (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.h:37: // Start reading the image from |filepath| on the file thread. Calls nit: Comment should be updated now. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.h:60: // Method that reads the file on the file thread and starts decoding it in nit: Comment should be updated now. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.h:81: // Accessed only on FILE thread. nit: Comment should be updated now. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:129: ClearObsoleteWallpaperPrefs(); DCHECK that shutdown has been called.
Done. Hoping no more lock issue. :) Thanks for review! https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.cc (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:63: task_runner->RunsTasksOnCurrentThread(); On 2013/01/22 10:04:12, Joao da Silva wrote: > DCHECK(task_runner->RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.cc:70: image_info_map_.insert(std::make_pair(image_decoder.get(), image_info)); Dooh. I just focused on the callback functions. Added lock. On 2013/01/22 10:04:12, Joao da Silva wrote: > Access to image_info_map_ must be locked https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_image_loader.h (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.h:33: typedef base::SequencedWorkerPool::SequenceToken SequenceToken; On 2013/01/22 10:04:12, Joao da Silva wrote: > This can be moved to the .cc file. Done. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.h:37: // Start reading the image from |filepath| on the file thread. Calls On 2013/01/22 11:30:54, Nikita Kostylev wrote: > nit: Comment should be updated now. Done. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.h:60: // Method that reads the file on the file thread and starts decoding it in On 2013/01/22 11:30:54, Nikita Kostylev wrote: > nit: Comment should be updated now. Done. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_image_loader.h:81: // Accessed only on FILE thread. On 2013/01/22 11:30:54, Nikita Kostylev wrote: > nit: Comment should be updated now. Done. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:194: delete WallpaperManager::Get(); Reading comment for singleton here: http://code.google.com/searchframe#OAMlx_jo-ck/src/base/memory/singleton.h&ex... I thought it might be good to avoid create singleton as much as possible. And wallpaper manager can be managed by user manager. But I maybe wrong. I will convert it to a singleton if you think it doesnt make sense. On 2013/01/22 10:04:12, Joao da Silva wrote: > This is quite strange. Why not make WallpaperManager a Singleton? > > (see MediaPlayer::GetInstance() for an example: > chrome/browser/chromeos/media/media_player.cc) https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:129: ClearObsoleteWallpaperPrefs(); On 2013/01/22 11:30:54, Nikita Kostylev wrote: > DCHECK that shutdown has been called. Done. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/image_deco... File chrome/browser/image_decoder.h (right): https://codereview.chromium.org/11968044/diff/26001/chrome/browser/image_deco... chrome/browser/image_decoder.h:13: #include "content/public/browser/browser_thread.h" On 2013/01/22 10:04:12, Joao da Silva wrote: > Not needed Done. https://codereview.chromium.org/11968044/diff/26001/chrome/browser/image_deco... chrome/browser/image_decoder.h:47: void Start(scoped_refptr<base::SequencedTaskRunner> task_runner); On 2013/01/22 10:04:12, Joao da Silva wrote: > #include "base/memory/ref_counted.h" Done.
ui/app_list LGTM
CrosSettings usage seems alright, and I don't think there are threading issues left, so lgtm. I'd prefer not to add the lock and to have better lifetime management of WallpaperManager (in particular regarding its deletion), but I'll leave that to Nikita who owns the code. Thanks for the measurement numbers btw. I'm all for this if it improves performance and the UX :-)
On 2013/01/22 20:40:21, Joao da Silva wrote: > CrosSettings usage seems alright, and I don't think there are threading issues > left, so lgtm. > > I'd prefer not to add the lock and to have better lifetime management of > WallpaperManager (in particular regarding its deletion), but I'll leave that to > Nikita who owns the code. > > Thanks for the measurement numbers btw. I'm all for this if it improves > performance and the UX :-) Please consider my question here: https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... I have not gotten answer on that one and I think it is important to consider that before you commit that change! I am pretty sure it is not critical for you but I would sleep better if I know you have acknowledged that question. :)
https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:670: bool result = CrosSettings::Get()->GetBoolean( On 2013/01/21 14:55:24, pastarmovj wrote: > Without the condition you removed on |PrepareTrustedValues| you can't not be > sure whether the value you get here is actually coming from the trusted policy > store or from a possibly expired cache. If this logic only controls which image > to show as a background it is not critical enough to care but if you make any > more grave decisions than that please consider reinstating the logic you had > here before. Julian, were're fine here with using cached value. That were one of the sources of slowdown comes from. The rest of login screen UI behaves the same: * Uses cached values on boot * Subscribes for updates when they are changed * Never proceeds with a real action like login if values are not trusted Yes, we only decide here which wallpaper to show on boot. We want great boot UX so any slowdown here has to be eliminated. Cases when cached value is different are rare. In this case it is when "show user pods" option is changed. In that case we're fine to show wallpaper of the user who logged in last time on boot and then change that to the default wallpaper (that is shown when on sign in UI is shown).
https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:670: bool result = CrosSettings::Get()->GetBoolean( On 2013/01/23 09:53:34, Nikita Kostylev wrote: > On 2013/01/21 14:55:24, pastarmovj wrote: > > Without the condition you removed on |PrepareTrustedValues| you can't not be > > sure whether the value you get here is actually coming from the trusted policy > > store or from a possibly expired cache. If this logic only controls which > image > > to show as a background it is not critical enough to care but if you make any > > more grave decisions than that please consider reinstating the logic you had > > here before. > > Julian, were're fine here with using cached value. That were one of the sources > of slowdown comes from. > > The rest of login screen UI behaves the same: > * Uses cached values on boot > * Subscribes for updates when they are changed > * Never proceeds with a real action like login if values are not trusted > > Yes, we only decide here which wallpaper to show on boot. > We want great boot UX so any slowdown here has to be eliminated. > > Cases when cached value is different are rare. In this case it is when "show > user pods" option is changed. In that case we're fine to show wallpaper of the > user who logged in last time on boot and then change that to the default > wallpaper (that is shown when on sign in UI is shown). Cool, thanks for explaining! I just wanted to make sure this concern doesn't go unnoticed. My main reason to insist on this was to make sure that the code would handle correctly the case where there is a cached value saying we are showing usernames but the actual policy has changed and then some portions of the UI might see one and another portions different value. It is not so critical it is just important the UI handles that gracefully.
https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/11968044/diff/13001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:670: bool result = CrosSettings::Get()->GetBoolean( Sorry pastarmovj, I missed your comment. Thanks Nikita for explansion. On 2013/01/23 10:05:59, pastarmovj wrote: > On 2013/01/23 09:53:34, Nikita Kostylev wrote: > > On 2013/01/21 14:55:24, pastarmovj wrote: > > > Without the condition you removed on |PrepareTrustedValues| you can't not be > > > sure whether the value you get here is actually coming from the trusted > policy > > > store or from a possibly expired cache. If this logic only controls which > > image > > > to show as a background it is not critical enough to care but if you make > any > > > more grave decisions than that please consider reinstating the logic you had > > > here before. > > > > Julian, were're fine here with using cached value. That were one of the > sources > > of slowdown comes from. > > > > The rest of login screen UI behaves the same: > > * Uses cached values on boot > > * Subscribes for updates when they are changed > > * Never proceeds with a real action like login if values are not trusted > > > > Yes, we only decide here which wallpaper to show on boot. > > We want great boot UX so any slowdown here has to be eliminated. > > > > Cases when cached value is different are rare. In this case it is when "show > > user pods" option is changed. In that case we're fine to show wallpaper of the > > user who logged in last time on boot and then change that to the default > > wallpaper (that is shown when on sign in UI is shown). > > Cool, thanks for explaining! I just wanted to make sure this concern doesn't go > unnoticed. My main reason to insist on this was to make sure that the code would > handle correctly the case where there is a cached value saying we are showing > usernames but the actual policy has changed and then some portions of the UI > might see one and another portions different value. It is not so critical it is > just important the UI handles that gracefully.
lgtm I've also executed several cros trybots
Thanks Nikita. My previous patch failed on some tests. I have uploaded a new patch to fix these tests. PTAL https://codereview.chromium.org/11968044/diff/19019/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11968044/diff/19019/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:194: delete WallpaperManager::Get(); It looks this cause some browser tests failing(DCHECK fail, some browser tests didn't add/remove observers). The life time of wallpaper manager need more careful thought. I am removing it here and added a TODO. https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.h:74: Add this function back because some tests (unit tests) use WallpaperManager instance but do not call Shutdown function. So it is not a good idea to add these observers in ctor.
https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:126: // TODO(bshe): Lifetime of WallpaperManager needs more consideration. Issue #? https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.h:74: On 2013/01/23 16:08:34, bshe wrote: > Add this function back because some tests (unit tests) use WallpaperManager > instance but do not call Shutdown function. So it is not a good idea to add > these observers in ctor. Should they call shutdown instead?
https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.cc:126: // TODO(bshe): Lifetime of WallpaperManager needs more consideration. On 2013/01/23 16:28:17, Nikita Kostylev wrote: > Issue #? Done. https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.h:74: We can explicitly call the shutdown in the tear down of these tests. But I forgot to mention another reason to do this. In unit tests, there are lots of place use wallpaper manager but do not have DBusThreadManager or CrosSettings instance. If we add observers in ctor, we have to add these intances to those tests. So this is probably the easiest in terms of making these tests pass. On 2013/01/23 16:28:17, Nikita Kostylev wrote: > On 2013/01/23 16:08:34, bshe wrote: > > Add this function back because some tests (unit tests) use WallpaperManager > > instance but do not call Shutdown function. So it is not a good idea to add > > these observers in ctor. > > Should they call shutdown instead?
https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.h:74: On 2013/01/23 17:11:45, bshe wrote: > We can explicitly call the shutdown in the tear down of these tests. But I > forgot to mention another reason to do this. > In unit tests, there are lots of place use wallpaper manager but do not have > DBusThreadManager or CrosSettings instance. If we add observers in ctor, we have > to add these intances to those tests. > So this is probably the easiest in terms of making these tests pass. > > On 2013/01/23 16:28:17, Nikita Kostylev wrote: > > On 2013/01/23 16:08:34, bshe wrote: > > > Add this function back because some tests (unit tests) use WallpaperManager > > > instance but do not call Shutdown function. So it is not a good idea to add > > > these observers in ctor. > > > > Should they call shutdown instead? > I think that shutdown should be called in tests, ideally using the same code path as the production code.
https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.h:74: Sorry. Just to be clear, do you want me to call this Shutdown method in any test that use WallpaperManager? I think the shutdown method is called by some tests already but not all. In the case that it is not called, we cant add observers in ctor. Otherwise, we will get size of observer_list != 0 error when program exist. On 2013/01/24 12:03:26, Nikita Kostylev wrote: > On 2013/01/23 17:11:45, bshe wrote: > > We can explicitly call the shutdown in the tear down of these tests. But I > > forgot to mention another reason to do this. > > In unit tests, there are lots of place use wallpaper manager but do not have > > DBusThreadManager or CrosSettings instance. If we add observers in ctor, we > have > > to add these intances to those tests. > > So this is probably the easiest in terms of making these tests pass. > > > > On 2013/01/23 16:28:17, Nikita Kostylev wrote: > > > On 2013/01/23 16:08:34, bshe wrote: > > > > Add this function back because some tests (unit tests) use > WallpaperManager > > > > instance but do not call Shutdown function. So it is not a good idea to > add > > > > these observers in ctor. > > > > > > Should they call shutdown instead? > > > > I think that shutdown should be called in tests, ideally using the same code > path as the production code.
https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.h:74: On 2013/01/24 14:56:54, bshe wrote: > Sorry. Just to be clear, do you want me to call this Shutdown method in any test > that use WallpaperManager? I think the shutdown method is called by some tests > already but not all. In the case that it is not called, we cant add observers in > ctor. Otherwise, we will get size of observer_list != 0 error when program > exist. Also you'll have DCHECK failing because shutdown was not called. My point is that if Shutdown() method is added as a required method for WallpaperManager it has to be always called, including those tests. Ideally you should not add ad-hoc solution but figure out why some tests do not call that method. Those are browser_tests, right?
https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/11968044/diff/18022/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager.h:74: Good point on the DCHECK fail. And yes they are browser_tests. Somehow I am not seeing the dcheck fail on the test results. It looks the wallpaper manager desctructor is not being called somehow? If this is the case, I will address it in issue 171694. Also, the Shutdown is probably no longer a required method. I am guessing by saying required, you mean the DCHECK in desctructor makes Shutdown required. The reason we added the DCHECK is because we previously adding observer in ctor. So we have to make sure Shutdown is called in dtor. But now we are not doing that. Observers is not always added. So the DCHECK is probably wrong? And AddObservers and Shutdown is probably not always needed for all browser tests? In the latest patch, I have updated the DCHECK to make sure if AddObservers is called, we have to call Shutdown when program exist. On 2013/01/24 15:39:00, Nikita Kostylev wrote: > On 2013/01/24 14:56:54, bshe wrote: > > Sorry. Just to be clear, do you want me to call this Shutdown method in any > test > > that use WallpaperManager? I think the shutdown method is called by some tests > > already but not all. In the case that it is not called, we cant add observers > in > > ctor. Otherwise, we will get size of observer_list != 0 error when program > > exist. > > Also you'll have DCHECK failing because shutdown was not called. > > My point is that if Shutdown() method is added as a required method for > WallpaperManager it has to be always called, including those tests. Ideally you > should not add ad-hoc solution but figure out why some tests do not call that > method. Those are browser_tests, right?
ping davemoore@ for owners and +more owners jhawkins: chrome/browser/image_decoder.h chrome/browser/image_decoder.cc chrome/browser/ui/webui/options/manage_profile_handler.cc
My files LGTM. On a related note: I thought we weren't bumping copyright files anymore. (?)
On 2013/01/25 17:17:02, James Hawkins wrote: > My files LGTM. On a related note: I thought we weren't bumping copyright files > anymore. (?) I am not sure. I looked some of the recent landed CLs. It looks we are not consistent with copyright files. Some files have 2013 and some didn't bump it. example: https://chromiumcodereview.appspot.com/11953021 The first file already changed to 2013 and the rest still 2012.
On 2013/01/25 18:59:04, bshe wrote: > On 2013/01/25 17:17:02, James Hawkins wrote: > > My files LGTM. On a related note: I thought we weren't bumping copyright files > > anymore. (?) > > I am not sure. I looked some of the recent landed CLs. It looks we are not > consistent with copyright files. Some files have 2013 and some didn't bump it. > example: https://chromiumcodereview.appspot.com/11953021 > The first file already changed to 2013 and the rest still 2012. --tbr davemoore since the change in the file is really trivial one (update a function call to march the changed function signature). davemoore@ and nikita@ please let me know if you have any future comment. I will definitely address them in a follow up CL if any. Since this is still potentially m25 cl, I am hoping to get this on tot to test it as early as possible. At the same time, 4 lgtms seems good enough to be on TOT. thanks! |
