Issue 208273005: If customization includes default wallpaper, download and apply it.

Alexander Alekseev

Please review: mnissler@ : pref_names.* bshe@ : wallpaper_manager, desktop_background_controller, dpolukhin@, nkostylev@: all ;-) a) Constraints: ...

6 years, 9 months ago (2014-03-24 16:10:24 UTC) #1

Nikita (slow)

On 2014/03/24 16:10:24, alemate wrote: > bshe@ : wallpaper_manager, desktop_background_controller, +derat@ as well > bshe@: ...

6 years, 9 months ago (2014-03-24 16:15:22 UTC) #2

Daniel Erat

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc#newcode37 ash/desktop_background/desktop_background_controller.cc:37: #include "chrome/browser/chromeos/login/wallpaper_manager.h" ash isn't allowed to depend on chrome. ...

6 years, 9 months ago (2014-03-24 16:25:48 UTC) #3

Alexander Alekseev

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc#newcode37 ash/desktop_background/desktop_background_controller.cc:37: #include "chrome/browser/chromeos/login/wallpaper_manager.h" On 2014/03/24 16:25:48, Daniel Erat wrote: > ...

6 years, 9 months ago (2014-03-24 17:33:42 UTC) #4

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/deskt...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_controller.cc:37: #include
"chrome/browser/chromeos/login/wallpaper_manager.h"
On 2014/03/24 16:25:48, Daniel Erat wrote:
> ash isn't allowed to depend on chrome. you need to make the chrome code call
> into this class instead. see suggestions below.

Done.

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_controller.cc:296:
customized_default_wallpaper_file_large_);
On 2014/03/24 16:25:48, Daniel Erat wrote:
> why is all this stuff necessary? does the following simpler approach work?
>
> - add default_small_wallpaper_path_ and default_large_wallpaper_path_ members
> - initialize them to the values from the flags in the c'tor
> - make SetDefaultWallpaperPath() just update the members and call
> SetDefaultWallpaper()
> - make SetDefaultWallpaper() use the members instead of checking the switches
> and leave it otherwise unchanged

Done.

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_controller.cc:401: const
base::FilePath& image_file) const {
On 2014/03/24 16:25:48, Daniel Erat wrote:
> why do you need this method? it looks like it's mostly a copy-and-paste of the
> existing method.

I need to detect the usage of default wallpaper regardless of the layout. Would
you prefer to add additional parameter to existing method
DefaultWallpaperIsAlreadyLoadingOrLoaded() to ignore resource_id?

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/deskt...
File ash/desktop_background/desktop_background_controller.h (right):

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_controller.h:139: void
SetDefaultWallpaperPath(
On 2014/03/24 16:25:48, Daniel Erat wrote:
> nit: move this above the overridden method (also in .cc file)

Done.

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_controller.h:140: const
base::FilePath& customized_default_wallpaper_file_small,
On 2014/03/24 16:25:48, Daniel Erat wrote:
> nit: mind removing the word "customized" from these argument names? i'm
worried
> it might make it easy to confuse this with SetCustomWallpaper().

Done.

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_controller.h:152: // (i.e. no need to
reload if exacltly duplicate request received)
On 2014/03/24 16:25:48, Daniel Erat wrote:
> nit: s/exacltly/exactly/

Done.

Daniel Erat

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc#newcode401 ash/desktop_background/desktop_background_controller.cc:401: const base::FilePath& image_file) const { On 2014/03/24 17:33:43, alemate ...

6 years, 9 months ago (2014-03-24 19:19:42 UTC) #5

Daniel Erat

https://codereview.chromium.org/208273005/diff/50001/ash/desktop_background/desktop_background_controller.cc File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/208273005/diff/50001/ash/desktop_background/desktop_background_controller.cc#newcode238 ash/desktop_background/desktop_background_controller.cc:238: ShellDelegate* delegate = ash::Shell::GetInstance()->delegate(); i still don't understand why ...

6 years, 9 months ago (2014-03-24 19:24:40 UTC) #6

Alexander Alekseev

https://codereview.chromium.org/208273005/diff/50001/ash/desktop_background/desktop_background_controller.cc File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/208273005/diff/50001/ash/desktop_background/desktop_background_controller.cc#newcode238 ash/desktop_background/desktop_background_controller.cc:238: ShellDelegate* delegate = ash::Shell::GetInstance()->delegate(); On 2014/03/24 19:24:40, Daniel Erat ...

6 years, 9 months ago (2014-03-24 19:47:54 UTC) #7

Alexander Alekseev

https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc#newcode401 ash/desktop_background/desktop_background_controller.cc:401: const base::FilePath& image_file) const { On 2014/03/24 19:19:43, Daniel ...

6 years, 9 months ago (2014-03-24 19:52:57 UTC) #8

Daniel Erat

On 2014/03/24 19:52:57, alemate wrote: > https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc > File ash/desktop_background/desktop_background_controller.cc (right): > > https://codereview.chromium.org/208273005/diff/1/ash/desktop_background/desktop_background_controller.cc#newcode401 > ...

6 years, 9 months ago (2014-03-24 20:23:01 UTC) #9

Mattias Nissler (ping if slow)

pref_names.{cc,h} LGTM, but please have somebody take another look after you addressed my other comments. ...

6 years, 9 months ago (2014-03-24 20:38:53 UTC) #10

bshe

https://codereview.chromium.org/208273005/diff/60019/ash/desktop_background/desktop_background_controller.cc File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/208273005/diff/60019/ash/desktop_background/desktop_background_controller.cc#newcode250 ash/desktop_background/desktop_background_controller.cc:250: bool DesktopBackgroundController::SetDefaultWallpaper(bool is_guest) { SetDefaultWallpaper is only called in ...

6 years, 9 months ago (2014-03-24 21:30:49 UTC) #11

Daniel Erat

https://codereview.chromium.org/210313004/ is along the lines of what I was thinking of to make DesktopBackgroundController support ...

6 years, 9 months ago (2014-03-24 22:42:50 UTC) #12

Alexander Alekseev

On 2014/03/24 22:42:50, Daniel Erat wrote: > https://codereview.chromium.org/210313004/ is along the lines of what I ...

6 years, 9 months ago (2014-03-24 23:04:23 UTC) #13

Alexander Alekseev

On 2014/03/24 22:42:50, Daniel Erat wrote: > https://codereview.chromium.org/210313004/ is along the lines of what I ...

6 years, 9 months ago (2014-03-24 23:05:56 UTC) #14

Daniel Erat

On 2014/03/24 23:05:56, alemate wrote: > On 2014/03/24 22:42:50, Daniel Erat wrote: > > https://codereview.chromium.org/210313004/ ...

6 years, 9 months ago (2014-03-24 23:15:01 UTC) #15

Alexander Alekseev

On 2014/03/24 23:15:01, Daniel Erat wrote: > On 2014/03/24 23:05:56, alemate wrote: > > On ...

6 years, 9 months ago (2014-03-25 00:15:46 UTC) #16

Daniel Erat

On 2014/03/25 00:15:46, alemate wrote: > On 2014/03/24 23:15:01, Daniel Erat wrote: > > On ...

6 years, 9 months ago (2014-03-25 00:39:02 UTC) #17

On 2014/03/25 00:15:46, alemate wrote:
> On 2014/03/24 23:15:01, Daniel Erat wrote:
> > On 2014/03/24 23:05:56, alemate wrote:
> > > On 2014/03/24 22:42:50, Daniel Erat wrote:
> > > > https://codereview.chromium.org/210313004/ is along the lines of what I
> was
> > > > thinking of to make DesktopBackgroundController support changing the
> default
> > > > wallpaper without adding another dependency back to WallpaperManager. If
> > this
> > > > does what you need, let me know and I'll add some tests to it and send
it
> > for
> > > > review.
> > > 
> > > But your CL says nothing about customization. When should it be applied?
> > 
> > after c/b/chromeos/login has downloaded the customized default wallpaper and
> > wants to use it instead of the previous default, it would call
> > SetDefaultWallpaperPaths().
> 
> There is no problem when chrome has already started and at some point it
> acquires new wallpaper.
> 
> But what if wallpaper has been already downloaded and cached to local drive?
> At browser start we should never use wallpaper from command line.
> That is why I need to make DesktopBackgroundController to check not only
> command-line
> for wallpaper paths, but also some third-party source like ShellDelegate for
> appropriate paths.
> Otherwise we would once use command-line default because chrome code has not
> reached the point to
> call SetDefaultWallapperPaths(). That is the question.
> I don't see how I can get away from modifying ShellDelegate.

thanks for the explanation. what about the following:

a) DesktopBackgroundController calls the delegate to get all of the paths at
startup (i.e. move all of the flag-reading code into the delegate). i'm not sure
if ShellDelegate is the correct place for this or if the current
UserWallpaperDelegate interface should be renamed to something like
WallpaperDelegate instead.

b) if customized wallpaper is downloaded later, a new
DesktopBackgroundController::UpdateDefaultWallpaper() method is called, which
triggers the same code path from (a).

?


>  
> > i think i'm also going to rename
> > DesktopBackgroundController::SetCustomWallpaper() to SetUserWallpaper()
since
> > we're using "custom" to refer to two different things now.

Alexander Alekseev

On 2014/03/25 00:39:02, Daniel Erat wrote: > On 2014/03/25 00:15:46, alemate wrote: > > On ...

6 years, 9 months ago (2014-03-25 01:25:01 UTC) #18

On 2014/03/25 00:39:02, Daniel Erat wrote:
> On 2014/03/25 00:15:46, alemate wrote:
> > On 2014/03/24 23:15:01, Daniel Erat wrote:
> > > On 2014/03/24 23:05:56, alemate wrote:
> > > > On 2014/03/24 22:42:50, Daniel Erat wrote:
> > > > > https://codereview.chromium.org/210313004/ is along the lines of what
I
> > was
> > > > > thinking of to make DesktopBackgroundController support changing the
> > default
> > > > > wallpaper without adding another dependency back to WallpaperManager.
If
> > > this
> > > > > does what you need, let me know and I'll add some tests to it and send
> it
> > > for
> > > > > review.
> > > > 
> > > > But your CL says nothing about customization. When should it be applied?
> > > 
> > > after c/b/chromeos/login has downloaded the customized default wallpaper
and
> > > wants to use it instead of the previous default, it would call
> > > SetDefaultWallpaperPaths().
> > 
> > There is no problem when chrome has already started and at some point it
> > acquires new wallpaper.
> > 
> > But what if wallpaper has been already downloaded and cached to local drive?
> > At browser start we should never use wallpaper from command line.
> > That is why I need to make DesktopBackgroundController to check not only
> > command-line
> > for wallpaper paths, but also some third-party source like ShellDelegate for
> > appropriate paths.
> > Otherwise we would once use command-line default because chrome code has not
> > reached the point to
> > call SetDefaultWallapperPaths(). That is the question.
> > I don't see how I can get away from modifying ShellDelegate.
> 
> thanks for the explanation. what about the following:
> 
> a) DesktopBackgroundController calls the delegate to get all of the paths at
> startup (i.e. move all of the flag-reading code into the delegate). i'm not
sure
> if ShellDelegate is the correct place for this or if the current
> UserWallpaperDelegate interface should be renamed to something like
> WallpaperDelegate instead.
> 
> b) if customized wallpaper is downloaded later, a new
> DesktopBackgroundController::UpdateDefaultWallpaper() method is called, which
> triggers the same code path from (a).
> 
> ?

Yes, and this is exactly what I was trying to implement ;-)
I do not know the exact creation order of DesktopBackgroundController /
WallpaperManager / CustomizationDocument. They all seem to me independent. And I
didn't find any "Init" method in DesktopBackgroundController. That is why I've
added lazy initialization with flag "customization_applied_" to be sure to call
it before setting the wallpapaper, but after all required objects have been
created.

Your Cl seems better (as you know DesktopBackgroundController better). The only
thing that seems strange to me is that the checks in GetDefaultWallpaperInfo()
differ from DefaultWallpaperIsAlreadyLoadingOrLoaded(). You've recently fixed
the races problem with this check when changing user wallapaper has been speed
up. I think that this might lead to problems if we allow different "default"
wallpapers for different users (we do allow different versions of customization
manifest for different users, we just do not allow different "default"
wallpapers for now). Because in multiprofile mode switching between users can be
very fast.

And I also should know (by tomorrow) which delegate to extend. I would be very
nice if you could tell me which one to modify.

> > > i think i'm also going to rename
> > > DesktopBackgroundController::SetCustomWallpaper() to SetUserWallpaper()
> since
> > > we're using "custom" to refer to two different things now.

Daniel Erat

On 2014/03/25 01:25:01, alemate wrote: > On 2014/03/25 00:39:02, Daniel Erat wrote: > > On ...

6 years, 9 months ago (2014-03-25 02:31:43 UTC) #19

On 2014/03/25 01:25:01, alemate wrote:
> On 2014/03/25 00:39:02, Daniel Erat wrote:
> > On 2014/03/25 00:15:46, alemate wrote:
> > > On 2014/03/24 23:15:01, Daniel Erat wrote:
> > > > On 2014/03/24 23:05:56, alemate wrote:
> > > > > On 2014/03/24 22:42:50, Daniel Erat wrote:
> > > > > > https://codereview.chromium.org/210313004/ is along the lines of
what
> I
> > > was
> > > > > > thinking of to make DesktopBackgroundController support changing the
> > > default
> > > > > > wallpaper without adding another dependency back to
WallpaperManager.
> If
> > > > this
> > > > > > does what you need, let me know and I'll add some tests to it and
send
> > it
> > > > for
> > > > > > review.
> > > > > 
> > > > > But your CL says nothing about customization. When should it be
applied?
> > > > 
> > > > after c/b/chromeos/login has downloaded the customized default wallpaper
> and
> > > > wants to use it instead of the previous default, it would call
> > > > SetDefaultWallpaperPaths().
> > > 
> > > There is no problem when chrome has already started and at some point it
> > > acquires new wallpaper.
> > > 
> > > But what if wallpaper has been already downloaded and cached to local
drive?
> > > At browser start we should never use wallpaper from command line.
> > > That is why I need to make DesktopBackgroundController to check not only
> > > command-line
> > > for wallpaper paths, but also some third-party source like ShellDelegate
for
> > > appropriate paths.
> > > Otherwise we would once use command-line default because chrome code has
not
> > > reached the point to
> > > call SetDefaultWallapperPaths(). That is the question.
> > > I don't see how I can get away from modifying ShellDelegate.
> > 
> > thanks for the explanation. what about the following:
> > 
> > a) DesktopBackgroundController calls the delegate to get all of the paths at
> > startup (i.e. move all of the flag-reading code into the delegate). i'm not
> sure
> > if ShellDelegate is the correct place for this or if the current
> > UserWallpaperDelegate interface should be renamed to something like
> > WallpaperDelegate instead.
> > 
> > b) if customized wallpaper is downloaded later, a new
> > DesktopBackgroundController::UpdateDefaultWallpaper() method is called,
which
> > triggers the same code path from (a).
> > 
> > ?
> 
> Yes, and this is exactly what I was trying to implement ;-)
> I do not know the exact creation order of DesktopBackgroundController /
> WallpaperManager / CustomizationDocument. They all seem to me independent. And
I
> didn't find any "Init" method in DesktopBackgroundController. That is why I've
> added lazy initialization with flag "customization_applied_" to be sure to
call
> it before setting the wallpapaper, but after all required objects have been
> created.
>  
> 
> Your Cl seems better (as you know DesktopBackgroundController better). The
only
> thing that seems strange to me is that the checks in GetDefaultWallpaperInfo()
> differ from DefaultWallpaperIsAlreadyLoadingOrLoaded(). You've recently fixed
> the races problem with this check when changing user wallapaper has been speed
> up. I think that this might lead to problems if we allow different "default"
> wallpapers for different users (we do allow different versions of
customization
> manifest for different users, we just do not allow different "default"
> wallpapers for now). Because in multiprofile mode switching between users can
be
> very fast.
> 
> And I also should know (by tomorrow) which delegate to extend. I would be very
> nice if you could tell me which one to modify.

i'm glad we agree. :-)

the different-default-wallpapers-for-different-users thing is interesting.
what's the motivation behind supporting different versions of the customization
manifest for each user? in the current pre-customization world, i believe that
we just have the concept of a single "default" wallpaper, e.g. if you've never
selected a wallpaper, you'll see a different image on lumpy vs. other machines
that have OEM customization of the default wallpaper, like spring. is there any
way for a user to switch back to the default after they've changed the
wallpaper?

i suspect that WallpaperManager is instantiated in
ChromeBrowserMainPartsChromeos::PreProfileInit() (it's hard to be certain since
it's a lazily-initialized singleton). ash::Shell looks like it's initialized
slightly later in the same method, so i don't think you'll have trouble with
WallpaperManager not existing when DesktopBackgroundController is initialized.
you could tell for sure by adding some logging statements to both classes'
constructors. it would safer for DesktopBackgroundController take a delegate
(implemented by WallpaperManager) in its c'tor so that the dependency is
explicit.

i don't think that adding a DesktopBackgroundController::Init() method would
accomplish anything; it's owned by ash::Shell, which generally both constructs
and initializes objects in its own Init() method.

also note that DesktopBackgroundController doesn't actually do anything until
SetDefaultWallpaper() or SetCustomWallpaper() has been called, so i would expect
it to be safe to create DBC even before you know for sure which wallpaper you
want to start with.

i don't understand the comment about the checks in GetDefaultWallpaperInfo() and
DefaultWallpaperIsAlreadyLoadingOrLoaded() differing. there shouldn't be any
checks in GetDefault(); i just added it to unduplicate some code that decides
which default wallpaper we should use at a given time.
DefaultWallpaperIsAlreadyLoadingOrLoaded() checks whether the passed-in
wallpaper is already loading or loaded.

my main concern is that DesktopBackgroundController doesn't get more complicated
to support this feature. it shouldn't need any more changes than what's
necessary to support the default wallpaper being changed instead of remaining
constant.

here's a perhaps even simpler change:

- change DesktopBackgroundController::SetDefaultWallpaper() to have the
following signature:

  bool SetDefaultWallpaper(const base::FilePath& small_path,
                           const base::FilePath& large_path);

- move all of the default-vs.-guest code and switch-vs.-customization-manifest
code into WallpaperManager and make it just pass the appropriate images when
calling SetDefaultWallpaper().

- leave the rest of SetDefaultWallpaper() unchanged. i think that
DefaultWallpaperIsAlreadyLoadingOrLoaded should already do what you need it to
do (i.e. if you pass an updated path, it'll return false).

this way you avoid adding any dependencies from DesktopBackgroundController back
to c/b/chromeos/login, and i don't think you'll need to update the delegate, and
you shouldn't need to make any major changes to DBC, and you can just call
SetDefaultWallpaper() again with the updated paths after you've downloaded the
images. this seems much cleaner and simpler. am i missing something?

if you go this route, you may need to update any non-chrome-os platforms that
are using ash. i don't know where their wallpaper currently comes from.

side observation: it is unfortunate that this code is being written just a few
days before the branch point. :-(

> > > > i think i'm also going to rename
> > > > DesktopBackgroundController::SetCustomWallpaper() to SetUserWallpaper()
> > since
> > > > we're using "custom" to refer to two different things now.

Dmitry Polukhin

Different users may have different customizations because we by design download and apply customizations when ...

6 years, 9 months ago (2014-03-25 02:43:09 UTC) #20

Different users may have different customizations because we by design
download and apply customizations when user logs in first time and don't
change it after that to don't confuse user. We discussed it with Zel
several times and he thunks that changing customization for existing users
will be confusing. Therefore users created at different times may have
different default customization (i.e. list of default apps). As for default
wallpaper for the machine it should be set only once when OEM customization
downloaded for the very first time and shouldn't be changed after that.


On Mon, Mar 24, 2014 at 7:31 PM, <derat@chromium.org> wrote:

> On 2014/03/25 01:25:01, alemate wrote:
>
>> On 2014/03/25 00:39:02, Daniel Erat wrote:
>> > On 2014/03/25 00:15:46, alemate wrote:
>> > > On 2014/03/24 23:15:01, Daniel Erat wrote:
>> > > > On 2014/03/24 23:05:56, alemate wrote:
>> > > > > On 2014/03/24 22:42:50, Daniel Erat wrote:
>> > > > > > https://codereview.chromium.org/210313004/ is along the lines
>> of
>>
> what
>
>> I
>> > > was
>> > > > > > thinking of to make DesktopBackgroundController support
>> changing the
>> > > default
>> > > > > > wallpaper without adding another dependency back to
>>
> WallpaperManager.
>
>> If
>> > > > this
>> > > > > > does what you need, let me know and I'll add some tests to it
>> and
>>
> send
>
>> > it
>> > > > for
>> > > > > > review.
>> > > > >
>> > > > > But your CL says nothing about customization. When should it be
>>
> applied?
>
>> > > >
>> > > > after c/b/chromeos/login has downloaded the customized default
>> wallpaper
>> and
>> > > > wants to use it instead of the previous default, it would call
>> > > > SetDefaultWallpaperPaths().
>> > >
>> > > There is no problem when chrome has already started and at some point
>> it
>> > > acquires new wallpaper.
>> > >
>> > > But what if wallpaper has been already downloaded and cached to local
>>
> drive?
>
>> > > At browser start we should never use wallpaper from command line.
>> > > That is why I need to make DesktopBackgroundController to check not
>> only
>> > > command-line
>> > > for wallpaper paths, but also some third-party source like
>> ShellDelegate
>>
> for
>
>> > > appropriate paths.
>> > > Otherwise we would once use command-line default because chrome code
>> has
>>
> not
>
>> > > reached the point to
>> > > call SetDefaultWallapperPaths(). That is the question.
>> > > I don't see how I can get away from modifying ShellDelegate.
>> >
>> > thanks for the explanation. what about the following:
>> >
>> > a) DesktopBackgroundController calls the delegate to get all of the
>> paths at
>> > startup (i.e. move all of the flag-reading code into the delegate). i'm
>> not
>> sure
>> > if ShellDelegate is the correct place for this or if the current
>> > UserWallpaperDelegate interface should be renamed to something like
>> > WallpaperDelegate instead.
>> >
>> > b) if customized wallpaper is downloaded later, a new
>> > DesktopBackgroundController::UpdateDefaultWallpaper() method is called,
>>
> which
>
>> > triggers the same code path from (a).
>> >
>> > ?
>>
>
>  Yes, and this is exactly what I was trying to implement ;-)
>> I do not know the exact creation order of DesktopBackgroundController /
>> WallpaperManager / CustomizationDocument. They all seem to me
>> independent. And
>>
> I
>
>> didn't find any "Init" method in DesktopBackgroundController. That is why
>> I've
>> added lazy initialization with flag "customization_applied_" to be sure to
>>
> call
>
>> it before setting the wallpapaper, but after all required objects have
>> been
>> created.
>>
>
>
>  Your Cl seems better (as you know DesktopBackgroundController better). The
>>
> only
>
>> thing that seems strange to me is that the checks in
>> GetDefaultWallpaperInfo()
>> differ from DefaultWallpaperIsAlreadyLoadingOrLoaded(). You've recently
>> fixed
>> the races problem with this check when changing user wallapaper has been
>> speed
>> up. I think that this might lead to problems if we allow different
>> "default"
>> wallpapers for different users (we do allow different versions of
>>
> customization
>
>> manifest for different users, we just do not allow different "default"
>> wallpapers for now). Because in multiprofile mode switching between users
>> can
>>
> be
>
>> very fast.
>>
>
>  And I also should know (by tomorrow) which delegate to extend. I would be
>> very
>> nice if you could tell me which one to modify.
>>
>
> i'm glad we agree. :-)
>
> the different-default-wallpapers-for-different-users thing is interesting.
> what's the motivation behind supporting different versions of the
> customization
> manifest for each user? in the current pre-customization world, i believe
> that
> we just have the concept of a single "default" wallpaper, e.g. if you've
> never
> selected a wallpaper, you'll see a different image on lumpy vs. other
> machines
> that have OEM customization of the default wallpaper, like spring. is
> there any
> way for a user to switch back to the default after they've changed the
> wallpaper?
>
> i suspect that WallpaperManager is instantiated in
> ChromeBrowserMainPartsChromeos::PreProfileInit() (it's hard to be certain
> since
> it's a lazily-initialized singleton). ash::Shell looks like it's
> initialized
> slightly later in the same method, so i don't think you'll have trouble
> with
> WallpaperManager not existing when DesktopBackgroundController is
> initialized.
> you could tell for sure by adding some logging statements to both classes'
> constructors. it would safer for DesktopBackgroundController take a
> delegate
> (implemented by WallpaperManager) in its c'tor so that the dependency is
> explicit.
>
> i don't think that adding a DesktopBackgroundController::Init() method
> would
> accomplish anything; it's owned by ash::Shell, which generally both
> constructs
> and initializes objects in its own Init() method.
>
> also note that DesktopBackgroundController doesn't actually do anything
> until
> SetDefaultWallpaper() or SetCustomWallpaper() has been called, so i would
> expect
> it to be safe to create DBC even before you know for sure which wallpaper
> you
> want to start with.
>
> i don't understand the comment about the checks in
> GetDefaultWallpaperInfo() and
> DefaultWallpaperIsAlreadyLoadingOrLoaded() differing. there shouldn't be
> any
> checks in GetDefault(); i just added it to unduplicate some code that
> decides
> which default wallpaper we should use at a given time.
> DefaultWallpaperIsAlreadyLoadingOrLoaded() checks whether the passed-in
> wallpaper is already loading or loaded.
>
> my main concern is that DesktopBackgroundController doesn't get more
> complicated
> to support this feature. it shouldn't need any more changes than what's
> necessary to support the default wallpaper being changed instead of
> remaining
> constant.
>
> here's a perhaps even simpler change:
>
> - change DesktopBackgroundController::SetDefaultWallpaper() to have the
> following signature:
>
>   bool SetDefaultWallpaper(const base::FilePath& small_path,
>                            const base::FilePath& large_path);
>
> - move all of the default-vs.-guest code and switch-vs.-customization-
> manifest
> code into WallpaperManager and make it just pass the appropriate images
> when
> calling SetDefaultWallpaper().
>
> - leave the rest of SetDefaultWallpaper() unchanged. i think that
> DefaultWallpaperIsAlreadyLoadingOrLoaded should already do what you need
> it to
> do (i.e. if you pass an updated path, it'll return false).
>
> this way you avoid adding any dependencies from
> DesktopBackgroundController back
> to c/b/chromeos/login, and i don't think you'll need to update the
> delegate, and
> you shouldn't need to make any major changes to DBC, and you can just call
> SetDefaultWallpaper() again with the updated paths after you've downloaded
> the
> images. this seems much cleaner and simpler. am i missing something?
>
> if you go this route, you may need to update any non-chrome-os platforms
> that
> are using ash. i don't know where their wallpaper currently comes from.
>
> side observation: it is unfortunate that this code is being written just a
> few
> days before the branch point. :-(
>
>
>  > > > i think i'm also going to rename
>> > > > DesktopBackgroundController::SetCustomWallpaper() to
>> SetUserWallpaper()
>> > since
>> > > > we're using "custom" to refer to two different things now.
>>
>
>
> https://codereview.chromium.org/208273005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Alexander Alekseev

I've removed modifications of ShellDelegate. Default wallpaper paths are initialized in WallpaperManager::InitializeWallpaper(). Please review again. ...

6 years, 9 months ago (2014-03-25 14:38:28 UTC) #21

I've removed modifications of ShellDelegate.
Default wallpaper paths are initialized in
WallpaperManager::InitializeWallpaper().

Please review again.

https://codereview.chromium.org/208273005/diff/60019/ash/desktop_background/d...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/208273005/diff/60019/ash/desktop_background/d...
ash/desktop_background/desktop_background_controller.cc:250: bool
DesktopBackgroundController::SetDefaultWallpaper(bool is_guest) {
On 2014/03/24 21:30:49, bshe wrote:
> SetDefaultWallpaper is only called in wallpaper_manager.cc and content_shell.
Do
> we have to let DesktopBackgroundController know about the new customized
default
> wallpaper? If not, we can probably do all the logic in wallpaper_manager.cc
and
> fall back to this SetDefaultWallpaper code if the new customized default
> wallpaper doesn't exist. It may make code simpler.

Done.

https://codereview.chromium.org/208273005/diff/60019/ash/desktop_background/d...
File ash/desktop_background/desktop_background_controller.h (right):

https://codereview.chromium.org/208273005/diff/60019/ash/desktop_background/d...
ash/desktop_background/desktop_background_controller.h:158: // loaded
|wallpaper_loader_| or stored in |current_wallpaper_|.
On 2014/03/24 20:38:54, Mattias Nissler wrote:
> nit: ... loaded *by* |wallpaper_loader|...

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_document.cc (right):

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:72: static const char
kCustomizationDefaultWallpaperDir[] = "customization/";
On 2014/03/24 21:30:49, bshe wrote:
> nit: remove static keyword to keep it consistent with other variables.

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:76: "default.downloaded";
On 2014/03/24 21:30:49, bshe wrote:
> ditto

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:138:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
On 2014/03/24 21:30:49, bshe wrote:
> It looks like this is called on worker pool thread, not file thread.

Yes, but there is no direct way to check that we're on a worker pool.
CurrentlyOn(FILE) is true on WorkerPool (WorkerPool is a replacement for FILE,
isn't it?).

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:556: DCHECK(success);
On 2014/03/24 21:30:49, bshe wrote:
> why do you DCHECK here and have a if statement below?

The idea is that we should fail in tests here, but probably should not crash if
some configuration has changed and wallpapers disappeared.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:566:
ServicesCustomizationDocument::GetCustomizedWallpaperDownloadedFileName() {
On 2014/03/24 21:30:49, bshe wrote:
> nit: 4 spaces indent

Done. (Strange, but clang-format doesn't agree with you here).

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:601: DCHECK(prefService);
On 2014/03/24 20:38:54, Mattias Nissler wrote:
> Remove, you'll get a crash report anyways below in line 604 if prefService ==
> NULL.

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:606: VLOG(1) <<
"ServicesCustomizationDocument::ApplyWallpaper() : "
On 2014/03/24 20:38:54, Mattias Nissler wrote:
> This VLOG is misleading, because the code below only uses current_url if it's
> valid (i.e. empty string gets ignored too).

If "current_url" is empty, "GURL(current_url).is_valid()" will fail and we will
start downloading new wallaper.

If "current_url" is valid, we would always use old wallpaper, but it seems
reasonable to me to signal that it has changed in manifest.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:612: // current_url ==
wallpaper_url.spec() )
On 2014/03/24 21:30:49, bshe wrote:
> I am not sure I understand this. What is we want to add a larger default
> wallpaper for xhdpi screen? it would not possible?

This is mentioned in design document: customization document might be updated,
but we never update default wallpaper in order not to confuse user.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:616:
SetDefaultWallpaperPath(GURL(current_url));
On 2014/03/24 21:30:49, bshe wrote:
> could we call:
> WallpaperManager::Get()->SetCustomizedDefaultWallpaper(
>        wallpaper_url,
>        GetCustomizedWallpaperDownloadedFileName(),
>        GetCustomizedWallpaperCacheDir());
> 
> directly from here? The static function SetDefaultWallpaperPath doesn't seem
to
> do much thing? You include the VLOG in the SetCustomizedDefaultWallpaper.

SetDefaultWallpaperPath() is called both from here and from
customization_wallpaper_downloader. So I've created that function as  a single
point of setting default wallpaper path in this object.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right):

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_wallpaper_downloader.cc:134: Retry();
On 2014/03/24 20:38:54, Mattias Nissler wrote:
> So this keeps retrying forever. Is that intentional? I guess you should give
up
> at some point.

Why? May be we should increase retry sleep...

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_wallpaper_downloader.cc:170:
ServicesCustomizationDocument::SetDefaultWallpaperPath(wallpaper_url_);
On 2014/03/24 20:38:54, Mattias Nissler wrote:
> The use of static functions for reporting success and self-destruction seems
> like a hack to me. The standard solution to this problem is to have the owning
> class pass a callback to the ctor that gets invoked with success or error
status
> and then the caller can destroy the downloader object.

I've recently received comment (in another CL) that self-destroying callbacks
are confusing (even with comment), and had to change it to direct calls to
"owner->DestroySelf()" ;-)

So I'd leave this decision to dpolukhin.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:50: 
On 2014/03/24 21:30:49, bshe wrote:
> nit: these should implemented before testApi class according the order in .h
> file. Please move them to the correct place.

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:51: class
WallpaperManager::CustomizedWallpaperRescaledFiles {
On 2014/03/24 21:30:49, bshe wrote:
> nit: can you move the definition to .h file. Also, this looks like a struct is
> enough? Same for CustomizedWallpaperFilesExist

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:57: base::FilePath
path_rescaled_original;
On 2014/03/24 21:30:49, bshe wrote:
> nit: the name is misleading. The original file is not rescaled.

The original file is rescaled to get safe image:

localhost ~ # ls -l /home/chronos/custom_wallpapers/customization/
total 4140
-rw-r--r-- 1 chronos chronos 1404517 Mar 25 16:55 default.downloaded
-rw-r--r-- 1 chronos chronos 1129849 Mar 25 16:55 default.downloaded.large.jpg
-rw-r--r-- 1 chronos chronos 1380721 Mar 25 16:55
default.downloaded.original.jpg
-rw-r--r-- 1 chronos chronos  318315 Mar 25 16:55 default.downloaded.small.jpg
localhost ~ # 

The "rescaled original" is not used, it is done mostly for debug.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:141: const char
kCustomizedWallpaperLargeFileSuffix[] = ".large.jpg";
On 2014/03/24 21:30:49, bshe wrote:
> nit: can you reuse kSmallWallpaperSuffix and kLargeWallpaperSuffix? The
original
> file shouldn't need a suffix. This way it is consistent with existing name
> pattern.

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:171:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
On 2014/03/24 21:30:49, bshe wrote:
> nit: worker pool thread instead of FILE?

Do you know simple way to check for worker pool?

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1450: DCHECK(written_bytes ==
size);
On 2014/03/24 21:30:49, bshe wrote:
> nit: DCHECK is probably not necessary in this case, you can remove it

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1524: // static
On 2014/03/24 21:30:49, bshe wrote:
> GetCustomizedWallpaperDefaultRescaledSmallFileName and
> GetCustomizedWallpaperDefaultRescaledLargeFileName looks very similar. Can you
> consolidate the two functions into one (like add a boolean/enum parameter or
> something)?

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1526:
WallpaperManager::GetCustomizedWallpaperDefaultRescaledSmallFileName() {
On 2014/03/24 21:30:49, bshe wrote:
> nit: 4 space indent please

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1540:
WallpaperManager::GetCustomizedWallpaperDefaultRescaledLargeFileName() {
On 2014/03/24 21:30:49, bshe wrote:
> nit: 4 space indent please

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1557:
DCHECK(wallpaper_url.is_valid() || wallpaper_url.is_empty());
On 2014/03/24 21:30:49, bshe wrote:
> nit: you shouldn't need to handle failed DCHECK case if I understand
correctly.

This is one of a few cases when we should fail in tests, but do not fail in
production. It checks for misconfiguration and should stop in debug build. But I
think we should not brick chromeos device if something is wrong with wallpaper.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1639: DCHECK(prefService);
On 2014/03/24 20:38:54, Mattias Nissler wrote:
> remove redundant DCHECK

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1700: DCHECK(prefService);
On 2014/03/24 20:38:54, Mattias Nissler wrote:
> remove redundant DCHECK

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1716:
->desktop_background_controller()
On 2014/03/24 21:30:49, bshe wrote:
> nit: this line seems can fit to the previous line

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1725: DCHECK(prefService);
On 2014/03/24 20:38:54, Mattias Nissler wrote:
> remove redundant DCHECK

Done.

https://codereview.chromium.org/208273005/diff/60019/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1729: return
!current_url.empty();
On 2014/03/24 20:38:54, Mattias Nissler wrote:
> Is this supposed to be "if not set"? If so, consider using
> PrefService::Preference::IsDefaultValue().

Done.

Daniel Erat

did you see the long comment that i wrote suggesting what seems like a much-cleaner ...

6 years, 9 months ago (2014-03-25 15:36:26 UTC) #22

Alexander Alekseev

On 2014/03/25 02:31:43, Daniel Erat wrote: > On 2014/03/25 01:25:01, alemate wrote: > > On ...

6 years, 9 months ago (2014-03-25 15:51:22 UTC) #23

On 2014/03/25 02:31:43, Daniel Erat wrote:
> On 2014/03/25 01:25:01, alemate wrote:
> > On 2014/03/25 00:39:02, Daniel Erat wrote:
> > > On 2014/03/25 00:15:46, alemate wrote:
> > > > On 2014/03/24 23:15:01, Daniel Erat wrote:
> > > > > On 2014/03/24 23:05:56, alemate wrote:
> > > > > > On 2014/03/24 22:42:50, Daniel Erat wrote:
> > > > > > > https://codereview.chromium.org/210313004/ is along the lines of
> what
> > I
> > > > was
> > > > > > > thinking of to make DesktopBackgroundController support changing
the
> > > > default
> > > > > > > wallpaper without adding another dependency back to
> WallpaperManager.
> > If
> > > > > this
> > > > > > > does what you need, let me know and I'll add some tests to it and
> send
> > > it
> > > > > for
> > > > > > > review.
> > > > > > 
> > > > > > But your CL says nothing about customization. When should it be
> applied?
> > > > > 
> > > > > after c/b/chromeos/login has downloaded the customized default
wallpaper
> > and
> > > > > wants to use it instead of the previous default, it would call
> > > > > SetDefaultWallpaperPaths().
> > > > 
> > > > There is no problem when chrome has already started and at some point it
> > > > acquires new wallpaper.
> > > > 
> > > > But what if wallpaper has been already downloaded and cached to local
> drive?
> > > > At browser start we should never use wallpaper from command line.
> > > > That is why I need to make DesktopBackgroundController to check not only
> > > > command-line
> > > > for wallpaper paths, but also some third-party source like ShellDelegate
> for
> > > > appropriate paths.
> > > > Otherwise we would once use command-line default because chrome code has
> not
> > > > reached the point to
> > > > call SetDefaultWallapperPaths(). That is the question.
> > > > I don't see how I can get away from modifying ShellDelegate.
> > > 
> > > thanks for the explanation. what about the following:
> > > 
> > > a) DesktopBackgroundController calls the delegate to get all of the paths
at
> > > startup (i.e. move all of the flag-reading code into the delegate). i'm
not
> > sure
> > > if ShellDelegate is the correct place for this or if the current
> > > UserWallpaperDelegate interface should be renamed to something like
> > > WallpaperDelegate instead.
> > > 
> > > b) if customized wallpaper is downloaded later, a new
> > > DesktopBackgroundController::UpdateDefaultWallpaper() method is called,
> which
> > > triggers the same code path from (a).
> > > 
> > > ?
> > 
> > Yes, and this is exactly what I was trying to implement ;-)
> > I do not know the exact creation order of DesktopBackgroundController /
> > WallpaperManager / CustomizationDocument. They all seem to me independent.
And
> I
> > didn't find any "Init" method in DesktopBackgroundController. That is why
I've
> > added lazy initialization with flag "customization_applied_" to be sure to
> call
> > it before setting the wallpapaper, but after all required objects have been
> > created.
> >  
> > 
> > Your Cl seems better (as you know DesktopBackgroundController better). The
> only
> > thing that seems strange to me is that the checks in
GetDefaultWallpaperInfo()
> > differ from DefaultWallpaperIsAlreadyLoadingOrLoaded(). You've recently
fixed
> > the races problem with this check when changing user wallapaper has been
speed
> > up. I think that this might lead to problems if we allow different "default"
> > wallpapers for different users (we do allow different versions of
> customization
> > manifest for different users, we just do not allow different "default"
> > wallpapers for now). Because in multiprofile mode switching between users
can
> be
> > very fast.
> > 
> > And I also should know (by tomorrow) which delegate to extend. I would be
very
> > nice if you could tell me which one to modify.
> 
> i'm glad we agree. :-)
> 
> the different-default-wallpapers-for-different-users thing is interesting.
> what's the motivation behind supporting different versions of the
customization
> manifest for each user? in the current pre-customization world, i believe that
> we just have the concept of a single "default" wallpaper, e.g. if you've never
> selected a wallpaper, you'll see a different image on lumpy vs. other machines
> that have OEM customization of the default wallpaper, like spring. is there
any
> way for a user to switch back to the default after they've changed the
> wallpaper?

It sepends on the notion of "User". If you mean Profile instance - no there is
no way to get back. If it is a real human using the device, in multiprofile
world
we a going towards switching between completely different "defaults" by pressing
"Ctrl+.".
So it seems we are going towards not having any real "defaults" in backends.


> i suspect that WallpaperManager is instantiated in
> ChromeBrowserMainPartsChromeos::PreProfileInit() (it's hard to be certain
since
> it's a lazily-initialized singleton). ash::Shell looks like it's initialized
> slightly later in the same method, so i don't think you'll have trouble with
> WallpaperManager not existing when DesktopBackgroundController is initialized.
> you could tell for sure by adding some logging statements to both classes'
> constructors. it would safer for DesktopBackgroundController take a delegate
> (implemented by WallpaperManager) in its c'tor so that the dependency is
> explicit.

OK. I have moved Default Wallpaper Path initialization to Wallpaper Manager.

> i don't think that adding a DesktopBackgroundController::Init() method would
> accomplish anything; it's owned by ash::Shell, which generally both constructs
> and initializes objects in its own Init() method.
> 
> also note that DesktopBackgroundController doesn't actually do anything until
> SetDefaultWallpaper() or SetCustomWallpaper() has been called, so i would
expect
> it to be safe to create DBC even before you know for sure which wallpaper you
> want to start with.
> 
> i don't understand the comment about the checks in GetDefaultWallpaperInfo()
and
> DefaultWallpaperIsAlreadyLoadingOrLoaded() differing. there shouldn't be any
> checks in GetDefault(); i just added it to unduplicate some code that decides
> which default wallpaper we should use at a given time.
> DefaultWallpaperIsAlreadyLoadingOrLoaded() checks whether the passed-in
> wallpaper is already loading or loaded.

But the only place where GetDefaultWallpaperInfo() is used in you code is where
you
check that current wallpaper is default and we need to call
SetDefaultWallpaper() to
replace it with new "default". My point is that:

--------------------------
GetDefaultWallpaperInfo(is_guest, &old_path, &old_layout, NULL, NULL);
if (DefaultWallpaperIsAlreadyLoadingOrLoaded(old_path, old_layout)) {
  SetDefaultWallpaper(is_guest);
}
--------------------------

if not equivalent to
--------------------------
if (DefaultWallpaperIsAlreadyLoadingOrLoaded(old_path_large,
        IDR_AURA_WALLPAPER_DEFAULT_LARGE) ||
    DefaultWallpaperIsAlreadyLoadingOrLoaded(old_path_small,
        IDR_AURA_WALLPAPER_DEFAULT_SMALL) {
  SetDefaultWallpaper(false);
}
--------------------------

and I am afraid of races here.

So I've just added DefaultWallpaperFileIsAlreadyLoadingOrLoaded() to
check for only file path.

Ok, if adding DefaultWallpaperFileIsAlreadyLoadingOrLoaded() doesn't seem good,
I've changed the code back to the old efaultWallpaperIsAlreadyLoadingOrLoaded().

> my main concern is that DesktopBackgroundController doesn't get more
complicated
> to support this feature. it shouldn't need any more changes than what's
> necessary to support the default wallpaper being changed instead of remaining
> constant.
> 
> here's a perhaps even simpler change:
> 
> - change DesktopBackgroundController::SetDefaultWallpaper() to have the
> following signature:
> 
>   bool SetDefaultWallpaper(const base::FilePath& small_path,
>                            const base::FilePath& large_path);
> 
> - move all of the default-vs.-guest code and switch-vs.-customization-manifest
> code into WallpaperManager and make it just pass the appropriate images when
> calling SetDefaultWallpaper().
> 
> - leave the rest of SetDefaultWallpaper() unchanged. i think that
> DefaultWallpaperIsAlreadyLoadingOrLoaded should already do what you need it to
> do (i.e. if you pass an updated path, it'll return false).
> 
> this way you avoid adding any dependencies from DesktopBackgroundController
back
> to c/b/chromeos/login, and i don't think you'll need to update the delegate,
and
> you shouldn't need to make any major changes to DBC, and you can just call
> SetDefaultWallpaper() again with the updated paths after you've downloaded the
> images. this seems much cleaner and simpler. am i missing something?
> 
> if you go this route, you may need to update any non-chrome-os platforms that
> are using ash. i don't know where their wallpaper currently comes from.
> 
> side observation: it is unfortunate that this code is being written just a few
> days before the branch point. :-(
> 
> > > > > i think i'm also going to rename
> > > > > DesktopBackgroundController::SetCustomWallpaper() to
SetUserWallpaper()
> > > since
> > > > > we're using "custom" to refer to two different things now.

Alexander Alekseev

I have removed DesktopBackgroundController::DefaultWallpaperFileIsAlreadyLoadingOrLoaded(). Would you accept this change?

6 years, 9 months ago (2014-03-25 15:53:02 UTC) #24

Daniel Erat

On 2014/03/25 15:53:02, alemate wrote: > I have removed > DesktopBackgroundController::DefaultWallpaperFileIsAlreadyLoadingOrLoaded(). > Would you accept ...

6 years, 9 months ago (2014-03-25 15:56:09 UTC) #25

Alexander Alekseev

On 2014/03/25 15:36:26, Daniel Erat wrote: > did you see the long comment that i ...

6 years, 9 months ago (2014-03-25 15:58:43 UTC) #26

Daniel Erat

On 2014/03/25 15:58:43, alemate wrote: > On 2014/03/25 15:36:26, Daniel Erat wrote: > > did ...

6 years, 9 months ago (2014-03-25 16:02:50 UTC) #27

On 2014/03/25 15:58:43, alemate wrote:
> On 2014/03/25 15:36:26, Daniel Erat wrote:
> > did you see the long comment that i wrote suggesting what seems like a
> > much-cleaner and simpler way of doing the DesktopBackgroundController stuff?
> is
> > there some reason that that won't work?
> 
> You mean this:
> 
> > - move all of the default-vs.-guest code and                                
 
> > switch-vs.-customization-manifest code into WallpaperManager and make it
just
> > pass the appropriate images when calling SetDefaultWallpaper().             
 
> >                                                                             
 
> > - leave the rest of SetDefaultWallpaper() unchanged. i think that           
 
> > DefaultWallpaperIsAlreadyLoadingOrLoaded should already do what you need it 
 
> > to do (i.e. if you pass an updated path, it'll return false).               
 
> >                                                                             
 
> > this way you avoid adding any dependencies from DesktopBackgroundController 
 
> > back to c/b/chromeos/login, and i don't think you'll need to update the     
 
> > delegate, and you shouldn't need to make any major changes to DBC, and you  
 
> > can just call SetDefaultWallpaper() again with the updated paths after
you've 
> > downloaded the images. this seems much cleaner and simpler. am i missing    
 
> > something?                                                                  
 
> >                                                                             
 
> > if you go this route, you may need to update any non-chrome-os platforms
that 
> > are using ash. i don't know where their wallpaper currently comes from. 
> 
> It will work, and I can probably implement it. Though it would be larger
> change in terms of number of updated lines in DBC / WallpaperManager.
> 
> Should I implement it?

yes, please do -- i think that this will leave the code in substantially cleaner
shape than other approaches would, since all of the default-wallpaper-choosing
code should be in one place instead of being distributed across DBC and
c/b/c/login.

the only changes to DBC should be deletions, right? i think that all of the
existing code will continue to work correctly if SetDefaultWallpaper() is
updated to take the image paths as arguments.

Alexander Alekseev

I've moved all wallpaper paths to WallpaperManager. Please review. (I've added back public DefaultWallpaperFileIsAlreadyLoadingOrLoaded(path) to ...

6 years, 9 months ago (2014-03-25 18:22:58 UTC) #28

Daniel Erat

https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/desktop_background_controller.cc File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/desktop_background_controller.cc#newcode324 ash/desktop_background/desktop_background_controller.cc:324: bool DesktopBackgroundController::DefaultWallpaperFileIsAlreadyLoadingOrLoaded( i'm sorry, but i still don't understand ...

6 years, 9 months ago (2014-03-25 18:32:54 UTC) #29

Alexander Alekseev

https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/desktop_background_controller.cc File ash/desktop_background/desktop_background_controller.cc (right): https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/desktop_background_controller.cc#newcode324 ash/desktop_background/desktop_background_controller.cc:324: bool DesktopBackgroundController::DefaultWallpaperFileIsAlreadyLoadingOrLoaded( On 2014/03/25 18:32:54, Daniel Erat wrote: > ...

6 years, 9 months ago (2014-03-25 19:09:33 UTC) #31

Daniel Erat

On 2014/03/25 19:09:33, alemate wrote: > https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/desktop_background_controller.cc > File ash/desktop_background/desktop_background_controller.cc (right): > > https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/desktop_background_controller.cc#newcode324 > ...

6 years, 9 months ago (2014-03-25 19:55:01 UTC) #32

Alexander Alekseev

On 2014/03/25 19:55:01, Daniel Erat wrote: > On 2014/03/25 19:09:33, alemate wrote: > > > ...

6 years, 9 months ago (2014-03-25 20:08:31 UTC) #33

Daniel Erat

On 2014/03/25 20:08:31, alemate wrote: > On 2014/03/25 19:55:01, Daniel Erat wrote: > > On ...

6 years, 9 months ago (2014-03-25 20:12:28 UTC) #34

On 2014/03/25 20:08:31, alemate wrote:
> On 2014/03/25 19:55:01, Daniel Erat wrote:
> > On 2014/03/25 19:09:33, alemate wrote:
> > >
> >
>
https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/...
> > > File ash/desktop_background/desktop_background_controller.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/...
> > > ash/desktop_background/desktop_background_controller.cc:324: bool
> > > DesktopBackgroundController::DefaultWallpaperFileIsAlreadyLoadingOrLoaded(
> > > On 2014/03/25 18:32:54, Daniel Erat wrote:
> > > > i'm sorry, but i still don't understand why you need this method. can
you
> > > > describe the specific sequence of events that you're concerned wouldn't
be
> > > > handled correctly if you use the existing method that additionally
checks
> > the
> > > > resource ID?
> > > 
> > > It will work with existing method, but I will have to call it in
> > > WallpaperManager like:
> > > 
> > > const bool need_reset =
> > >       DefaultWallpaperIsAlreadyLoadingOrLoaded(
> > >           default_small_wallpaper_file_,
> > >           IDR_AURA_WALLPAPER_DEFAULT_SMALL) ||
> > >      DefaultWallpaperIsAlreadyLoadingOrLoaded(
> > >           default_large_wallpaper_file_,
> > >           IDR_AURA_WALLPAPER_DEFAULT_LARGE);
> > > 
> > > So the idea was not to expose IDR_AURA_* constants (and DBC implementation
> > > details) outside of ash:: . If you think it's OK, I can reuse existing.
> > 
> > why does WallpaperManager need to call it? SetDefaultWallpaper() already
calls
> > it before loading anything -- isn't that enough?
> 
> Because I need to check that old default wallpaper is shown before making
> changes:
> 
> void SetDefaultWallpaperPath(...) {
>   bool default_wallpaper_was_in_use = dbc->is_default_wallpaper_in_use();
>   ... // set default paths here.
>   if (default_wallpaper_was_in_use)
>     set_default_wallpaper(...); // Update wallpaper.
> }

isn't WallpaperManager already in a good position to know whether it previously
requested default vs. user wallpaper?

Daniel Erat

On 2014/03/25 20:12:28, Daniel Erat wrote: > On 2014/03/25 20:08:31, alemate wrote: > > On ...

6 years, 9 months ago (2014-03-25 20:18:50 UTC) #35

On 2014/03/25 20:12:28, Daniel Erat wrote:
> On 2014/03/25 20:08:31, alemate wrote:
> > On 2014/03/25 19:55:01, Daniel Erat wrote:
> > > On 2014/03/25 19:09:33, alemate wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/...
> > > > File ash/desktop_background/desktop_background_controller.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/...
> > > > ash/desktop_background/desktop_background_controller.cc:324: bool
> > > >
DesktopBackgroundController::DefaultWallpaperFileIsAlreadyLoadingOrLoaded(
> > > > On 2014/03/25 18:32:54, Daniel Erat wrote:
> > > > > i'm sorry, but i still don't understand why you need this method. can
> you
> > > > > describe the specific sequence of events that you're concerned
wouldn't
> be
> > > > > handled correctly if you use the existing method that additionally
> checks
> > > the
> > > > > resource ID?
> > > > 
> > > > It will work with existing method, but I will have to call it in
> > > > WallpaperManager like:
> > > > 
> > > > const bool need_reset =
> > > >       DefaultWallpaperIsAlreadyLoadingOrLoaded(
> > > >           default_small_wallpaper_file_,
> > > >           IDR_AURA_WALLPAPER_DEFAULT_SMALL) ||
> > > >      DefaultWallpaperIsAlreadyLoadingOrLoaded(
> > > >           default_large_wallpaper_file_,
> > > >           IDR_AURA_WALLPAPER_DEFAULT_LARGE);
> > > > 
> > > > So the idea was not to expose IDR_AURA_* constants (and DBC
implementation
> > > > details) outside of ash:: . If you think it's OK, I can reuse existing.
> > > 
> > > why does WallpaperManager need to call it? SetDefaultWallpaper() already
> calls
> > > it before loading anything -- isn't that enough?
> > 
> > Because I need to check that old default wallpaper is shown before making
> > changes:
> > 
> > void SetDefaultWallpaperPath(...) {
> >   bool default_wallpaper_was_in_use = dbc->is_default_wallpaper_in_use();
> >   ... // set default paths here.
> >   if (default_wallpaper_was_in_use)
> >     set_default_wallpaper(...); // Update wallpaper.
> > }
> 
> isn't WallpaperManager already in a good position to know whether it
previously
> requested default vs. user wallpaper?

alternately, if this is difficult to do for some reason, i wouldn't object to a
tiny new method in DBC along the lines of this:

  bool IsUsingDefaultWallpaper() const {
    return !current_default_wallpaper_path_.empty() ||
        default_wallpaper_loader_;
  }

Alexander Alekseev

On 2014/03/25 20:12:28, Daniel Erat wrote: > On 2014/03/25 20:08:31, alemate wrote: > > On ...

6 years, 9 months ago (2014-03-25 20:27:15 UTC) #36

On 2014/03/25 20:12:28, Daniel Erat wrote:
> On 2014/03/25 20:08:31, alemate wrote:
> > On 2014/03/25 19:55:01, Daniel Erat wrote:
> > > On 2014/03/25 19:09:33, alemate wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/...
> > > > File ash/desktop_background/desktop_background_controller.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/...
> > > > ash/desktop_background/desktop_background_controller.cc:324: bool
> > > >
DesktopBackgroundController::DefaultWallpaperFileIsAlreadyLoadingOrLoaded(
> > > > On 2014/03/25 18:32:54, Daniel Erat wrote:
> > > > > i'm sorry, but i still don't understand why you need this method. can
> you
> > > > > describe the specific sequence of events that you're concerned
wouldn't
> be
> > > > > handled correctly if you use the existing method that additionally
> checks
> > > the
> > > > > resource ID?
> > > > 
> > > > It will work with existing method, but I will have to call it in
> > > > WallpaperManager like:
> > > > 
> > > > const bool need_reset =
> > > >       DefaultWallpaperIsAlreadyLoadingOrLoaded(
> > > >           default_small_wallpaper_file_,
> > > >           IDR_AURA_WALLPAPER_DEFAULT_SMALL) ||
> > > >      DefaultWallpaperIsAlreadyLoadingOrLoaded(
> > > >           default_large_wallpaper_file_,
> > > >           IDR_AURA_WALLPAPER_DEFAULT_LARGE);
> > > > 
> > > > So the idea was not to expose IDR_AURA_* constants (and DBC
implementation
> > > > details) outside of ash:: . If you think it's OK, I can reuse existing.
> > > 
> > > why does WallpaperManager need to call it? SetDefaultWallpaper() already
> calls
> > > it before loading anything -- isn't that enough?
> > 
> > Because I need to check that old default wallpaper is shown before making
> > changes:
> > 
> > void SetDefaultWallpaperPath(...) {
> >   bool default_wallpaper_was_in_use = dbc->is_default_wallpaper_in_use();
> >   ... // set default paths here.
> >   if (default_wallpaper_was_in_use)
> >     set_default_wallpaper(...); // Update wallpaper.
> > }
> 
> isn't WallpaperManager already in a good position to know whether it
previously
> requested default vs. user wallpaper?

Probably. But doing this in a fair way would require removal of
DBC::DefaultWallpaperIsAlreadyLoadingOrLoaded() completely and much more
refactoring that I was trying to avoid ;-)

Let's add new feature (customized default wallpapers) as it is described in
design document: "Just replace current wallpaper path with new one, which was
downloaded."
I hope there is someone who knows WallpaperManager better and can refactor it to
a better design. ;-)

> alternately, if this is difficult to do for some reason, i wouldn't object to
a
> tiny new method in DBC along the lines of this:
> 
>   bool IsUsingDefaultWallpaper() const {
>     return !current_default_wallpaper_path_.empty() ||
>         default_wallpaper_loader_;
>   }


But this is doesn't match the checks in
DefaultWallpaperIsAlreadyLoadingOrLoaded() ! Is this correct?

May be I should can implement it like this:

bool IsUsingDefaultWallpaper(path_small, path_large) const {
  return DefaultWallpaperIsAlreadyLoadingOrLoaded(path_small,
IDR_AURA_WALLPAPER_DEFAULT_SMALL) ||
      DefaultWallpaperIsAlreadyLoadingOrLoaded(path_large,
IDR_AURA_WALLPAPER_DEFAULT_LARGE);
}

Daniel Erat

On 2014/03/25 20:27:15, alemate wrote: > On 2014/03/25 20:12:28, Daniel Erat wrote: > > On ...

6 years, 9 months ago (2014-03-25 20:52:30 UTC) #37

On 2014/03/25 20:27:15, alemate wrote:
> On 2014/03/25 20:12:28, Daniel Erat wrote:
> > On 2014/03/25 20:08:31, alemate wrote:
> > > On 2014/03/25 19:55:01, Daniel Erat wrote:
> > > > On 2014/03/25 19:09:33, alemate wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/...
> > > > > File ash/desktop_background/desktop_background_controller.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/208273005/diff/220001/ash/desktop_background/...
> > > > > ash/desktop_background/desktop_background_controller.cc:324: bool
> > > > >
> DesktopBackgroundController::DefaultWallpaperFileIsAlreadyLoadingOrLoaded(
> > > > > On 2014/03/25 18:32:54, Daniel Erat wrote:
> > > > > > i'm sorry, but i still don't understand why you need this method.
can
> > you
> > > > > > describe the specific sequence of events that you're concerned
> wouldn't
> > be
> > > > > > handled correctly if you use the existing method that additionally
> > checks
> > > > the
> > > > > > resource ID?
> > > > > 
> > > > > It will work with existing method, but I will have to call it in
> > > > > WallpaperManager like:
> > > > > 
> > > > > const bool need_reset =
> > > > >       DefaultWallpaperIsAlreadyLoadingOrLoaded(
> > > > >           default_small_wallpaper_file_,
> > > > >           IDR_AURA_WALLPAPER_DEFAULT_SMALL) ||
> > > > >      DefaultWallpaperIsAlreadyLoadingOrLoaded(
> > > > >           default_large_wallpaper_file_,
> > > > >           IDR_AURA_WALLPAPER_DEFAULT_LARGE);
> > > > > 
> > > > > So the idea was not to expose IDR_AURA_* constants (and DBC
> implementation
> > > > > details) outside of ash:: . If you think it's OK, I can reuse
existing.
> > > > 
> > > > why does WallpaperManager need to call it? SetDefaultWallpaper() already
> > calls
> > > > it before loading anything -- isn't that enough?
> > > 
> > > Because I need to check that old default wallpaper is shown before making
> > > changes:
> > > 
> > > void SetDefaultWallpaperPath(...) {
> > >   bool default_wallpaper_was_in_use = dbc->is_default_wallpaper_in_use();
> > >   ... // set default paths here.
> > >   if (default_wallpaper_was_in_use)
> > >     set_default_wallpaper(...); // Update wallpaper.
> > > }
> > 
> > isn't WallpaperManager already in a good position to know whether it
> previously
> > requested default vs. user wallpaper?
> 
> Probably. But doing this in a fair way would require removal of
> DBC::DefaultWallpaperIsAlreadyLoadingOrLoaded() completely and much more
> refactoring that I was trying to avoid ;-)

why would it require DefaultWallpaperIsAlreadyLoadingOrLoaded() to be removed?

> Let's add new feature (customized default wallpapers) as it is described in
> design document: "Just replace current wallpaper path with new one, which was
> downloaded."
> I hope there is someone who knows WallpaperManager better and can refactor it
to
> a better design. ;-)
> 
> > alternately, if this is difficult to do for some reason, i wouldn't object
to
> a
> > tiny new method in DBC along the lines of this:
> > 
> >   bool IsUsingDefaultWallpaper() const {
> >     return !current_default_wallpaper_path_.empty() ||
> >         default_wallpaper_loader_;
> >   }
> 
> 
> But this is doesn't match the checks in
> DefaultWallpaperIsAlreadyLoadingOrLoaded() ! Is this correct?

i feel like we're talking about different things. backing up:

a) DesktopBackgroundController is responsible for displaying wallpaper images on
the desktop.
b) it supports just two operations: loading trusted "default" images from disk
(SetDefaultWallpaper()), and displaying already-decoded "user" images
(SetCustomWallpaper()).
c) DefaultWallpaperIsAlreadyLoadingOrLoaded() and
CustomWallpaperIsAlreadyLoaded() are just private, internal implementation
details that are used to avoid reloading already-loading or already-loaded
wallpaper (which WallpaperManager ought not ask DBC to do, but does).
d) right now, the wallpaper used by SetDefaultWallpaper() is hardcoded, but i'm
suggesting updating SetDefaultWallpaper() to take caller-supplied paths.
e) SetDefaultWallpaper() will still be able to tell whether the wallpaper that
was passed to it is already loading/loaded or not, though. callers shouldn't
need to check this themselves.
f) since you're fetching the custom manifest in the background, it may finish
loading while DBC is displaying user wallpaper rather than default wallpaper.
you only want to call SetDefaultWallpaper() when default wallpaper is already
shown, right?
g) the IsUsingDefaultWallpaper() implementation that i suggested above should be
enough to determine whether any (previous, current, etc. -- it shouldn't matter)
default wallpaper is loading or loaded.

happy to VC if any of the above doesn't make sense.

> May be I should can implement it like this:
> 
> bool IsUsingDefaultWallpaper(path_small, path_large) const {
>   return DefaultWallpaperIsAlreadyLoadingOrLoaded(path_small,
> IDR_AURA_WALLPAPER_DEFAULT_SMALL) ||
>       DefaultWallpaperIsAlreadyLoadingOrLoaded(path_large,
> IDR_AURA_WALLPAPER_DEFAULT_LARGE);
> }

Alexander Alekseev

I've added DesktopBackgroundController::IsUsingDefaultWallpaper(). (It works, thank you, Daniel!) I've also added "retry failed wallpaper fetch ...

6 years, 9 months ago (2014-03-25 21:27:19 UTC) #38

Daniel Erat

thanks! the ash changes look fine to me now. i glanced at the c/b/c/login code ...

6 years, 9 months ago (2014-03-25 21:58:28 UTC) #39

Dmitry Polukhin

The URL comes from customization manifest that is fetched over HTTPS from Google server. So ...

6 years, 9 months ago (2014-03-25 22:06:51 UTC) #40

The URL comes from customization manifest that is fetched over HTTPS from
Google server. So it should be safe. But because these files are writable
by Chrome we have to use sandboxed decoder as we do for user wallpapers. I
think we should use the same code path as we do for wallpapers that are not
on read-only roots.

On Tue, Mar 25, 2014 at 2:58 PM, <derat@chromium.org> wrote:

> thanks! the ash changes look fine to me now.
>
> i glanced at the c/b/c/login code and added some comments, but somebody
> more
> familiar with it should do a full review.
>
> a few security-related questions, since DesktopBackgroundController's image
> decoder isn't sandboxed:
>
> a) where will the URLs containing the manifest and the wallpaper image come
> from? do the URLs live somewhere in the readonly partition?
>

> b) are the URLs all https?
>
> c) where do the downloaded and reencoded images get saved? they'll need to
> be
> writable by the chronos user, so i'm concerned that a malicious user could
> overwrite them and use them to compromise the pre-login browser the next
> time it
> gets restarted. perhaps DesktopBackgroundController needs to be updated to
> use a
> sandboxed decoder. (this may be easy to do.)
>
>
> https://codereview.chromium.org/208273005/diff/460001/ash/
> desktop_background/desktop_background_controller_unittest.cc
> File ash/desktop_background/desktop_background_controller_unittest.cc
> (right):
>
> https://codereview.chromium.org/208273005/diff/460001/ash/
> desktop_background/desktop_background_controller_unittest.cc#newcode231
> ash/desktop_background/desktop_background_controller_unittest.cc:231:
> void WriteWallpapersAndSetFlags() {
> nit: rename this to something like WriteWallpapers() since it's no
> longer setting any flags. also update its comment.
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/customization_document.cc
> File chrome/browser/chromeos/customization_document.cc (right):
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/customization_document.cc#newcode602
> chrome/browser/chromeos/customization_document.cc:602: PrefService*
> prefService = g_browser_process->local_state();
> s/prefService/pref_service/
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/customization_document.h
> File chrome/browser/chromeos/customization_document.h (right):
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/customization_document.h#newcode181
> chrome/browser/chromeos/customization_document.h:181: // This is also
> used by Wallpaper manager to set customized wallpaper path
> nit: s/Wallpaper manager/WallpaperManager/
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/customization_document.h#newcode182
> chrome/browser/chromeos/customization_document.h:182: // to global
> default before first wallpaper was shown.
> nit: s/was/is/
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/customization_wallpaper_downloader.cc
> File chrome/browser/chromeos/customization_wallpaper_downloader.cc
> (right):
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/customization_wallpaper_downloader.cc#newcode100
> chrome/browser/chromeos/customization_wallpaper_downloader.cc:100:
>
> DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
> have you run this in debug mode? i didn't think that this check succeeds
> -- last i knew, the blocking pool threads and the file thread aren't the
> same thing.
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/customization_wallpaper_downloader.cc#newcode105
> chrome/browser/chromeos/customization_wallpaper_downloader.cc:105:
> *success = true;
> nit:
>
>   *success = CreateDirectory...();
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.cc
> File chrome/browser/chromeos/login/wallpaper_manager.cc (right):
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.cc#newcode134
> chrome/browser/chromeos/login/wallpaper_manager.cc:134: namespace
> chromeos {
> nit: put the above anonymous namespace within the chromeos namespace so
> you don't need to use chromeos:: references within it
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1593
> chrome/browser/chromeos/login/wallpaper_manager.cc:1593: // Re-encode
> orginal file to jpeg format and save the result for debug
> i don't understand the "for debug" here. how likely is corruption? if
> you're really reencoding the file as jpeg, what makes you think that
> it'll match the original bytes?
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1596
> chrome/browser/chromeos/login/wallpaper_manager.cc:1596: *success |=
> ResizeAndSaveWallpaper(wallpaper,
> shouldn't all of the resizes need to succeed for this to be considered
> successful, rather than just one of them?
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1626
> chrome/browser/chromeos/login/wallpaper_manager.cc:1626: PrefService*
> prefService = g_browser_process->local_state();
> s/prefService/pref_service/
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1677
> chrome/browser/chromeos/login/wallpaper_manager.cc:1677: PrefService*
> prefService = g_browser_process->local_state();
> s/prefService/pref_service/
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1681
> chrome/browser/chromeos/login/wallpaper_manager.cc:1681: if
> ((current_url != wallpaper_url.spec()) || (!exist->AllRescaledExist()))
> {
> nit: remove unnecessary parentheses
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1699
> chrome/browser/chromeos/login/wallpaper_manager.cc:1699: PrefService*
> prefService = g_browser_process->local_state();
> s/prefService/pref_service/
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1722
> chrome/browser/chromeos/login/wallpaper_manager.cc:1722: const bool
> need_reset = dbc->IsUsingDefaultWallpaper();
> nit: you can just inline this below now:
>
>   if (dbc->IsUsingDefaultWallpaper()) {
>     dbc->SetDefaultWallpaper(...);
>   }
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.h
> File chrome/browser/chromeos/login/wallpaper_manager.h (right):
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.h#newcode91
> chrome/browser/chromeos/login/wallpaper_manager.h:91: bool dowloaded;
> s/dowloaded/downloaded/
>
> https://codereview.chromium.org/208273005/diff/460001/
> chrome/browser/chromeos/login/wallpaper_manager.h#newcode336
> chrome/browser/chromeos/login/wallpaper_manager.h:336: //
> scaled_directory - directory where resized versions are stored
> this comment seems outdated
>
> https://codereview.chromium.org/208273005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Daniel Erat

On 2014/03/25 22:06:51, Dmitry Polukhin wrote: > The URL comes from customization manifest that is ...

6 years, 9 months ago (2014-03-25 22:11:00 UTC) #41

On 2014/03/25 22:06:51, Dmitry Polukhin wrote:
> The URL comes from customization manifest that is fetched over HTTPS from
> Google server. So it should be safe. But because these files are writable
> by Chrome we have to use sandboxed decoder as we do for user wallpapers. I
> think we should use the same code path as we do for wallpapers that are not
> on read-only roots.

hmm, i was wondering about that. in that case, WallpaperManager would just call
SetCustomWallpaper() instead of SetDefaultWallpaper() to use the customized
images, and DesktopBackgroundController() wouldn't change at all. we'd probably
want to avoid doing any resizing before saving to disk in that case (since
SetCustomWallpaper() doesn't take multiple differently-sized images).

> On Tue, Mar 25, 2014 at 2:58 PM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=derat@chromium.org> wrote:
> 
> > thanks! the ash changes look fine to me now.
> >
> > i glanced at the c/b/c/login code and added some comments, but somebody
> > more
> > familiar with it should do a full review.
> >
> > a few security-related questions, since DesktopBackgroundController's image
> > decoder isn't sandboxed:
> >
> > a) where will the URLs containing the manifest and the wallpaper image come
> > from? do the URLs live somewhere in the readonly partition?
> >
> 
> > b) are the URLs all https?
> >
> > c) where do the downloaded and reencoded images get saved? they'll need to
> > be
> > writable by the chronos user, so i'm concerned that a malicious user could
> > overwrite them and use them to compromise the pre-login browser the next
> > time it
> > gets restarted. perhaps DesktopBackgroundController needs to be updated to
> > use a
> > sandboxed decoder. (this may be easy to do.)
> >
> >
> > https://codereview.chromium.org/208273005/diff/460001/ash/
> > desktop_background/desktop_background_controller_unittest.cc
> > File ash/desktop_background/desktop_background_controller_unittest.cc
> > (right):
> >
> > https://codereview.chromium.org/208273005/diff/460001/ash/
> > desktop_background/desktop_background_controller_unittest.cc#newcode231
> > ash/desktop_background/desktop_background_controller_unittest.cc:231:
> > void WriteWallpapersAndSetFlags() {
> > nit: rename this to something like WriteWallpapers() since it's no
> > longer setting any flags. also update its comment.
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/customization_document.cc
> > File chrome/browser/chromeos/customization_document.cc (right):
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/customization_document.cc#newcode602
> > chrome/browser/chromeos/customization_document.cc:602: PrefService*
> > prefService = g_browser_process->local_state();
> > s/prefService/pref_service/
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/customization_document.h
> > File chrome/browser/chromeos/customization_document.h (right):
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/customization_document.h#newcode181
> > chrome/browser/chromeos/customization_document.h:181: // This is also
> > used by Wallpaper manager to set customized wallpaper path
> > nit: s/Wallpaper manager/WallpaperManager/
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/customization_document.h#newcode182
> > chrome/browser/chromeos/customization_document.h:182: // to global
> > default before first wallpaper was shown.
> > nit: s/was/is/
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/customization_wallpaper_downloader.cc
> > File chrome/browser/chromeos/customization_wallpaper_downloader.cc
> > (right):
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/customization_wallpaper_downloader.cc#newcode100
> > chrome/browser/chromeos/customization_wallpaper_downloader.cc:100:
> >
> > DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
> > have you run this in debug mode? i didn't think that this check succeeds
> > -- last i knew, the blocking pool threads and the file thread aren't the
> > same thing.
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/customization_wallpaper_downloader.cc#newcode105
> > chrome/browser/chromeos/customization_wallpaper_downloader.cc:105:
> > *success = true;
> > nit:
> >
> >   *success = CreateDirectory...();
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.cc
> > File chrome/browser/chromeos/login/wallpaper_manager.cc (right):
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.cc#newcode134
> > chrome/browser/chromeos/login/wallpaper_manager.cc:134: namespace
> > chromeos {
> > nit: put the above anonymous namespace within the chromeos namespace so
> > you don't need to use chromeos:: references within it
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1593
> > chrome/browser/chromeos/login/wallpaper_manager.cc:1593: // Re-encode
> > orginal file to jpeg format and save the result for debug
> > i don't understand the "for debug" here. how likely is corruption? if
> > you're really reencoding the file as jpeg, what makes you think that
> > it'll match the original bytes?
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1596
> > chrome/browser/chromeos/login/wallpaper_manager.cc:1596: *success |=
> > ResizeAndSaveWallpaper(wallpaper,
> > shouldn't all of the resizes need to succeed for this to be considered
> > successful, rather than just one of them?
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1626
> > chrome/browser/chromeos/login/wallpaper_manager.cc:1626: PrefService*
> > prefService = g_browser_process->local_state();
> > s/prefService/pref_service/
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1677
> > chrome/browser/chromeos/login/wallpaper_manager.cc:1677: PrefService*
> > prefService = g_browser_process->local_state();
> > s/prefService/pref_service/
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1681
> > chrome/browser/chromeos/login/wallpaper_manager.cc:1681: if
> > ((current_url != wallpaper_url.spec()) || (!exist->AllRescaledExist()))
> > {
> > nit: remove unnecessary parentheses
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1699
> > chrome/browser/chromeos/login/wallpaper_manager.cc:1699: PrefService*
> > prefService = g_browser_process->local_state();
> > s/prefService/pref_service/
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.cc#newcode1722
> > chrome/browser/chromeos/login/wallpaper_manager.cc:1722: const bool
> > need_reset = dbc->IsUsingDefaultWallpaper();
> > nit: you can just inline this below now:
> >
> >   if (dbc->IsUsingDefaultWallpaper()) {
> >     dbc->SetDefaultWallpaper(...);
> >   }
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.h
> > File chrome/browser/chromeos/login/wallpaper_manager.h (right):
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.h#newcode91
> > chrome/browser/chromeos/login/wallpaper_manager.h:91: bool dowloaded;
> > s/dowloaded/downloaded/
> >
> > https://codereview.chromium.org/208273005/diff/460001/
> > chrome/browser/chromeos/login/wallpaper_manager.h#newcode336
> > chrome/browser/chromeos/login/wallpaper_manager.h:336: //
> > scaled_directory - directory where resized versions are stored
> > this comment seems outdated
> >
> > https://codereview.chromium.org/208273005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....

Alexander Alekseev

I've removed DBC::SetDefaultWallpaper. Please review. https://codereview.chromium.org/208273005/diff/460001/ash/desktop_background/desktop_background_controller_unittest.cc File ash/desktop_background/desktop_background_controller_unittest.cc (right): https://codereview.chromium.org/208273005/diff/460001/ash/desktop_background/desktop_background_controller_unittest.cc#newcode231 ash/desktop_background/desktop_background_controller_unittest.cc:231: void WriteWallpapersAndSetFlags() { On ...

6 years, 8 months ago (2014-03-27 00:28:36 UTC) #42

Daniel Erat

https://codereview.chromium.org/208273005/diff/460001/chrome/browser/chromeos/customization_wallpaper_downloader.cc File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right): https://codereview.chromium.org/208273005/diff/460001/chrome/browser/chromeos/customization_wallpaper_downloader.cc#newcode100 chrome/browser/chromeos/customization_wallpaper_downloader.cc:100: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); On 2014/03/27 00:28:37, alemate wrote: > On 2014/03/25 ...

6 years, 8 months ago (2014-03-27 01:22:30 UTC) #43

Daniel Erat

here are some initial comments; i didn't make it very far through c/b/chromeos. this is ...

6 years, 8 months ago (2014-03-27 01:45:49 UTC) #44

Daniel Erat

On 2014/03/27 01:45:49, Daniel Erat wrote: > here are some initial comments; i didn't make ...

6 years, 8 months ago (2014-03-27 01:47:24 UTC) #45

Dmitry Polukhin

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos/customization_document.cc File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos/customization_document.cc#newcode72 chrome/browser/chromeos/customization_document.cc:72: const char kCustomizationDefaultWallpaperDir[] = "customization/"; Please add comment, also ...

6 years, 8 months ago (2014-03-27 04:36:31 UTC) #46

Dmitry Polukhin

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos/login/wizard_controller.cc File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos/login/wizard_controller.cc#newcode320 chrome/browser/chromeos/login/wizard_controller.cc:320: ServicesCustomizationDocument::GetInstance()->StartFetching(); It will always fetch new customization manifest even ...

6 years, 8 months ago (2014-03-27 04:46:10 UTC) #47

Alexander Alekseev

I'm ready to split this into two CL: - DBC + WallpaperManager : move all ...

6 years, 8 months ago (2014-03-31 14:15:39 UTC) #48

I'm ready to split this into two CL:
- DBC + WallpaperManager : move all wallpaper loading to WM.
- ServicesCustomizationDocument : inplement customized wallpaper.

As soon as reviwers agree to split CL, I'll upload two different ones.

https://codereview.chromium.org/208273005/diff/460001/chrome/browser/chromeos...
File chrome/browser/chromeos/customization_wallpaper_downloader.cc (right):

https://codereview.chromium.org/208273005/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_wallpaper_downloader.cc:100:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
On 2014/03/27 01:22:30, Daniel Erat wrote:
> On 2014/03/27 00:28:37, alemate wrote:
> > On 2014/03/25 21:58:29, Daniel Erat wrote:
> > > have you run this in debug mode? i didn't think that this check succeeds
--
> > last
> > > i knew, the blocking pool threads and the file thread aren't the same
thing.
> > 
> > You're right, this doesn't work. I used to think it does.
> > 
> > I couldn't find a way to check we're on a worker pool without any special
> > SequenceID. So I'm checking for "not on UI thread".
> 
> DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

Done.

https://codereview.chromium.org/208273005/diff/670001/ash/desktop_background/...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/208273005/diff/670001/ash/desktop_background/...
ash/desktop_background/desktop_background_controller.cc:53: const int
kWallpaperThumbnailHeight = 68;
On 2014/03/27 01:45:50, Daniel Erat wrote:
> delete these if you don't use them

They are used in extensions/wallpaper_api.cc

https://codereview.chromium.org/208273005/diff/670001/ash/desktop_background/...
ash/desktop_background/desktop_background_controller.cc:57: class
DesktopBackgroundController::WallpaperLoader
On 2014/03/27 01:45:50, Daniel Erat wrote:
> delete WallpaperLoader if you don't use it anymore

Done.

https://codereview.chromium.org/208273005/diff/670001/ash/desktop_background/...
ash/desktop_background/desktop_background_controller.cc:227: void
DesktopBackgroundController::CreateEmptyWallpaper() {
On 2014/03/27 01:45:50, Daniel Erat wrote:
> delete this if it's not called anymore

It's used by Wallpapermanager, API, DefaultDelegate.

https://codereview.chromium.org/208273005/diff/670001/ash/desktop_background/...
ash/desktop_background/desktop_background_controller.cc:232: WallpaperResolution
DesktopBackgroundController::GetAppropriateResolution() {
On 2014/03/27 01:45:50, Daniel Erat wrote:
> delete this if you don't use it

It's used by Wallpapermanager (to select cached wallpaper).

https://codereview.chromium.org/208273005/diff/670001/ash/desktop_background/...
File ash/desktop_background/desktop_background_controller.h (right):

https://codereview.chromium.org/208273005/diff/670001/ash/desktop_background/...
ash/desktop_background/desktop_background_controller.h:95: // Sets the user
selected custom wallpaper. Called when user selected a file
On 2014/03/27 01:45:50, Daniel Erat wrote:
> update this comment if it's no longer only used for user-selected wallpaper

Done.

https://codereview.chromium.org/208273005/diff/670001/ash/desktop_background/...
ash/desktop_background/desktop_background_controller.h:97: void
SetCustomWallpaper(const gfx::ImageSkia& image, WallpaperLayout layout);
On 2014/03/27 01:45:50, Daniel Erat wrote:
> rename this to SetWallpaper()

Done.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
File chrome/browser/chromeos/customization_document.cc (right):

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:72: const char
kCustomizationDefaultWallpaperDir[] = "customization/";
On 2014/03/27 04:36:32, Dmitry Polukhin wrote:
> Please add comment, also please avoid injecting '/', use  AppendRelativePath
> instead.

Done.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:376:
registry->RegisterStringPref(prefs::kCustomizationDefaultWallpaperURL, "");
On 2014/03/27 04:36:32, Dmitry Polukhin wrote:
> I'm bit worry about keeping URL to download file. It could be attack vector to
> replace this URL with some malicious URL in local settings (potential cross
user
> infection path). I think we should re-download customization manifest if fail
> was not download and saved to disk at once. We download customization manifest
> using HTPS from Google server so it is more reliable sources than URL in local
> settings.
This URL is never fetched. It's used as a flag that image by this URL has beed
cached. It's only compared to the URL from manifest.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:489: CheckAndApplyWallpaper();
On 2014/03/27 04:36:32, Dmitry Polukhin wrote:
> Why do you need this??? Wallpaper should be applied only once per machine. But
> this code path will be called for every user on the machine.

Done.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:539: void
ServicesCustomizationDocument::StartOEMWallpaperDownload(
On 2014/03/27 01:45:50, Daniel Erat wrote:
> please make the order of the methods match between .h and .cc files; the
> implementation is difficult to navigate otherwise

Done.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:577: const GURL&
wallpaper_url) {
On 2014/03/27 01:45:50, Daniel Erat wrote:
> why does this need to be a static method?

It just doesn't require access to object. It doesn't need to be static.
Do you think I should still make it "normal" method?

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:592:
ServicesCustomizationDocument::GetInstance();
On 2014/03/27 01:45:50, Daniel Erat wrote:
> why does this need to be a static method? can't the caller just call
> GetInstance() itself?

Done.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:592:
ServicesCustomizationDocument::GetInstance();
On 2014/03/27 04:36:32, Dmitry Polukhin wrote:
> On 2014/03/27 01:45:50, Daniel Erat wrote:
> > why does this need to be a static method? can't the caller just call
> > GetInstance() itself?
> 
> I think we need to use weak pointers that comes from parameter here. Otherwise
> we might face crashes on browser shutdown when ServicesCustomizationDocument
is
> destroyed.

I don't see the way to crash here, as Singleton will be reinitialized on Get().
Also, CustomizedWallpaperDownloader is owned by ServicedCustomizationDocument,
so it should be destroyed before owner.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:600:
DCHECK(wallpaper_url.is_valid());
On 2014/03/27 04:36:32, Dmitry Polukhin wrote:
> Could you please add checks for expect thread for each method that is called
> with PostTask?

All of them already have these checks. ApplyWallpaper is never called with
PostTask.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:650: scoped_ptr<bool>
exists(new bool(false));
On 2014/03/27 04:36:32, Dmitry Polukhin wrote:
> I don't like this approach to me it would be cleaner to pass callback to
> callback that will be called if file exists. scoped_ptr<bool> looks very
> suspicious.

This approach guarantees that WeakPointer never leaves UI thread, so it will be
destroyed on UI thread. Passing callback with WeakPointer to another thread
means it would be once destroyed there.

I can do what you want, but would you accept current approach instead?

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
File chrome/browser/chromeos/customization_document.h (right):

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.h:188: friend class
CustomizationWallpaperDownloader;
On 2014/03/27 04:36:32, Dmitry Polukhin wrote:
> What exactly do you need from private interface of
> ServicesCustomizationDocument? I would prefer to expand public interface in
> controlled way instead of open impl part.

OnCustomizedWallpaperDownloaded();

Ok, let's make it public.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.h:246: static void
SetDefaultWallpaperPath(const GURL& wallpaper_url);
On 2014/03/27 01:45:50, Daniel Erat wrote:
> nit: SetDefaultWallpaperURL -- "Path" makes it sounds like it's a FilePath.

Done.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
File chrome/browser/chromeos/customization_wallpaper_downloader.h (right):

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_wallpaper_downloader.h:25: class
CustomizationWallpaperDownloader : public net::URLFetcherDelegate {
On 2014/03/27 04:36:32, Dmitry Polukhin wrote:
> It is very sad that we have no code re-use with existing wallpaper manager
that
> can also download and set user's wallpaper. But I'm not familiar with
wallpaper
> code so it is unavoidable :(

Actually we reuse all possible code from WallpaperManager as it cannot download
wallpapers ;-) User wallpapers are downloaded by extension which is very
specific to the server API and requires active user session.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_wallpaper_downloader.h:53: void
OnWallpaperDirectoryCreated(scoped_ptr<bool> success);
On 2014/03/27 04:36:32, Dmitry Polukhin wrote:
> Why do you need scoped_ptr<bool> again it seems that you can pass what even
you
> need as parameter without with trick.

It's the same: to never pass WeakPointer to other thread.

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:320:
ServicesCustomizationDocument::GetInstance()->StartFetching();
On 2014/03/27 04:46:11, Dmitry Polukhin wrote:
> It will always fetch new customization manifest even if wallpaper was already
> applied. It is wrong. Manifest needs to be fetched only if it was not applied.

Done.

Daniel Erat

splitting the CL that way sounds good. https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos/customization_document.cc File chrome/browser/chromeos/customization_document.cc (right): https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos/customization_document.cc#newcode577 chrome/browser/chromeos/customization_document.cc:577: const GURL& ...

6 years, 8 months ago (2014-03-31 14:22:14 UTC) #49

bshe

On 2014/03/31 14:22:14, Daniel Erat wrote: > splitting the CL that way sounds good. > ...

6 years, 8 months ago (2014-03-31 14:26:09 UTC) #50

Alexander Alekseev

6 years, 8 months ago (2014-03-31 15:39:14 UTC) #51

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
File chrome/browser/chromeos/customization_document.cc (right):

https://codereview.chromium.org/208273005/diff/670001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.cc:577: const GURL&
wallpaper_url) {
On 2014/03/31 14:22:15, Daniel Erat wrote:
> On 2014/03/31 14:15:40, alemate wrote:
> > On 2014/03/27 01:45:50, Daniel Erat wrote:
> > > why does this need to be a static method?
> > 
> > It just doesn't require access to object. It doesn't need to be static.
> > Do you think I should still make it "normal" method?
> 
> if it doesn't need to be accessed outside of the class, just make it into a
> function in the anonymous namespace. this makes the header file smaller and is
> easier to maintain (since you don't need to maintain a separate method
signature
> in the header).

It's called from both this class and CustomizationWallpaperDownloader.

Issue 208273005: If customization includes default wallpaper, download and apply it. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Add ShellDelegate::CustomizedWallpaper #

Patch Set 3 : Do not analyze command-line in SetDefaultWallpaper(). #

Patch Set 4 : Rebase. #

Patch Set 5 : Start customization manifest fetch on EULA accepted. Never re-fetch wallpaper. #

Patch Set 6 : Remove modifications of ShellDelegate. #

Patch Set 7 : Unused code is removed. #

Patch Set 8 : Removed DefaultWallpaperFileIsAlreadyLoadingOrLoaded. #

Patch Set 9 : Moved all wallpaper paths from DBC to WallpaperManager. #

Patch Set 10 : Fix comment. #

Patch Set 11 : Add DBC::IsUsingDefaultWallpaper(); Restart wallpaper fetch on device restart. #

Patch Set 12 : Use sandboxed ImageDecoder to read downloaded image file. #

Patch Set 13 : Deleted DBC::SetDefaultWallpaper. #

Patch Set 14 : Update after-review. #

Messages