Create profile .ico file on profile creation

chrome/browser/profiles/profile_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/profiles/profile_impl.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
@@ skipping 731 matching lines @@
   } else {
     last_session_exit_type_ =
         SessionTypePrefValueToExitType(exit_type_pref_value);
   }
   // Mark the session as open.
   prefs_->SetString(prefs::kSessionExitType, kPrefExitTypeCrashed);
   // Force this to true in case we fallback and use it.
   // TODO(sky): remove this in a couple of releases (m28ish).
   prefs_->SetBoolean(prefs::kSessionExitedCleanly, true);
 
+#if defined(OS_WIN)
+  // Ensure the profile's icon file has been created.
+  if (!prefs_->GetBoolean(prefs::kProfileIconCreated)) {
+    ProfileShortcutManager* profile_shortcut_manager =
+        g_browser_process->profile_manager()->profile_shortcut_manager();
+    profile_shortcut_manager->CreateProfileIcon(GetPath());

[Alexei Svitkine (slow), 2013/05/03 17:20:47]

Please check that profile_shorcut_manager is not NULL. It can be NULL if
ProfileShortcutManager::IsFeatureEnabled() is false.

[gab, 2013/05/07 12:38:39]

So this means that profile taskbar icons (pinned or not) will now depend on
ProfileShortcuts being enabled? That sounds wrong to me (i.e., taskbar icons
have always been badged, way before profile shortcuts came around). I'm only
comfortable with this if the code to disable profile shortcuts is removed (I'm
not sure what the team's policy is to remove feature flags, but it feels like
once profile shortcuts ship to stable we could remove this? i.e., was the flag
only to be able to turn it on/off for individual milestones during
development?).

[calamity, 2013/05/08 08:15:42]

Hmm. I hadn't realized that this could be a problem. I think making
CreateProfileIcon a static method is a bit messy since it shares the majority of
its implementation with CreateProfileShortcuts.

Can we just change IsFeatureEnabled to ShouldCreateShortcuts? This method is
mainly used to decide whether or not to create profile_shortcut_manager_ in
profile_manager, but we could just always create it and then
DCHECK(ShouldCreateShortcuts()) at the top of shortcut creation method calls.

I haven't noticed any problems locally except that the profile icon isn't set
the first time the new profile is created. A subsequent patch to browser should
fix that.

[gab, 2013/05/08 13:01:08]

Even if we remove app_list from the condition, I'm still uncomfortable until:

1) BrowserDistribution::CanCreateDesktopShortcuts() is changed for a generic
BrowserDistribution::CanCreateShortcuts() -- I think it's basically the same
rules (i.e. distributions that can create desktop shortcuts are the ones that
can create shortcuts in general), but it's worth looking through the codebase to
make sure.

2) Profile shortcuts are made to work with custom user-data-dir (why doesn't
this work? seems like it shouldn't be too hard... is it just a matter of reading
the switch and writing it back to the shortcuts created for a custom
user-data-dir (and perhaps giving those shortcuts a custom name based on the
user-data-dir to avoid naming conflicts)?)


Another option perhaps is to build a resource file of all the possible badged
Chrome icons and index in that file for the correct icon at runtime (i.e.,
multiple user-data-dirs could use the same resource file with no problem without
having to make profile shortcuts compatible with user-data-dirs :))

[Alexei Svitkine (slow), 2013/05/08 15:12:04]

I wrote a CL to implement this in: https://codereview.chromium.org/11852022/

(I believe it covered most of the cases, but there was still some
--user-data-dir edge cases that weren't covered with that CL.)

It added quite a bit of extra complexity to the profile shortcuts code and
Sailesh didn't think it made sense to add such complexity for a use case normal
users aren't expected to run. Disabling the feature entirely was a good
compromise we agreed on.

I still agree with that conclusion, we shouldn't complicate the code so much to
support --user-data-dir.
That's a good idea, though I'm not sure whether we want to lose the flexibility
of custom-badging functionality long-term - for example if/once profiles start
using the GAIA picture for their avatar. (Though it's also questionable whether
that will look good for badging.)

[Alexei Svitkine (slow), 2013/05/08 15:12:04]

We could make CreateOrUpdateShortcutsForProfileAtPath() static too, since it's
private already.
I guess we could do that, though this would require changing the current users
of the API that currently check if the profile shortcut manager isn't NULL
before calling stuff on it, whereas now they'll need to check
ShouldCreateShortcuts().

[calamity, 2013/05/09 06:12:16]

There are only a few callers. I'll just set it to check for NULL for now and fix
it in a follow-up CL?

[gab, 2013/05/09 13:43:56]

I see, so as it is now we only use this icon for shortcut properties, but at
runtime Chrome still sets its icon and badges at runtime like before, right? 
We will have to change this code to use the icon created in this CL instead
though, right?
In which case this will regress custom user-data-dir in that they will no longer
get badged icons in their taskbar (I agree however that custom user-data-dir is
probably not nearly as useful as it used to be, now that we've had
multi-profiles for over a year); the appid will still be set though, thus the
icons will still be split in the taskbar at runtime for separate profiles, they
simply won't be badged (unless we keep the runtime badging code around for this
use-case only, but I also don't like that...).
Hmmm.. badging with the GAIA picture seems weird to me.

At the same time I also agree that making something static like a big resource
blob of badged icons might come back to bite us and be hard to maintain when we
want to change/add badges.


I reread the code again and tried to come up with a better approach, but I think
the current approach is the only possible approach since this is so tied with
the current code (i.e., all the logic to change icons when going from 1 to 2
profiles, back to a single profile, etc.).
I still don't like regressing a feature of Chrome though (custom
user-data-dirs), should we officially deprecate custom user-data-dirs given that
they have essentially been replaced by multi-profiles? (I wouldn't go as far as
removing the code for now as some people still use it for testing, but
deprecating it in the sense that features will no longer go out of their way to
support it?)
Adding sail@ for his opinion on this.

[gab, 2013/05/09 13:43:56]

I'm not sure what you mean here, if you always create the ProfileManager, then
you need more than a DCHECK, you need actual logic (i.e. ifs) to make this a
no-op in the right distributions... anyways, if we keep disabling for custom
user-data-dirs, might as well keep the disabling logic where it's at now imo.

+    prefs_->SetBoolean(prefs::kProfileIconCreated, true);

[Alexei Svitkine (slow), 2013/05/03 17:20:47]

I think this should be set in a callback as a result of CreateProfileIcon
succeeding.

Which probably means that CreateProfileIcon should take a callback, similar to
what HasProfileShortcuts() does.

[calamity, 2013/05/08 08:15:42]

We won't need this pref if we are checking the profile icon every startup.

[gab, 2013/05/08 13:01:08]

Haven't looked deeply in the pref part of this, but it seems checking a pref is
preferable then doing I/O every startup.

[calamity, 2013/05/09 06:12:16]

The pref is a "on first run past this change" pref which won't check the icon
file exists on every startup. The startup IO will be done async and will use the
hashing code to only write if necessary.

+  }
+#endif
+
   ProfileDependencyManager::GetInstance()->CreateProfileServices(this, false);
 
   DCHECK(!net_pref_observer_.get());
   net_pref_observer_.reset(new NetPrefObserver(
       prefs_.get(),
       prerender::PrerenderManagerFactory::GetForProfile(this),
       predictor_));
 
   ChromeVersionService::OnProfileLoaded(prefs_.get(), IsNewProfile());
   DoFinalInit();
@@ skipping 402 matching lines @@
                                      base::FilePath* cache_path,
                                      int* max_size) {
   DCHECK(cache_path);
   DCHECK(max_size);
   base::FilePath path(prefs_->GetFilePath(prefs::kDiskCacheDir));
   if (!path.empty())
     *cache_path = path;
   *max_size = is_media_context ? prefs_->GetInteger(prefs::kMediaCacheSize) :
                                  prefs_->GetInteger(prefs::kDiskCacheSize);
 }