Remove user-related data from local_state and add to user_preferences, i...

Remove user-related data  from local_state and add to user_preferences, in preparation for multi-profile.

devtools
  kDevToolsSplitLocation

browser
  kBrowserWindowPlacement

Also add a method to browser_prefs to delete obsolete preferences from local state, and fix all related tests.

BUG=66717
TEST=all browser, interactive, and ui tests work properly

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72153

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=73481

[Miranda Callahan, 2011-01-13 21:56:48]

Apavlov, if you could review the devtools change, and Ben, if you could look at
everything else -- ChromeViewsDelegate and the browser window placement stuff. 
Thank you!

[Ben Goodger (Google), 2011-01-13 22:02:10]

How is per-profile data that is local-only (not to be synced) handled?

[mirandac, 2011-01-13 22:13:38]

That data will remain in Local State, etc.; there will be separate
folders for per-profile data, but one shared folder for all the
non-synced data.  This preliminary step just involves moving some of
the data that should be stored by profile into the user's Preferences
file.

On Thu, Jan 13, 2011 at 17:02,  <ben@chromium.org> wrote:
> How is per-profile data that is local-only (not to be synced) handled?
>
> http://codereview.chromium.org/5915006/
>

[Ben Goodger (Google), 2011-01-13 22:21:23]

http://codereview.chromium.org/5915006/diff/174001/chrome/browser/net/predict...
File chrome/browser/net/predictor_api.cc (left):

http://codereview.chromium.org/5915006/diff/174001/chrome/browser/net/predict...
chrome/browser/net/predictor_api.cc:396: // Remove obsolete preferences from
local state if necessary.
???

[Miranda Callahan, 2011-01-13 22:29:37]

http://codereview.chromium.org/5915006/diff/174001/chrome/browser/net/predict...
File chrome/browser/net/predictor_api.cc (left):

http://codereview.chromium.org/5915006/diff/174001/chrome/browser/net/predict...
chrome/browser/net/predictor_api.cc:396: // Remove obsolete preferences from
local state if necessary.
On 2011/01/13 22:21:23, Ben Goodger wrote:
> ???

I put this code in predictor_api when I added the patch to shift the Dns state
from local state to preferences; when I added the second patch to shift more
preferences out of local state it seemed bad to have two functions removing
obsolete prefs in different places -- so I moved this function to browser_prefs
and combined it with this latest patch in a single function to remove obsolete
preferences.

...which I then neglected to call.  Fixing.  Adding another test.

[Ben Goodger (Google), 2011-01-13 22:32:56]

http://codereview.chromium.org/5915006/diff/174001/chrome/browser/net/predict...
File chrome/browser/net/predictor_api.cc (left):

http://codereview.chromium.org/5915006/diff/174001/chrome/browser/net/predict...
chrome/browser/net/predictor_api.cc:396: // Remove obsolete preferences from
local state if necessary.
OK... I am not familiar with this code enough to say where it should live.

[Miranda Callahan, 2011-01-13 22:33:17]

On 2011/01/13 22:29:37, Miranda Callahan wrote:
>
http://codereview.chromium.org/5915006/diff/174001/chrome/browser/net/predict...
> File chrome/browser/net/predictor_api.cc (left):
> 
>
http://codereview.chromium.org/5915006/diff/174001/chrome/browser/net/predict...
> chrome/browser/net/predictor_api.cc:396: // Remove obsolete preferences from
> local state if necessary.
> On 2011/01/13 22:21:23, Ben Goodger wrote:
> > ???
> 
> I put this code in predictor_api when I added the patch to shift the Dns state
> from local state to preferences; when I added the second patch to shift more
> preferences out of local state it seemed bad to have two functions removing
> obsolete prefs in different places -- so I moved this function to
browser_prefs
> and combined it with this latest patch in a single function to remove obsolete
> preferences.
> 
> ...which I then neglected to call.  Fixing.  Adding another test.

I also see here that I treated the window placement values the same way I
treated the dns values -- deleting and rewriting instead of properly migrating. 
This is fine for dns (was suggested by jar, in fact), but not so good for window
data.  Changing to migration from deletion.

[Miranda Callahan, 2011-01-14 17:44:56]

On 2011/01/13 22:02:10, Ben Goodger wrote:
> How is per-profile data that is local-only (not to be synced) handled?

Please disregard earlier comment in which I answered the wrong question!

The sync mechanism uses a setting in the Preferences file (a dictionary of
preference names mapped to boolean values) in order to determine exactly which
data is to be synced .  If data appears in the Preferences file but isn't
mentioned in the sync setting, or has the sync setting set to "false", the sync
mechanism will ignore it.

So all the per-profile data will live together -- the sync dictionary in each
Preferences file will allow discrimination between local and synced data.

[Ben Goodger (Google), 2011-01-14 17:48:04]

Should I continue to look at this or are you making further changes?

-Ben

On Fri, Jan 14, 2011 at 9:44 AM, <mirandac@chromium.org> wrote:

> On 2011/01/13 22:02:10, Ben Goodger wrote:
>
>> How is per-profile data that is local-only (not to be synced) handled?
>>
>
> Please disregard earlier comment in which I answered the wrong question!
>
> The sync mechanism uses a setting in the Preferences file (a dictionary of
> preference names mapped to boolean values) in order to determine exactly
> which
> data is to be synced .  If data appears in the Preferences file but isn't
> mentioned in the sync setting, or has the sync setting set to "false", the
> sync
> mechanism will ignore it.
>
> So all the per-profile data will live together -- the sync dictionary in
> each
> Preferences file will allow discrimination between local and synced data.
>
>
> http://codereview.chromium.org/5915006/
>

[Miranda Callahan, 2011-01-14 17:50:43]

I have a CL with new unit tests in the works right now -- will let you know when
it's updated (I just talked to a sync guy here to make sure my understanding of
synced preferences was correct, which is why I added that comment now).

On 2011/01/14 17:48:04, Ben Goodger wrote:
> Should I continue to look at this or are you making further changes?
> 
> -Ben
> 
> On Fri, Jan 14, 2011 at 9:44 AM, <mailto:mirandac@chromium.org> wrote:
> 
> > On 2011/01/13 22:02:10, Ben Goodger wrote:
> >
> >> How is per-profile data that is local-only (not to be synced) handled?
> >>
> >
> > Please disregard earlier comment in which I answered the wrong question!
> >
> > The sync mechanism uses a setting in the Preferences file (a dictionary of
> > preference names mapped to boolean values) in order to determine exactly
> > which
> > data is to be synced .  If data appears in the Preferences file but isn't
> > mentioned in the sync setting, or has the sync setting set to "false", the
> > sync
> > mechanism will ignore it.
> >
> > So all the per-profile data will live together -- the sync dictionary in
> > each
> > Preferences file will allow discrimination between local and synced data.
> >
> >
> > http://codereview.chromium.org/5915006/
> >

[apavlov, 2011-01-18 00:33:03]

http://codereview.chromium.org/5915006/diff/174001/chrome/browser/debugger/de...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/5915006/diff/174001/chrome/browser/debugger/de...
chrome/browser/debugger/devtools_window.cc:223:
prefs->RegisterDictionaryPref(wp_key.c_str());
I have no relation to this code, but the FindPreference() thing seems to be
missing altogether. I'd suggest that yurys@chromium.org or pfeldman@chromium.org
have a look at this change.

[Miranda Callahan, 2011-01-20 16:14:29]

Finally ready for another review; some ChromeOS compatibility issues came up
that took a bit of time to resolve -- though the problem and solution ended up
being fairly simple (and creating new profiles with the same local state data in
ChromeOS tests uncovered issues that would have shown up later in a multiprofile
system -- which is good).

Adding davemoore to the review, as we discussed some of this patch yesterday;
also pfeldman for the devtools change at apavlov's suggestion:

@pfeldman: devtools code changes
@beng: chrome_views_delegate and window_sizer changes
@davemoore: everything else -- which is the general prefs migration code we
discussed yesterday, especially in browser_prefs and browser.cc

NOTE: I changed the right side of the window in the profiles test data to 450
from 400 because Mac will not accept a browser width of less than 400.  This
allows us to add Mac testing to the pref_service tests.

[Miranda Callahan, 2011-01-20 16:18:17]

Another note: the windows try bot won't accept my changes to Local State because
it's getting confused by the space in the name; I have run tests on my local
windows machine and things seem to work fine.  I've talked to Maruel, who thinks
that committing Local State alone should work fine -- so committing in two
patches will be my strategy.

[Miranda Callahan, 2011-01-21 15:15:08]

Ok, ready for review with almost every profile accessed from the browser or the
profile stored with the accessing instance.

The one exception is ChromeViewsDelegate: because it's a static ViewsDelegate,
we can't store  profile information -- nor can we rewrite its methods to take a
browser or profile variable.  I think the answer will be to attach a profile tag
to the window names, and then have the profile_manager encode and decode the
correct profile to save to based on the tag name.  This solution isn't nice, but
it allows us to pass information about profiles through the ViewsDelegate
without the delegate itself needing knowledge of anything in the browser/
directory. For now, I'm going to leave it as is, accessing the DefaultProfile,
so as not to burden this already long CL; will add this to a second patch.

Ben, what do you think?

[Ben Goodger (Google), 2011-01-21 15:34:08]

Much better! LGTM.

[pfeldman, 2011-01-21 16:28:18]

DevTools part LGTM.

[DaveMoore, 2011-01-21 17:46:27]

pref migration code lgtm

[Miranda Callahan, 2011-01-25 21:27:19]

Ben, I need one more review of chrome_views_delegate.cc -- I had to revert this
because it failed to work with the TaskManager on Windows. Instead of making the
TaskManager window data per profile, which doesn't make sense, I added a
function to ChromeViewsDelegate to choose the pref_service to use by the window
name -- I had mentioned that I would probably need to do this for multiprofile
in any case in an earlier comment. 

(Sorry for the delay fixing this; I had to get in a CL for the sync promo this
weekend...)

[Miranda Callahan, 2011-01-25 21:28:06]

(It passed all trybots with patch set 18; set 19 just fixes a comment.)

[Ben Goodger (Google), 2011-01-26 23:40:27]

This change LGTM... as a side note, if the task manager shows items from
multiple profiles we are probably going to want a column to show what
profile the item comes from.

That Task Manager redesign can't come soon enough...

-Ben

On Tue, Jan 25, 2011 at 1:28 PM, <mirandac@chromium.org> wrote:

> (It passed all trybots with patch set 18; set 19 just fixes a comment.)
>
>
>
>
> http://codereview.chromium.org/5915006/
>