Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
221873005:
Some cleanup of WallpaperManager. (Closed)
|
Created:
6 years, 8 months ago by Thiemo Nagel Modified:
6 years, 8 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Visibility:
Public. |
DescriptionSome cleanup of WallpaperManager.
* Add const, static and/or private in some places.
* Integrate ProcessCustomWallpaper() into SetCustomWallpaper() to avoid PostTask() and deep copy of wallpaper in case it is not persistent.
* Replace UserImage by gfx::ImageSkia in some places to simplify code and to avoid useless temporary objects.
* Remove unused DAILY User::WallpaperType.
* Remove unused GetOriginalWallpaperPathForUser().
* Replace SetLastSelectedUser() by direct member access.
* Add initialization of WallpaperInfo.date that was missing in one case.
* Rename all ImageSkia method parameters to "image" and all UserImage method parameters to "user_image" for consistency.
* Rename ResizeWallpaper() to ResizeImage().
* Improve some comments.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266026
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Address Biao's comments. #Patch Set 3 : Rebased. #Patch Set 4 : Ascertain thread-safety. #Patch Set 5 : SetCustomWallpaper(): Replace UserImage parameter by gfx::ImageSkia. #Patch Set 6 : Remove unused variable. #Patch Set 7 : Rebased. #Patch Set 8 : Compilation fixes. #
Total comments: 19
Patch Set 9 : Rebase. #Patch Set 10 : Address nits. #Patch Set 11 : Some more refactoring. #Patch Set 12 : Make pass of deep_copy more explicit. #Patch Set 13 : git cl format. #Patch Set 14 : Rebase on master. #Patch Set 15 : Compilation fix. #Messages
Total messages: 27 (0 generated)
Hi Biao/Nikita, over the last weeks, I've accumulated various bits of cleanup for WallpaperManager which I'd like to ask you to take a look at. Thank you! Thiemo
https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user.h (right): https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user.h:110: enum WallpaperType { Could you document the removed enum please? This enum is used for UMA stats and the value shouldn't be reused in the future. A document could reduce the risk of these enum values being reused. https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user.h:111: CUSTOMIZED = 1, // Selected by user. nit: Selected by user or Google pushed wallpaper. https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:697: UserImage(*deep_copy, wallpaper.raw_image()))); the ownership of deep_copy is not passed to the |SaveCustomWallpaper|. It may run into problems as ImageSkia is not thread safe. https://codereview.chromium.org/221873005/diff/60001/chromeos/chromeos_switch... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/221873005/diff/60001/chromeos/chromeos_switch... chromeos/chromeos_switches.cc:137: // Specifies the profile to use once a chromeos user is logged in. This is nit: One space before "This is..."
Hi Biao, thanks a lot for your review! I've addressed your comments as far as I've understood them. Please see my questions in the comments. Thanks again! Thiemo https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user.h (right): https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user.h:110: enum WallpaperType { On 2014/04/02 15:11:10, bshe wrote: > Could you document the removed enum please? This enum is used for UMA stats and > the value shouldn't be reused in the future. A document could reduce the risk of > these enum values being reused. Done. https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user.h:111: CUSTOMIZED = 1, // Selected by user. On 2014/04/02 15:11:10, bshe wrote: > nit: Selected by user or Google pushed wallpaper. How can Google push a wallpaper? Are you referring to the "surprise me" checkbox in the wallpaper picker? https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:697: UserImage(*deep_copy, wallpaper.raw_image()))); On 2014/04/02 15:11:10, bshe wrote: > the ownership of deep_copy is not passed to the |SaveCustomWallpaper|. It may > run into problems as ImageSkia is not thread safe. I don't understand what you're concerned about. Could you please elaborate? In which way is my handling of the deep_copy different from the existing code? To my understanding, ImageSkia is an "empty hull" and only contains pointers to the data, which is possibly shared with other ImageSkia instances. By calling DeepCopy(), an new ImageSkia is generated that has pointers to a fresh set of image data. This is handed over to the worker thread. How can that cause troubles with thread safety? And wallpaper.raw_image() is copied inside the UserImage() constructor, thus I don't see a problem there, either. But maybe I'm overlooking something? Please explain! https://codereview.chromium.org/221873005/diff/60001/chromeos/chromeos_switch... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/221873005/diff/60001/chromeos/chromeos_switch... chromeos/chromeos_switches.cc:137: // Specifies the profile to use once a chromeos user is logged in. This is On 2014/04/02 15:11:10, bshe wrote: > nit: One space before "This is..." Done.
+oshima san. I am not an expert of ImageSkia and thread safety associated with it. Oshima may have more insights. https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:697: UserImage(*deep_copy, wallpaper.raw_image()))); +oshima I remember it could cause problem when try to modify the ref count of the image data on different threads. Base::Pass will pass the ownership of the deep copy object to worker thread. So when deep_copy is out of scope, we wont try to decrease ref count. But with the current approach, we will try to decrease the ref count. And we essentially try to Add/Remove ref counts on two different threads on a non thread safe object. https://chromiumcodereview.appspot.com/11366143 should have more details. On 2014/04/02 15:39:56, Thiemo Nagel wrote: > On 2014/04/02 15:11:10, bshe wrote: > > the ownership of deep_copy is not passed to the |SaveCustomWallpaper|. It may > > run into problems as ImageSkia is not thread safe. > > I don't understand what you're concerned about. Could you please elaborate? In > which way is my handling of the deep_copy different from the existing code? > > To my understanding, ImageSkia is an "empty hull" and only contains pointers to > the data, which is possibly shared with other ImageSkia instances. By calling > DeepCopy(), an new ImageSkia is generated that has pointers to a fresh set of > image data. This is handed over to the worker thread. How can that cause > troubles with thread safety? > > And wallpaper.raw_image() is copied inside the UserImage() constructor, thus I > don't see a problem there, either. > > But maybe I'm overlooking something? Please explain!
I think I've addressed the thread-safety issue now. Thank you for catching this! Could you please take another look? bshe: About your nit concerning Google-pushed wallpaper: How can Google push a wallpaper? Are you referring to the "surprise me" checkbox in the wallpaper picker? Thank you and best regards! Thiemo https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/221873005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wallpaper_manager.cc:697: UserImage(*deep_copy, wallpaper.raw_image()))); On 2014/04/02 16:32:19, bshe wrote: > I remember it could cause problem when try to modify the ref count of the image > data on different threads. Base::Pass will pass the ownership of the deep copy > object to worker thread. So when deep_copy is out of scope, we wont try to > decrease ref count. But with the current approach, we will try to decrease the > ref count. And we essentially try to Add/Remove ref counts on two different > threads on a non thread safe object. Thanks a lot for the explanation! I've fixed this by following the example of wallpaper_api.cc.
Hi Biao and Nikita, may I ping you again about this? I've addressed all the comments that were raised so far and tests have passed, except for bots that always fail. Thank you! Thiemo
lgtm with nits https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/wallpaper_api.cc (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_api.cc:122: const gfx::ImageSkia& wallpaper) { nit: some methods use |wallpaper| others use |image| for the param name. How about using just one for consistency. https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:575: const base::FilePath& thumbnail_path, scoped_ptr<gfx::ImageSkia> image) { nit: some methods use |wallpaper| others use |image| for the param name. How about using just one for consistency. https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/user.h (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/user.h:110: // Enum values 0 and 3 have been removed. Do not re-use them! nit: How about using this syntax: This way it is more clear what these indices were used for. /* DAILY = 0 */ // Removed. Do not re-use the id! /* UNKNOWN = 3 */ // Removed. Do not re-use the id! https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (left): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:853: void WallpaperManager::SetLastSelectedUser( I guess this was previously used for tests, maybe not. Not anymore. https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1040: void WallpaperManager::DeleteAllExcept(const base::FilePath& path) { nit: This function seems to be used only in this file. How about moving it to unnamed namespace instead? https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1053: void WallpaperManager::DeleteWallpaperInList( nit: This function seems to be used only in this file. How about moving it to unnamed namespace instead? https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1104: void WallpaperManager::EnsureCustomWallpaperDirectories( nit: This function seems to be used only in this file. How about moving it to unnamed namespace instead? https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:424: // because that's the callback interface provided by UserImageLoader.) nit: Delete one of the new lines? Seems to be duplicate.
On 2014/04/18 09:28:19, Nikita Kostylev wrote: > lgtm with nits > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > File chrome/browser/chromeos/extensions/wallpaper_api.cc (right): > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/wallpaper_api.cc:122: const gfx::ImageSkia& > wallpaper) { > nit: some methods use |wallpaper| others use |image| for the param name. How > about using just one for consistency. > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/wallpaper_private_api.cc:575: const > base::FilePath& thumbnail_path, scoped_ptr<gfx::ImageSkia> image) { > nit: some methods use |wallpaper| others use |image| for the param name. How > about using just one for consistency. > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/user.h (right): > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/login/user.h:110: // Enum values 0 and 3 have been > removed. Do not re-use them! > nit: How about using this syntax: > This way it is more clear what these indices were used for. > > /* DAILY = 0 */ // Removed. Do not re-use the id! > > /* UNKNOWN = 3 */ // Removed. Do not re-use the id! > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/wallpaper_manager.cc (left): > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wallpaper_manager.cc:853: void > WallpaperManager::SetLastSelectedUser( > I guess this was previously used for tests, maybe not. > Not anymore. > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/wallpaper_manager.cc (right): > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wallpaper_manager.cc:1040: void > WallpaperManager::DeleteAllExcept(const base::FilePath& path) { > nit: This function seems to be used only in this file. > How about moving it to unnamed namespace instead? > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wallpaper_manager.cc:1053: void > WallpaperManager::DeleteWallpaperInList( > nit: This function seems to be used only in this file. > How about moving it to unnamed namespace instead? > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wallpaper_manager.cc:1104: void > WallpaperManager::EnsureCustomWallpaperDirectories( > nit: This function seems to be used only in this file. > How about moving it to unnamed namespace instead? > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/wallpaper_manager.h (right): > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wallpaper_manager.h:424: // because that's the > callback interface provided by UserImageLoader.) > nit: Delete one of the new lines? Seems to be duplicate. lgtm
lgtm with a nit https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:732: base::Passed(&deep_copy))); I think deep_copy.Pass() is better now.
Hi Biao, Mitsuru, Nikita, thanks a lot for your comments which I've addressed with the exception of deep_copy.Pass(), see comment for explanation. Are you happy with the renaming of method parameters, Nikita? I've also renamed ResizeWallpaper() to ResizeImage() to make clearer that it doesn't change the state of WallpaperManager. Prompted by your suggestion to move some methods into the private namespace, I've also moved SaveWallpaperInternal() there. And I've made ResizeImage() and ResizeAndSaveWallpaper() static to emphasize that they don't access member variables which required changing DCHECK(BrowserThread::GetBlockingPool()->IsRunningSequenceOnCurrentThread(sequence_token_)) to DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)) which I believe to be unproblematic. I apologize that this CL keeps growing and growing, I'll try not to increase it any further. Are you still ok with the CL or are there objections or nits? Thank you! Thiemo https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/wallpaper_api.cc (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_api.cc:122: const gfx::ImageSkia& wallpaper) { On 2014/04/18 09:28:19, Nikita Kostylev wrote: > nit: some methods use |wallpaper| others use |image| for the param name. How > about using just one for consistency. Done. https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:575: const base::FilePath& thumbnail_path, scoped_ptr<gfx::ImageSkia> image) { On 2014/04/18 09:28:19, Nikita Kostylev wrote: > nit: some methods use |wallpaper| others use |image| for the param name. How > about using just one for consistency. Done. https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/user.h (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/user.h:110: // Enum values 0 and 3 have been removed. Do not re-use them! On 2014/04/18 09:28:19, Nikita Kostylev wrote: > nit: How about using this syntax: > This way it is more clear what these indices were used for. > > /* DAILY = 0 */ // Removed. Do not re-use the id! > > /* UNKNOWN = 3 */ // Removed. Do not re-use the id! Done. (I had modeled the syntax after that of the OAuthTokenStatus enum, but I don't mind either way.) https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:732: base::Passed(&deep_copy))); On 2014/04/22 00:13:47, oshima wrote: > I think deep_copy.Pass() is better now. I have tried that before. Using deep_copy.Pass() results in a screenful of template errors, it seems that for some reason a const qualifier is added that shouldn't be there. I didn't get to the bottom of it, and followed the example of the surrounding code by using base::Passed(). https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1040: void WallpaperManager::DeleteAllExcept(const base::FilePath& path) { On 2014/04/18 09:28:19, Nikita Kostylev wrote: > nit: This function seems to be used only in this file. > How about moving it to unnamed namespace instead? Done. https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1053: void WallpaperManager::DeleteWallpaperInList( On 2014/04/18 09:28:19, Nikita Kostylev wrote: > nit: This function seems to be used only in this file. > How about moving it to unnamed namespace instead? Done. https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:1104: void WallpaperManager::EnsureCustomWallpaperDirectories( On 2014/04/18 09:28:19, Nikita Kostylev wrote: > nit: This function seems to be used only in this file. > How about moving it to unnamed namespace instead? Done. https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.h (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.h:424: // because that's the callback interface provided by UserImageLoader.) On 2014/04/18 09:28:19, Nikita Kostylev wrote: > nit: Delete one of the new lines? Seems to be duplicate. Thanks! Fixed.
https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:732: base::Passed(&deep_copy))); On 2014/04/23 17:14:06, Thiemo Nagel wrote: > On 2014/04/22 00:13:47, oshima wrote: > > I think deep_copy.Pass() is better now. > > I have tried that before. Using deep_copy.Pass() results in a screenful of > template errors, it seems that for some reason a const qualifier is added that > shouldn't be there. I didn't get to the bottom of it, and followed the example > of the surrounding code by using base::Passed(). Didn't you pass &deep_copy.Pass() instead of deep_copy.Pass()?
https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:732: base::Passed(&deep_copy))); > Didn't you pass &deep_copy.Pass() instead of deep_copy.Pass()? Neither works. In the case of deep_copy.Pass(), this is what I get: In file included from ../../base/bind.h:13:0, from ../../base/timer/timer.h:54, from ../../ui/display/chromeos/display_configurator.h:17, from ../../ash/display/display_manager.h:20, from ../../ash/display/display_controller.h:12, from ../../ash/desktop_background/desktop_background_controller.h:9, from ../../chrome/browser/chromeos/login/wallpaper_manager.h:12, from ../../chrome/browser/chromeos/login/wallpaper_manager.cc:5: ../../base/bind_internal.h: In constructor 'base::internal::BindState<Runnable, RunType, void(P1, P2, P3, P4, P5)>::BindState(const Runnable&, const P1&, const P2&, const P3&, const P4&, const P5&) [with Runnable = base::internal::RunnableAdapter<void (chromeos::WallpaperManager::*)(const std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const>, RunType = void(const chromeos::WallpaperManager*, const std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>), P1 = base::internal::UnretainedWrapper<chromeos::WallpaperManager>, P2 = std::basic_string<char>, P3 = base::FilePath, P4 = ash::WallpaperLayout, P5 = scoped_ptr<gfx::ImageSkia>]': ../../base/bind.h:354:73: instantiated from 'base::Callback<typename base::internal::BindState<typename base::internal::FunctorTraits<Functor>::RunnableType, typename base::internal::FunctorTraits<Functor>::RunType, void(typename base::internal::CallbackParamTraits<A1>::StorageType, typename base::internal::CallbackParamTraits<A2>::StorageType, typename base::internal::CallbackParamTraits<A3>::StorageType, typename base::internal::CallbackParamTraits<A4>::StorageType, typename base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType> base::Bind(Functor, const P1&, const P2&, const P3&, const P4&, const P5&) [with Functor = void (chromeos::WallpaperManager::*)(const std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const, P1 = base::internal::UnretainedWrapper<chromeos::WallpaperManager>, P2 = std::basic_string<char>, P3 = base::FilePath, P4 = ash::WallpaperLayout, P5 = scoped_ptr<gfx::ImageSkia>, typename base::internal::BindState<typename base::internal::FunctorTraits<Functor>::RunnableType, typename base::internal::FunctorTraits<Functor>::RunType, void(typename base::internal::CallbackParamTraits<A1>::StorageType, typename base::internal::CallbackParamTraits<A2>::StorageType, typename base::internal::CallbackParamTraits<A3>::StorageType, typename base::internal::CallbackParamTraits<A4>::StorageType, typename base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType = void()]' ../../chrome/browser/chromeos/login/wallpaper_manager.cc:796:36: instantiated from here ../../base/bind_internal.h:2686:15: error: passing 'const scoped_ptr<gfx::ImageSkia>' as 'this' argument of 'scoped_ptr<T, D>::operator scoped_ptr<T, D>::RValue() [with T = gfx::ImageSkia, D = base::DefaultDeleter<gfx::ImageSkia>]' discards qualifiers [-fpermissive] ../../base/bind_internal.h: In static member function 'static void base::internal::InvokeHelper<false, void, Runnable, void(A1, A2, A3, A4, A5)>::MakeItSo(Runnable, A1, A2, A3, A4, A5) [with Runnable = base::internal::RunnableAdapter<void (chromeos::WallpaperManager::*)(const std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const>, A1 = chromeos::WallpaperManager*, A2 = const std::basic_string<char>&, A3 = const base::FilePath&, A4 = const ash::WallpaperLayout&, A5 = const scoped_ptr<gfx::ImageSkia>&]': ../../base/bind_internal.h:1811:60: instantiated from 'static R base::internal::Invoker<5, StorageType, R(X1, X2, X3, X4, X5)>::Run(base::internal::BindStateBase*) [with StorageType = base::internal::BindState<base::internal::RunnableAdapter<void (chromeos::WallpaperManager::*)(const std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const>, void(const chromeos::WallpaperManager*, const std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>), void(base::internal::UnretainedWrapper<chromeos::WallpaperManager>, std::basic_string<char>, base::FilePath, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)>, R = void, X1 = const chromeos::WallpaperManager*, X2 = const std::basic_string<char>&, X3 = const base::FilePath&, X4 = ash::WallpaperLayout, X5 = scoped_ptr<gfx::ImageSkia>]' ../../base/callback.h:389:28: instantiated from 'base::Callback<R()>::Callback(base::internal::BindState<Runnable, BindRunType, BoundArgsType>*) [with Runnable = base::internal::RunnableAdapter<void (chromeos::WallpaperManager::*)(const std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const>, BindRunType = void(const chromeos::WallpaperManager*, const std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>), BoundArgsType = void(base::internal::UnretainedWrapper<chromeos::WallpaperManager>, std::basic_string<char>, base::FilePath, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>), R = void]' ../../base/bind.h:354:73: instantiated from 'base::Callback<typename base::internal::BindState<typename base::internal::FunctorTraits<Functor>::RunnableType, typename base::internal::FunctorTraits<Functor>::RunType, void(typename base::internal::CallbackParamTraits<A1>::StorageType, typename base::internal::CallbackParamTraits<A2>::StorageType, typename base::internal::CallbackParamTraits<A3>::StorageType, typename base::internal::CallbackParamTraits<A4>::StorageType, typename base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType> base::Bind(Functor, const P1&, const P2&, const P3&, const P4&, const P5&) [with Functor = void (chromeos::WallpaperManager::*)(const std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const, P1 = base::internal::UnretainedWrapper<chromeos::WallpaperManager>, P2 = std::basic_string<char>, P3 = base::FilePath, P4 = ash::WallpaperLayout, P5 = scoped_ptr<gfx::ImageSkia>, typename base::internal::BindState<typename base::internal::FunctorTraits<Functor>::RunnableType, typename base::internal::FunctorTraits<Functor>::RunType, void(typename base::internal::CallbackParamTraits<A1>::StorageType, typename base::internal::CallbackParamTraits<A2>::StorageType, typename base::internal::CallbackParamTraits<A3>::StorageType, typename base::internal::CallbackParamTraits<A4>::StorageType, typename base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType = void()]' ../../chrome/browser/chromeos/login/wallpaper_manager.cc:796:36: instantiated from here ../../base/bind_internal.h:991:5: error: passing 'base::enable_if<true, const scoped_ptr<gfx::ImageSkia> >::type {aka const scoped_ptr<gfx::ImageSkia>}' as 'this' argument of 'scoped_ptr<T, D>::operator scoped_ptr<T, D>::RValue() [with T = gfx::ImageSkia, D = base::DefaultDeleter<gfx::ImageSkia>]' discards qualifiers [-fpermissive] ../../base/bind_internal.h:398:5: error: initializing argument 5 of 'R base::internal::RunnableAdapter<R (T::*)(A1, A2, A3, A4)const>::Run(const T*, typename base::internal::CallbackParamTraits<A2>::ForwardType, typename base::internal::CallbackParamTraits<A3>::ForwardType, typename base::internal::CallbackParamTraits<A4>::ForwardType, typename base::internal::CallbackParamTraits<A5>::ForwardType) [with R = void, T = chromeos::WallpaperManager, A1 = const std::basic_string<char>&, A2 = const base::FilePath&, A3 = ash::WallpaperLayout, A4 = scoped_ptr<gfx::ImageSkia>, typename base::internal::CallbackParamTraits<A2>::ForwardType = const std::basic_string<char>&, typename base::internal::CallbackParamTraits<A3>::ForwardType = const base::FilePath&, typename base::internal::CallbackParamTraits<A4>::ForwardType = const ash::WallpaperLayout&, typename base::internal::CallbackParamTraits<A5>::ForwardType = scoped_ptr<gfx::ImageSkia>]'
On 2014/04/23 17:44:37, Thiemo Nagel wrote: > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/wallpaper_manager.cc (right): > > https://codereview.chromium.org/221873005/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wallpaper_manager.cc:732: > base::Passed(&deep_copy))); > > Didn't you pass &deep_copy.Pass() instead of deep_copy.Pass()? > > Neither works. In the case of deep_copy.Pass(), this is what I get: > > In file included from ../../base/bind.h:13:0, > from ../../base/timer/timer.h:54, > from ../../ui/display/chromeos/display_configurator.h:17, > from ../../ash/display/display_manager.h:20, > from ../../ash/display/display_controller.h:12, > from > ../../ash/desktop_background/desktop_background_controller.h:9, > from > ../../chrome/browser/chromeos/login/wallpaper_manager.h:12, > from > ../../chrome/browser/chromeos/login/wallpaper_manager.cc:5: > ../../base/bind_internal.h: In constructor 'base::internal::BindState<Runnable, > RunType, void(P1, P2, P3, P4, P5)>::BindState(const Runnable&, const P1&, const > P2&, const P3&, const P4&, const P5&) [with Runnable = > base::internal::RunnableAdapter<void (chromeos::WallpaperManager::*)(const > std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, > scoped_ptr<gfx::ImageSkia>)const>, RunType = void(const > chromeos::WallpaperManager*, const std::basic_string<char>&, const > base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>), P1 = > base::internal::UnretainedWrapper<chromeos::WallpaperManager>, P2 = > std::basic_string<char>, P3 = base::FilePath, P4 = ash::WallpaperLayout, P5 = > scoped_ptr<gfx::ImageSkia>]': > ../../base/bind.h:354:73: instantiated from 'base::Callback<typename > base::internal::BindState<typename > base::internal::FunctorTraits<Functor>::RunnableType, typename > base::internal::FunctorTraits<Functor>::RunType, void(typename > base::internal::CallbackParamTraits<A1>::StorageType, typename > base::internal::CallbackParamTraits<A2>::StorageType, typename > base::internal::CallbackParamTraits<A3>::StorageType, typename > base::internal::CallbackParamTraits<A4>::StorageType, typename > base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType> > base::Bind(Functor, const P1&, const P2&, const P3&, const P4&, const P5&) [with > Functor = void (chromeos::WallpaperManager::*)(const std::basic_string<char>&, > const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const, > P1 = base::internal::UnretainedWrapper<chromeos::WallpaperManager>, P2 = > std::basic_string<char>, P3 = base::FilePath, P4 = ash::WallpaperLayout, P5 = > scoped_ptr<gfx::ImageSkia>, typename base::internal::BindState<typename > base::internal::FunctorTraits<Functor>::RunnableType, typename > base::internal::FunctorTraits<Functor>::RunType, void(typename > base::internal::CallbackParamTraits<A1>::StorageType, typename > base::internal::CallbackParamTraits<A2>::StorageType, typename > base::internal::CallbackParamTraits<A3>::StorageType, typename > base::internal::CallbackParamTraits<A4>::StorageType, typename > base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType = > void()]' > ../../chrome/browser/chromeos/login/wallpaper_manager.cc:796:36: instantiated > from here > ../../base/bind_internal.h:2686:15: error: passing 'const > scoped_ptr<gfx::ImageSkia>' as 'this' argument of 'scoped_ptr<T, D>::operator > scoped_ptr<T, D>::RValue() [with T = gfx::ImageSkia, D = > base::DefaultDeleter<gfx::ImageSkia>]' discards qualifiers [-fpermissive] > ../../base/bind_internal.h: In static member function 'static void > base::internal::InvokeHelper<false, void, Runnable, void(A1, A2, A3, A4, > A5)>::MakeItSo(Runnable, A1, A2, A3, A4, A5) [with Runnable = > base::internal::RunnableAdapter<void (chromeos::WallpaperManager::*)(const > std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, > scoped_ptr<gfx::ImageSkia>)const>, A1 = chromeos::WallpaperManager*, A2 = const > std::basic_string<char>&, A3 = const base::FilePath&, A4 = const > ash::WallpaperLayout&, A5 = const scoped_ptr<gfx::ImageSkia>&]': > ../../base/bind_internal.h:1811:60: instantiated from 'static R > base::internal::Invoker<5, StorageType, R(X1, X2, X3, X4, > X5)>::Run(base::internal::BindStateBase*) [with StorageType = > base::internal::BindState<base::internal::RunnableAdapter<void > (chromeos::WallpaperManager::*)(const std::basic_string<char>&, const > base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const>, > void(const chromeos::WallpaperManager*, const std::basic_string<char>&, const > base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>), > void(base::internal::UnretainedWrapper<chromeos::WallpaperManager>, > std::basic_string<char>, base::FilePath, ash::WallpaperLayout, > scoped_ptr<gfx::ImageSkia>)>, R = void, X1 = const chromeos::WallpaperManager*, > X2 = const std::basic_string<char>&, X3 = const base::FilePath&, X4 = > ash::WallpaperLayout, X5 = scoped_ptr<gfx::ImageSkia>]' > ../../base/callback.h:389:28: instantiated from > 'base::Callback<R()>::Callback(base::internal::BindState<Runnable, BindRunType, > BoundArgsType>*) [with Runnable = base::internal::RunnableAdapter<void > (chromeos::WallpaperManager::*)(const std::basic_string<char>&, const > base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const>, > BindRunType = void(const chromeos::WallpaperManager*, const > std::basic_string<char>&, const base::FilePath&, ash::WallpaperLayout, > scoped_ptr<gfx::ImageSkia>), BoundArgsType = > void(base::internal::UnretainedWrapper<chromeos::WallpaperManager>, > std::basic_string<char>, base::FilePath, ash::WallpaperLayout, > scoped_ptr<gfx::ImageSkia>), R = void]' > ../../base/bind.h:354:73: instantiated from 'base::Callback<typename > base::internal::BindState<typename > base::internal::FunctorTraits<Functor>::RunnableType, typename > base::internal::FunctorTraits<Functor>::RunType, void(typename > base::internal::CallbackParamTraits<A1>::StorageType, typename > base::internal::CallbackParamTraits<A2>::StorageType, typename > base::internal::CallbackParamTraits<A3>::StorageType, typename > base::internal::CallbackParamTraits<A4>::StorageType, typename > base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType> > base::Bind(Functor, const P1&, const P2&, const P3&, const P4&, const P5&) [with > Functor = void (chromeos::WallpaperManager::*)(const std::basic_string<char>&, > const base::FilePath&, ash::WallpaperLayout, scoped_ptr<gfx::ImageSkia>)const, > P1 = base::internal::UnretainedWrapper<chromeos::WallpaperManager>, P2 = > std::basic_string<char>, P3 = base::FilePath, P4 = ash::WallpaperLayout, P5 = > scoped_ptr<gfx::ImageSkia>, typename base::internal::BindState<typename > base::internal::FunctorTraits<Functor>::RunnableType, typename > base::internal::FunctorTraits<Functor>::RunType, void(typename > base::internal::CallbackParamTraits<A1>::StorageType, typename > base::internal::CallbackParamTraits<A2>::StorageType, typename > base::internal::CallbackParamTraits<A3>::StorageType, typename > base::internal::CallbackParamTraits<A4>::StorageType, typename > base::internal::CallbackParamTraits<A5>::StorageType)>::UnboundRunType = > void()]' > ../../chrome/browser/chromeos/login/wallpaper_manager.cc:796:36: instantiated > from here > ../../base/bind_internal.h:991:5: error: passing 'base::enable_if<true, const > scoped_ptr<gfx::ImageSkia> >::type {aka const scoped_ptr<gfx::ImageSkia>}' as > 'this' argument of 'scoped_ptr<T, D>::operator scoped_ptr<T, D>::RValue() [with > T = gfx::ImageSkia, D = base::DefaultDeleter<gfx::ImageSkia>]' discards > qualifiers [-fpermissive] > ../../base/bind_internal.h:398:5: error: initializing argument 5 of 'R > base::internal::RunnableAdapter<R (T::*)(A1, A2, A3, A4)const>::Run(const T*, > typename base::internal::CallbackParamTraits<A2>::ForwardType, typename > base::internal::CallbackParamTraits<A3>::ForwardType, typename > base::internal::CallbackParamTraits<A4>::ForwardType, typename > base::internal::CallbackParamTraits<A5>::ForwardType) [with R = void, T = > chromeos::WallpaperManager, A1 = const std::basic_string<char>&, A2 = const > base::FilePath&, A3 = ash::WallpaperLayout, A4 = scoped_ptr<gfx::ImageSkia>, > typename base::internal::CallbackParamTraits<A2>::ForwardType = const > std::basic_string<char>&, typename > base::internal::CallbackParamTraits<A3>::ForwardType = const base::FilePath&, > typename base::internal::CallbackParamTraits<A4>::ForwardType = const > ash::WallpaperLayout&, typename > base::internal::CallbackParamTraits<A5>::ForwardType = > scoped_ptr<gfx::ImageSkia>]' Maybe I wasn't clear. What I meant was base::Passed(deep_copy.Pass()) and it works for me.
> Maybe I wasn't clear. What I meant was > > base::Passed(deep_copy.Pass()) > > and it works for me. Oh, sorry, I didn't understand what you were aiming to do. Yes, that works and I've applied it in all places where deep_copy is used.
lgtm
lgtm
On 2014/04/24 13:56:11, oshima wrote: > lgtm lgtm
On 2014/04/24 16:28:41, bshe wrote: > On 2014/04/24 13:56:11, oshima wrote: > > lgtm > > lgtm Thank you all! I'll try to commit, but CQ isn't in a good mood today.
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tnagel@chromium.org/221873005/330001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/chromeos/login/wallpaper_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/chromeos/login/wallpaper_manager.cc
Hunk #1 succeeded at 35 (offset 1 line).
Hunk #2 succeeded at 83 (offset 1 line).
Hunk #3 succeeded at 146 with fuzz 1 (offset 25 lines).
Hunk #4 succeeded at 330 (offset 96 lines).
Hunk #5 succeeded at 351 (offset 96 lines).
Hunk #6 succeeded at 574 (offset 96 lines).
Hunk #7 succeeded at 582 (offset 96 lines).
Hunk #8 succeeded at 687 (offset 105 lines).
Hunk #9 succeeded at 726 (offset 105 lines).
Hunk #10 succeeded at 749 (offset 105 lines).
Hunk #11 succeeded at 763 (offset 105 lines).
Hunk #12 succeeded at 827 (offset 105 lines).
Hunk #13 succeeded at 838 (offset 105 lines).
Hunk #14 succeeded at 847 (offset 105 lines).
Hunk #15 succeeded at 865 (offset 105 lines).
Hunk #16 succeeded at 873 (offset 105 lines).
Hunk #17 succeeded at 880 (offset 105 lines).
Hunk #18 succeeded at 911 (offset 105 lines).
Hunk #19 succeeded at 1010 (offset 105 lines).
Hunk #20 succeeded at 1039 (offset 105 lines).
Hunk #21 succeeded at 1085 (offset 105 lines).
Hunk #22 succeeded at 1097 (offset 105 lines).
Hunk #23 succeeded at 1151 (offset 106 lines).
Hunk #24 succeeded at 1216 (offset 106 lines).
Hunk #25 succeeded at 1247 (offset 106 lines).
Hunk #26 succeeded at 1481 (offset 106 lines).
Hunk #27 succeeded at 1505 (offset 106 lines).
Hunk #28 succeeded at 1532 (offset 106 lines).
Hunk #29 succeeded at 1561 (offset 106 lines).
Hunk #30 FAILED at 1543.
1 out of 30 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/login/wallpaper_manager.cc.rej
Patch: chrome/browser/chromeos/login/wallpaper_manager.cc
Index: chrome/browser/chromeos/login/wallpaper_manager.cc
diff --git a/chrome/browser/chromeos/login/wallpaper_manager.cc
b/chrome/browser/chromeos/login/wallpaper_manager.cc
index
751bcc34c3f7e0afcdeb718f95279660815b5190..eedaa8b2a7c18426f0c01afa8dd69480af3ed1ee
100644
--- a/chrome/browser/chromeos/login/wallpaper_manager.cc
+++ b/chrome/browser/chromeos/login/wallpaper_manager.cc
@@ -34,6 +34,7 @@
#include "chrome/browser/chromeos/login/login_display_host_impl.h"
#include "chrome/browser/chromeos/login/startup_utils.h"
#include "chrome/browser/chromeos/login/user.h"
+#include "chrome/browser/chromeos/login/user_image.h"
#include "chrome/browser/chromeos/login/user_manager.h"
#include "chrome/browser/chromeos/login/wizard_controller.h"
#include "chrome/browser/chromeos/settings/cros_settings.h"
@@ -81,9 +82,6 @@ const char kNewWallpaperLayoutNodeName[] = "layout";
const char kNewWallpaperFileNodeName[] = "file";
const char kNewWallpaperTypeNodeName[] = "type";
-// File path suffix of the original custom wallpaper.
-const char kOriginalCustomWallpaperSuffix[] = "_wallpaper";
-
// Maximum number of wallpapers cached by CacheUsersWallpapers().
const int kMaxWallpapersToCache = 3;
@@ -123,6 +121,63 @@ bool MoveCustomWallpaperDirectory(const char* sub_dir,
return false;
}
+// Deletes everything else except |path| in the same directory.
+void DeleteAllExcept(const base::FilePath& path) {
+ base::FilePath dir = path.DirName();
+ if (base::DirectoryExists(dir)) {
+ base::FileEnumerator files(dir, false, base::FileEnumerator::FILES);
+ for (base::FilePath current = files.Next(); !current.empty();
+ current = files.Next()) {
+ if (current != path)
+ base::DeleteFile(current, false);
+ }
+ }
+}
+
+// Deletes a list of wallpaper files in |file_list|.
+void DeleteWallpaperInList(const std::vector<base::FilePath>& file_list) {
+ for (std::vector<base::FilePath>::const_iterator it = file_list.begin();
+ it != file_list.end(); ++it) {
+ base::FilePath path = *it;
+ // Some users may still have legacy wallpapers with png extension. We need
+ // to delete these wallpapers too.
+ if (!base::DeleteFile(path, true) &&
+ !base::DeleteFile(path.AddExtension(".png"), false)) {
+ LOG(ERROR) << "Failed to remove user wallpaper at " << path.value();
+ }
+ }
+}
+
+// Creates all new custom wallpaper directories for |user_id_hash| if not
exist.
+void EnsureCustomWallpaperDirectories(const std::string& user_id_hash) {
+ base::FilePath dir;
+ dir = GetCustomWallpaperDir(kSmallWallpaperSubDir);
+ dir = dir.Append(user_id_hash);
+ if (!base::PathExists(dir))
+ base::CreateDirectory(dir);
+ dir = GetCustomWallpaperDir(kLargeWallpaperSubDir);
+ dir = dir.Append(user_id_hash);
+ if (!base::PathExists(dir))
+ base::CreateDirectory(dir);
+ dir = GetCustomWallpaperDir(kOriginalWallpaperSubDir);
+ dir = dir.Append(user_id_hash);
+ if (!base::PathExists(dir))
+ base::CreateDirectory(dir);
+ dir = GetCustomWallpaperDir(kThumbnailWallpaperSubDir);
+ dir = dir.Append(user_id_hash);
+ if (!base::PathExists(dir))
+ base::CreateDirectory(dir);
+}
+
+// Saves wallpaper image raw |data| to |path| (absolute path) in file system.
+// Returns true on success.
+bool SaveWallpaperInternal(const base::FilePath& path,
+ const char* data,
+ int size) {
+ int written_bytes = base::WriteFile(path, data, size);
+ return written_bytes == size;
+}
+
} // namespace
const char kWallpaperSequenceTokenName[] = "wallpaper-sequence";
@@ -179,9 +234,9 @@ WallpaperManager::PendingWallpaper::PendingWallpaper(
WallpaperManager::PendingWallpaper::~PendingWallpaper() {}
void WallpaperManager::PendingWallpaper::ResetSetWallpaperImage(
- const gfx::ImageSkia& user_wallpaper,
+ const gfx::ImageSkia& image,
const WallpaperInfo& info) {
- SetMode(user_wallpaper, info, base::FilePath(), false);
+ SetMode(image, info, base::FilePath(), false);
}
void WallpaperManager::PendingWallpaper::ResetLoadWallpaper(
@@ -200,11 +255,11 @@ void
WallpaperManager::PendingWallpaper::ResetSetDefaultWallpaper() {
}
void WallpaperManager::PendingWallpaper::SetMode(
- const gfx::ImageSkia& user_wallpaper,
+ const gfx::ImageSkia& image,
const WallpaperInfo& info,
const base::FilePath& wallpaper_path,
const bool is_default) {
- user_wallpaper_ = user_wallpaper;
+ user_wallpaper_ = image;
info_ = info;
wallpaper_path_ = wallpaper_path;
default_ = is_default;
@@ -423,14 +478,6 @@ base::FilePath WallpaperManager::GetCustomWallpaperPath(
return custom_wallpaper_path.Append(user_id_hash).Append(file);
}
-base::FilePath WallpaperManager::GetOriginalWallpaperPathForUser(
- const std::string& user_id) {
- std::string filename = user_id + kOriginalCustomWallpaperSuffix;
- base::FilePath user_data_dir;
- PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
- return user_data_dir.AppendASCII(filename);
-}
-
bool WallpaperManager::GetLoggedInUserWallpaperInfo(WallpaperInfo* info) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -439,6 +486,8 @@ bool
WallpaperManager::GetLoggedInUserWallpaperInfo(WallpaperInfo* info) {
info->layout = current_user_wallpaper_info_.layout =
ash::WALLPAPER_LAYOUT_CENTER_CROPPED;
info->type = current_user_wallpaper_info_.type = User::DEFAULT;
+ info->date = current_user_wallpaper_info_.date =
+ base::Time::Now().LocalMidnight();
return true;
}
@@ -533,17 +582,16 @@ void WallpaperManager::RemoveUserWallpaperInfo(const
std::string& user_id) {
DeleteUserWallpapers(user_id, info.file);
}
-bool WallpaperManager::ResizeWallpaper(
- const UserImage& wallpaper,
- ash::WallpaperLayout layout,
- int preferred_width,
- int preferred_height,
- scoped_refptr<base::RefCountedBytes>* output,
- gfx::ImageSkia* output_skia) const {
- DCHECK(BrowserThread::GetBlockingPool()->
- IsRunningSequenceOnCurrentThread(sequence_token_));
- int width = wallpaper.image().width();
- int height = wallpaper.image().height();
+// static
+bool WallpaperManager::ResizeImage(const gfx::ImageSkia& image,
+ ash::WallpaperLayout layout,
+ int preferred_width,
+ int preferred_height,
+ scoped_refptr<base::RefCountedBytes>*
output,
+ gfx::ImageSkia* output_skia) {
+ DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+ int width = image.width();
+ int height = image.height();
int resized_width;
int resized_height;
*output = new base::RefCountedBytes();
@@ -573,19 +621,20 @@ bool WallpaperManager::ResizeWallpaper(
}
gfx::ImageSkia resized_image = gfx::ImageSkiaOperations::CreateResizedImage(
- wallpaper.image(),
+ image,
skia::ImageOperations::RESIZE_LANCZOS3,
gfx::Size(resized_width, resized_height));
- SkBitmap image = *(resized_image.bitmap());
- SkAutoLockPixels lock_input(image);
+ SkBitmap bitmap = *(resized_image.bitmap());
+ SkAutoLockPixels lock_input(bitmap);
gfx::JPEGCodec::Encode(
- reinterpret_cast<unsigned char*>(image.getAddr32(0, 0)),
+ reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)),
gfx::JPEGCodec::FORMAT_SkBitmap,
- image.width(),
- image.height(),
- image.width() * image.bytesPerPixel(),
- kDefaultEncodingQuality, &(*output)->data());
+ bitmap.width(),
+ bitmap.height(),
+ bitmap.width() * bitmap.bytesPerPixel(),
+ kDefaultEncodingQuality,
+ &(*output)->data());
if (output_skia) {
resized_image.MakeThreadSafe();
@@ -595,13 +644,13 @@ bool WallpaperManager::ResizeWallpaper(
return true;
}
-bool WallpaperManager::ResizeAndSaveWallpaper(
- const UserImage& wallpaper,
- const base::FilePath& path,
- ash::WallpaperLayout layout,
- int preferred_width,
- int preferred_height,
- gfx::ImageSkia* result_out) const {
+// static
+bool WallpaperManager::ResizeAndSaveWallpaper(const …
(message too large)
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tnagel@chromium.org/221873005/350001
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tnagel@chromium.org/221873005/370001
Message was sent while issue was closed.
Change committed as 266026 |
