Use username_hash instead of email

Use username_hash instead of email as directory name

Previously, email was used as directory name to save user's
custom wallpaper. It is not safe as email may contain special
characters.

After this CL, username_hash will be used as directory name.
And previously saved custom wallpaper will be migrated to new
directory among user login.


BUG=287020
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226140

[bshe, 2013-09-17 23:18:05]

Hi Nikita.

Would you mind to take a look at wallpaper_manager related change? I will add
test for move custom wallpaper in a follow up CL.

+cc miket Not sure when will you be OOO. But please feel free to comment and
review this CL. I put you in CC list just in case that you already OOO.

Thanks!

[bshe, 2013-09-18 00:03:30]

On 2013/09/17 23:18:05, bshe wrote:
> Hi Nikita.
> 
> Would you mind to take a look at wallpaper_manager related change? I will add
> test for move custom wallpaper in a follow up CL.
> 
> +cc miket Not sure when will you be OOO. But please feel free to comment and
> review this CL. I put you in CC list just in case that you already OOO.
> 
> Thanks!

Sorry Nikita. Do you mind to hold off this review for now? I discovered this
code wont work after browser crash. I comment in the issue and cced you.
I will send this for review again once I figure out how to handle browser crash.

[bshe, 2013-09-20 21:19:52]

On 2013/09/18 00:03:30, bshe wrote:
> On 2013/09/17 23:18:05, bshe wrote:
> > Hi Nikita.
> > 
> > Would you mind to take a look at wallpaper_manager related change? I will
add
> > test for move custom wallpaper in a follow up CL.
> > 
> > +cc miket Not sure when will you be OOO. But please feel free to comment and
> > review this CL. I put you in CC list just in case that you already OOO.
> > 
> > Thanks!
> 
> Sorry Nikita. Do you mind to hold off this review for now? I discovered this
> code wont work after browser crash. I comment in the issue and cced you.
> I will send this for review again once I figure out how to handle browser
crash.

Hi Nikita. It looks like after your ChromeOS side change:
https://chromium-review.googlesource.com/#/c/169850/ and Chrome side change:
https://codereview.chromium.org/23967015/
username_hash should set to user hash instead of "user" after browser crash. If
this
is correct, this CL is probably fine. Would you mind to take a look?

[bshe, 2013-09-23 22:22:29]

On 2013/09/20 21:19:52, bshe wrote:
> On 2013/09/18 00:03:30, bshe wrote:
> > On 2013/09/17 23:18:05, bshe wrote:
> > > Hi Nikita.
> > > 
> > > Would you mind to take a look at wallpaper_manager related change? I will
> add
> > > test for move custom wallpaper in a follow up CL.
> > > 
> > > +cc miket Not sure when will you be OOO. But please feel free to comment
and
> > > review this CL. I put you in CC list just in case that you already OOO.
> > > 
> > > Thanks!
> > 
> > Sorry Nikita. Do you mind to hold off this review for now? I discovered this
> > code wont work after browser crash. I comment in the issue and cced you.
> > I will send this for review again once I figure out how to handle browser
> crash.
> 
> Hi Nikita. It looks like after your ChromeOS side change:
> https://chromium-review.googlesource.com/#/c/169850/ and Chrome side change:
> https://codereview.chromium.org/23967015/
> username_hash should set to user hash instead of "user" after browser crash.
If
> this
> is correct, this CL is probably fine. Would you mind to take a look?

friendly ping? I just test it on my device with the two above mentioned CL. It
seems
the username_hash is now the real hash instead of "user" after crash. So this CL
should
be OK.

[Nikita (slow), 2013-09-25 14:47:10]

My main question is whether WallpaperManager will handle legacy configuration
when user_id_hash is empty.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_api.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_api.cc:43: username_hash_ =
This may be empty string if multi-profiles are not enabled.
Have you tested this w/o --multi-profiles switch?

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_api.cc:69:
wallpaper_manager->SetCustomWallpaper(email_, username_hash_, file_name_,
nit: One parameter per line please.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_api.h (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_api.h:57: std::string
username_hash_;
nit: We try to migrate from email to user_id.
Please rename username > user_id (user_id_hash) in this CL.

I'll be removing UserManager API as well. When it was added we were not decided
on the exact name.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:504:
wallpaper_manager->SetCustomWallpaper(email_, username_hash_, file_name_,
nit: One parameter per line.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:166:
std::string(kTestUser1Hash) + "/" + id,
nit: Please use platform specific path separator constant.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:247:
std::string(kTestUser1Hash) + "/" + id,
nit: Same here.

[Nikita (slow), 2013-09-25 16:23:12]

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:464: username_hash + '/' +
file,
nit: Use separator constant.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:855: const std::string&
username_hash) {
nit: Either align this parameter with previous or move first one on the second
line.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:858: if
(MoveCustomWallpaperDirectory(kOriginalWallpaperSubDir, email,
nit: Move second parameter on separate line.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:867: email, username_hash));
nit: Move to next line.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:874: void
WallpaperManager::MoveCustomWallpapersSuccess(const std::string& email,
nit: same here

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:890:
task_runner_->PostTask(FROM_HERE,
Please postpone this task since when NOTIFICATION_LOGIN_USER_CHANGED happens we
should give priority to session restore and other tasks.

[bshe, 2013-09-26 16:49:57]

Thanks for review. Do you mind to take another look?

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_api.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_api.cc:43: username_hash_ =
I tested w/o --multi-profiles switch. It seems work for both cases.
These are the cases that I tested (have two logged in users on my dev machine):
1. no --multi-profiles
  a. login in one user, change custom wallpaper, logout, wallpaper
     works correctly.
  b. login again, navigate to about://inducebrowsercrashforrealz,
     change custom wallpaper again, verified the user_name_id is
     the real hash instead of "user". logout, custom wallpaper
     works correctly.
2. enable --multi-profiles
  a. login one user and then login another, repeat 1.b step. Works
     fine.
  b. login one user and then login another, switch to the new
     logged in user, repeat 1.b step. works fine.
  c. switch the primary login account and do 2.a and 2.b, works fine.

On 2013/09/25 14:47:11, Nikita Kostylev wrote:
> This may be empty string if multi-profiles are not enabled.
> Have you tested this w/o --multi-profiles switch?

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_api.cc:69:
wallpaper_manager->SetCustomWallpaper(email_, username_hash_, file_name_,
On 2013/09/25 14:47:11, Nikita Kostylev wrote:
> nit: One parameter per line please.

Done.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_api.h (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_api.h:57: std::string
username_hash_;
On 2013/09/25 14:47:11, Nikita Kostylev wrote:
> nit: We try to migrate from email to user_id.
> Please rename username > user_id (user_id_hash) in this CL.
> 
> I'll be removing UserManager API as well. When it was added we were not
decided
> on the exact name.

Done.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:504:
wallpaper_manager->SetCustomWallpaper(email_, username_hash_, file_name_,
On 2013/09/25 14:47:11, Nikita Kostylev wrote:
> nit: One parameter per line.

Done.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:464: username_hash + '/' +
file,
Not sure if you mean base::FilePath::kSeparators[0]?
I constructed a new FilePath and using Append to avoid
the separator. But happy to change it back to use
base::FilePath::kSeparators[0] if you think it is better.

On 2013/09/25 16:23:12, Nikita Kostylev wrote:
> nit: Use separator constant.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:855: const std::string&
username_hash) {
On 2013/09/25 16:23:12, Nikita Kostylev wrote:
> nit: Either align this parameter with previous or move first one on the second
> line.

Done.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:858: if
(MoveCustomWallpaperDirectory(kOriginalWallpaperSubDir, email,
On 2013/09/25 16:23:12, Nikita Kostylev wrote:
> nit: Move second parameter on separate line.

Done.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:867: email, username_hash));
On 2013/09/25 16:23:12, Nikita Kostylev wrote:
> nit: Move to next line.

Done.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:874: void
WallpaperManager::MoveCustomWallpapersSuccess(const std::string& email,
On 2013/09/25 16:23:12, Nikita Kostylev wrote:
> nit: same here

Done.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:890:
task_runner_->PostTask(FROM_HERE,
Postponed it after receiving NOTIFICATION_SESSION_STARTED. It may run again
after browser crash. This should be fine since after first migration, the
following calls should be noop. Or should I just add a delay?

On 2013/09/25 16:23:12, Nikita Kostylev wrote:
> Please postpone this task since when NOTIFICATION_LOGIN_USER_CHANGED happens
we
> should give priority to session restore and other tasks.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:166:
std::string(kTestUser1Hash) + "/" + id,
Will switch to base::FilePath::kSeperators[0] if make sense.

On 2013/09/25 14:47:11, Nikita Kostylev wrote:
> nit: Please use platform specific path separator constant.

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:247:
std::string(kTestUser1Hash) + "/" + id,
same.

On 2013/09/25 14:47:11, Nikita Kostylev wrote:
> nit: Same here.

[Nikita (slow), 2013-09-26 17:46:18]

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:890:
task_runner_->PostTask(FROM_HERE,
On 2013/09/26 16:49:57, bshe wrote:
> Postponed it after receiving NOTIFICATION_SESSION_STARTED. It may run again
> after browser crash. This should be fine since after first migration, the
> following calls should be noop. Or should I just add a delay?
> 
> On 2013/09/25 16:23:12, Nikita Kostylev wrote:
> > Please postpone this task since when NOTIFICATION_LOGIN_USER_CHANGED happens
> we
> > should give priority to session restore and other tasks.
> 

I'm proposing to add reasonable delay - in seconds, like 30 seconds.

After NOTIFICATION_SESSION_STARTED it is still pretty busy.

[bshe, 2013-09-26 21:02:11]

Hi Nikita. Do you mind to take another look at this latest patch? Sorry it
contains some diff from rebase. I forgot to upload a patchset before rebase. The
only file that I changed is wallpaper_manager.cc in the newest patch.

Thanks!

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/23480087/diff/27001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:890:
task_runner_->PostTask(FROM_HERE,
I see. Added a 30 seconds delay for the task.


On 2013/09/26 17:46:18, Nikita Kostylev wrote:
> On 2013/09/26 16:49:57, bshe wrote:
> > Postponed it after receiving NOTIFICATION_SESSION_STARTED. It may run again
> > after browser crash. This should be fine since after first migration, the
> > following calls should be noop. Or should I just add a delay?
> > 
> > On 2013/09/25 16:23:12, Nikita Kostylev wrote:
> > > Please postpone this task since when NOTIFICATION_LOGIN_USER_CHANGED
happens
> > we
> > > should give priority to session restore and other tasks.
> > 
> 
> I'm proposing to add reasonable delay - in seconds, like 30 seconds.
> 
> After NOTIFICATION_SESSION_STARTED it is still pretty busy.

[Nikita (slow), 2013-09-30 15:52:21]

lgtm

[commit-bot: I haz the power, 2013-09-30 17:35:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/23480087/68001

[commit-bot: I haz the power, 2013-10-01 00:29:54]

Retried try job too often on win_rel for step(s) content_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-01 03:27:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/23480087/68001

[commit-bot: I haz the power, 2013-10-01 04:58:10]

Change committed as 226140