No longer register app window placement preference keys on the fly.

No longer register app window placement preference keys on
the fly.

Instead register a new Dictionary,
browser.app_window_placement, at startup and put apps'
placement/size info in sub dictionaries.

Also migrate values from the old dynamically-registered
preferences to their new statically-registered home.

BUG=167256
Committed: https://crrev.com/e30f8f0957be8ffa94a70dc9e8e6f61c9b2990c5
Cr-Commit-Position: refs/heads/master@{#295163}

[dgrogan, 2014-09-04 00:19:22]

dgrogan@chromium.org changed reviewers:
+ bauerb@chromium.org

[dgrogan, 2014-09-04 00:19:23]

Hi Bernhard, could you review this? Details of a few points of ugliness
forthcoming.

[dgrogan, 2014-09-04 00:50:51]

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service.cc
File base/prefs/pref_service.cc (right):

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service...
base/prefs/pref_service.cc:559: const base::Value* browser_value = NULL;
The other accessor methods use pref_registry to get values. But the values that
I'm trying to migrate don't have a registered key.

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service.h
File base/prefs/pref_service.h (right):

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service...
base/prefs/pref_service.h:276: scoped_ptr<base::DictionaryValue>
GetBrowserDictionary() const;
This is the ugliest. See browser_ui_prefs.cc for its usage.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:21: prefs->GetBrowserDictionary();
An alternative to the GetBrowserDictionary method is to plumb the
user_pref_store in here somehow. That didn't seem like a clear win.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.cc:83: if
(prefs->FindPreference(window_name.c_str())) {
This seems fragile... any window_name that isn't a registered preference gets
put into the app dictionary. But I don't see a better way to retrieve the
dictionary based solely on window_name, which is what ChromeViewsDelegate
insists on.

[Bernhard Bauer, 2014-09-04 11:06:01]

bauerb@chromium.org changed reviewers:
+ gab@chromium.org

[Bernhard Bauer, 2014-09-04 11:06:03]

+gab

There might be another way to do the migration, which wouldn't involve opening
up the innards of PrefService... there is a PrefFilter, which is called when the
user preferences are loaded from disk, and which can modify them before the rest
of the preference system sees them. Gab, what do you think?

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service.cc
File base/prefs/pref_service.cc (right):

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service...
base/prefs/pref_service.cc:564: NOTREACHED();
Using NOTREACHED() to handle the result of a check implies that the check could
fail, which contradicts the NOTREACHED(). Use a DCHECK instead to check the
result. See also
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-.

(I know there are other places in this file that use NOTREACHED(), but they are
old code that should be fixed).

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service.h
File base/prefs/pref_service.h (right):

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service...
base/prefs/pref_service.h:273: // This is only to be used to migrate
unregistered prefs to become registered.
One thing we could do to ensure no one else uses this method would be to make it
private and friend the caller. WDYT?

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service...
base/prefs/pref_service.h:274: // TODO(dgrogan): Remove after a suitable
migration period. See
Can you add what a suitable migration period is? (E.g. which milestone) That
way, even if we both forget about this, someone else who comes across this code
in ten years can fix it.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1130: const char*
wp_key(prefs::kAppWindowPlacement);
Nit: it feels weird to me to initialize a pointer variable with parentheses.
Could you use `=`?

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1132: if
(!prefs->GetDictionary(wp_key)->GetDictionary(kDevToolsApp, NULL)) {
You can use DictionaryValue::HasKey.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_browsertest.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_browsertest.cc:239:
base::FilePath().AppendASCII("profiles").
An alternative to FilePath().AppendASCII("...") would be to use
FilePath(FILE_PATH_LITERAL("...")). Sometimes it's abbreviated via an #ifdef to
FPL.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_browsertest.cc:257:
EXPECT_NE(browser_placement, static_cast<base::DictionaryValue*>(NULL));
Put the expected value first for a nicer error message when the check fails.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:22: if (!browser_dict.get())
scoped_ptr has a bool conversion operator.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:31: if (StartsWithASCII(full_key,
search_for, true /*case_sensitive*/)) {
Nit: space inside of a /* comment */.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:36: bool found =
browser_dict->GetDictionary(it.key(), &old_value);
You can use it.value() to get the value, then call GetAsDictionary(). That will
make it a bit clearer that we expect the value to be a dictionary (but we know
that it exists).

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.cc:94: const std::string entry =
browser->is_type_popup() ?
Declare this as const char*?

[dgrogan, 2014-09-04 20:26:33]

As I mention in the inline comments, I am going to look into (ab)using
PrefFilter to do the migration.

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service.cc
File base/prefs/pref_service.cc (right):

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service...
base/prefs/pref_service.cc:564: NOTREACHED();
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> Using NOTREACHED() to handle the result of a check implies that the check
could
> fail, which contradicts the NOTREACHED(). Use a DCHECK instead to check the
> result. See also
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-.
> 
> (I know there are other places in this file that use NOTREACHED(), but they
are
> old code that should be fixed).

Removed the NOTREACHED since it is possible that someone modified their
"browser" entry to be a non-dictionary.

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service.h
File base/prefs/pref_service.h (right):

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service...
base/prefs/pref_service.h:273: // This is only to be used to migrate
unregistered prefs to become registered.
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> One thing we could do to ensure no one else uses this method would be to make
it
> private and friend the caller. WDYT?

Changed (for this patchset, at least). I don't like two things about it. (1)
When we remove the method it will be easy to forget about the "friend" line. (2)
The declarations are ugly and indicate a layering violation, though none exists.
I'm going to look into using PrefFilter to obviate this issue.

https://codereview.chromium.org/511393003/diff/200001/base/prefs/pref_service...
base/prefs/pref_service.h:274: // TODO(dgrogan): Remove after a suitable
migration period. See
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> Can you add what a suitable migration period is? (E.g. which milestone) That
> way, even if we both forget about this, someone else who comes across this
code
> in ten years can fix it.

Done.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1130: const char*
wp_key(prefs::kAppWindowPlacement);
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> Nit: it feels weird to me to initialize a pointer variable with parentheses.
> Could you use `=`?

Done.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1132: if
(!prefs->GetDictionary(wp_key)->GetDictionary(kDevToolsApp, NULL)) {
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> You can use DictionaryValue::HasKey.

Done.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_browsertest.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_browsertest.cc:239:
base::FilePath().AppendASCII("profiles").
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> An alternative to FilePath().AppendASCII("...") would be to use
> FilePath(FILE_PATH_LITERAL("...")). Sometimes it's abbreviated via an #ifdef
to
> FPL.

Done. I suppose the advantage to the way you suggest is that it's not clear what
is meant to call AppendASCII("whatever") on a newly constructed FilePath object
that had no parameters. Though then you have situations like this one where
using FILE_PATH_LITERAL causes an inconsistency between how the first directory
is appended vs how the rest of the directories are appended which seems
confusing, at least to me. And base::FilePath().AppendASCII is alive and well in
chromium:

https://code.google.com/p/chromium/codesearch#search/&q=FilePath%5C(%5C)%5C.A...

I'm going to leave this to use FILE_PATH_LITERAL as you suggest because, despite
evidence to the contrary, I don't really care.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_browsertest.cc:257:
EXPECT_NE(browser_placement, static_cast<base::DictionaryValue*>(NULL));
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> Put the expected value first for a nicer error message when the check fails.

Done.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:22: if (!browser_dict.get())
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> scoped_ptr has a bool conversion operator.

Done.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:31: if (StartsWithASCII(full_key,
search_for, true /*case_sensitive*/)) {
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> Nit: space inside of a /* comment */.

Done.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:36: bool found =
browser_dict->GetDictionary(it.key(), &old_value);
On 2014/09/04 11:06:02, Bernhard Bauer wrote:
> You can use it.value() to get the value, then call GetAsDictionary(). That
will
> make it a bit clearer that we expect the value to be a dictionary (but we know
> that it exists).

I got rid of the conversion to dictionary. The only thing I could do with it was
DCHECK that it was a dictionary, but as I brought up elsewhere it's possible
that someone has modified their Preferences such that the result isn't a
dictionary.

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/511393003/diff/200001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.cc:94: const std::string entry =
browser->is_type_popup() ?
On 2014/09/04 11:06:03, Bernhard Bauer wrote:
> Declare this as const char*?

Done.

[dgrogan, 2014-09-04 21:57:56]

Adding a new BrowserUIPrefsMigrationFilter will be more intrusive than I was
hoping. Looks like I would also have to create a new CompositePrefFilter that
takes multiple filters and calls them in sequence. Then a CompositePrefFilter
would be instantiated and passed to JsonPrefStore instead of PrefHashFilter.
Given how sensitive the hardening is I'm a bit wary of even taking the chance of
messing up the implementation of CompositePrefFilter. So, given that the
GetBrowserDictionary hack is temporary (famous last words) and not that useful
for general consumption (aka not that likely to be used in the meantime) I'd
prefer to stick with the current approach.

Thoughts?

[gab, 2014-09-05 00:53:10]

Took a look at the entire CL.

I agree that this minor encapsulation hurdle for the purpose of migration code
is perhaps not as intrusive as a new PrefFilter.

Although I also like the idea of being able to do pref cleanups
(migrations/deletion of old prefs) in a way that would be transparent to the
rest of Chrome (currently a bunch of old prefs are registered just so they can
be cleared...).

A CompositePrefFilter wouldn't be required if JsonPrefStore is modified to allow
for multiple PrefFilters.

Actually this makes me think; this perhaps wouldn't need to be a PrefFilter per
se (which have the property of doing their work before *anything* else sees the
prefs) in your case you could probably just be an observer of the JsonPrefStore
and interact directly with it (rather than through PrefService) when notified of
OnInitializationCompleted (which is still before anything -- except the
PrefFilter and potential other observers -- gets to see prefs which is good
enough)?

It all depends on how deeply integrated we want this cleanup code to be. The
current CL is as high-level as it can reasonably be but hooking deeper could
provide nicer properties if we can justify the intrusion.

Cheers,
Gab

PS: Sorry for the flow of thoughts, just dropped everything that came to mind
without really trying to formalize which option I think is best, let me know if
you want a more opinionated review.

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1130: const char* wp_key =
prefs::kAppWindowPlacement;
Inlining the constant will be more readable IMO.

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:21: prefs->GetBrowserDictionary();
I'm torn here, you could do this without requiring a DeepCopy of the "browser"
dictionary and could also avoid DeepCopy of the individual dicts below, but it
would involve PrefService exposing the true DictionaryValue* which is perhaps
even more intrusive (but encapsulation is already broken with the current
approach so wtv..?)... 
The idea would be to use DictionaryValue::Remove which allows you to grab the
deleted data and put it somewhere else rather than DeepCopy+ClearPref below.

The DeepCopy is particularly painful as you will do it every launch despite it
being a no-op for every launch but the first one... (you should probably exit
fast if you can tell the migration has already occurred).

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:35:
ReplaceFirstSubstringAfterOffset(&new_key, 0, search_for, "");
|new_key| (which is equal to |full_key| at this point) always begins with
"browser.window_placement_" (as per the condition this code is wrapped in)

So you can replace the above two lines by:

std::string new_key(full_key.substr(search_for.length()));

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.cc:87: return make_scoped_ptr(new
WindowPlacementPrefUpdate(prefs, window_name))
Isn't the whole point of WindowPlacementPrefUpdate that it handles whether or
not the pref is present? If so, why also do this check in this method as opposed
to simply always returning a WindowPlacementPrefUpdate?

[dgrogan, 2014-09-06 00:47:06]

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1130: const char* wp_key =
prefs::kAppWindowPlacement;
On 2014/09/05 00:53:10, gab wrote:
> Inlining the constant will be more readable IMO.

Done.

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:21: prefs->GetBrowserDictionary();
On 2014/09/05 00:53:10, gab wrote:
> I'm torn here, you could do this without requiring a DeepCopy of the "browser"
> dictionary and could also avoid DeepCopy of the individual dicts below, but it
> would involve PrefService exposing the true DictionaryValue* which is perhaps
> even more intrusive (but encapsulation is already broken with the current
> approach so wtv..?)... 
> The idea would be to use DictionaryValue::Remove which allows you to grab the
> deleted data and put it somewhere else rather than DeepCopy+ClearPref below.
> 
> The DeepCopy is particularly painful as you will do it every launch despite it
> being a no-op for every launch but the first one... (you should probably exit
> fast if you can tell the migration has already occurred).

I moved the migration to work off a PrefStoreObserver which does a Remove like
you suggest here. So there are no DeepCopy calls in the new code. Yay. Also
added a fast-exit check.

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
chrome/browser/ui/browser_ui_prefs.cc:35:
ReplaceFirstSubstringAfterOffset(&new_key, 0, search_for, "");
On 2014/09/05 00:53:10, gab wrote:
> |new_key| (which is equal to |full_key| at this point) always begins with
> "browser.window_placement_" (as per the condition this code is wrapped in)
> 
> So you can replace the above two lines by:
> 
> std::string new_key(full_key.substr(search_for.length()));

Done.

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/511393003/diff/220001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.cc:87: return make_scoped_ptr(new
WindowPlacementPrefUpdate(prefs, window_name))
On 2014/09/05 00:53:10, gab wrote:
> Isn't the whole point of WindowPlacementPrefUpdate that it handles whether or
> not the pref is present? If so, why also do this check in this method as
opposed
> to simply always returning a WindowPlacementPrefUpdate?

The check here is for kBrowserWindowPlacement and kBrowserWindowPlacementPopup,
which don't go in the new kAppWindowPlacement dictionary. The check in
WindowPlacementPrefUpdate is to look in the new dictionary to see if the apps'
sub dictionaries already exist.

[dgrogan, 2014-09-06 02:32:58]

This is ready for another look. I moved the migration code to
chrome/browser/prefs/chrome_pref_service_factory.cc and got rid of the
PrefService hack.

[gab, 2014-09-08 21:29:26]

lg, comments below.

@Bernhard how do you like the observer approach? I proposed it in the last
review cycle as one option as it allows to get in early on prefs (before they're
handed to the browser) without needing to be a PrefFilter per se.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:409: class
BrowserUIPrefsMigrationObserver : public PrefStore::Observer {
I think this should live in its own file:
 1) to avoid polluting chrome_pref_service_factory.cc
 2) to allow unit testing (I don't think you'd actually need a full blown
browser test in this case, unittests should suffice, mocked by an in memory
DictionaryPrefStore rather than having to use IO).

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:409: class
BrowserUIPrefsMigrationObserver : public PrefStore::Observer {
Add a meta comment explaining the purpose of this class:
 1) Why it can't do typical migration like everybody else (migration problem).
 2) A note that it deletes itself when done.
 3) A todo to remove this code in say M42.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:416: private:
space above.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:418: };
DISALLOW_COPY_AND_ASSIGN above.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:447:
scoped_ptr<base::DictionaryValue> app_window_placement;
Add a comment about how those pref used to look like for future readers to
understand this migration code without knowing about the old state of the world.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:536: new
BrowserUIPrefsMigrationObserver(user_pref_store));
Add a comment above that the BrowserUIPrefsMigrationObserver deletes itself when
done.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_browsertest.cc (right):

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_browsertest.cc:248:
CHECK(base::CopyFile(original_pref_file, tmp_pref_file));
s/tmp_pref_file/test_pref_file ? (it's not really a temp file, it's the file
under test -- which happens to be in a temp dir)

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_browsertest.cc:261:
EXPECT_NE(static_cast<base::DictionaryValue*>(NULL), browser_placement);
Also verify that things in this dictionary appear as expected from the test
file.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_browsertest.cc:267: }
I think you also want to verify that other kBrowserWindowPlacement + "_*"
entries were deleted.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.cc:83: if
(prefs->FindPreference(window_name.c_str())) {
Add a comment here about the fact that you're first looking for non-app windows
(i.e. browser/popup) as I was confused by this originally even in the context of
this CL.

https://codereview.chromium.org/511393003/diff/260001/chrome/test/data/profil...
File chrome/test/data/profiles/app_window_placement/Default/Preferences (right):

https://codereview.chromium.org/511393003/diff/260001/chrome/test/data/profil...
chrome/test/data/profiles/app_window_placement/Default/Preferences:25: }
Also add another test app which is not DevTools (since that's specific/known
app).

[dgrogan, 2014-09-09 03:15:44]

Patchset #15 is WIP and has a sync to ToT in there. So it's a bit of a mess. I
will tease those apart tomorrow. I still wouldn't mind a look at the new
unittest.cc to ensure that's consistent with what gab@ is envisioning.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:409: class
BrowserUIPrefsMigrationObserver : public PrefStore::Observer {
On 2014/09/08 21:29:25, gab wrote:
> Add a meta comment explaining the purpose of this class:
>  1) Why it can't do typical migration like everybody else (migration problem).
>  2) A note that it deletes itself when done.
>  3) A todo to remove this code in say M42.

Done.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:409: class
BrowserUIPrefsMigrationObserver : public PrefStore::Observer {
On 2014/09/08 21:29:25, gab wrote:
> I think this should live in its own file:
>  1) to avoid polluting chrome_pref_service_factory.cc
>  2) to allow unit testing (I don't think you'd actually need a full blown
> browser test in this case, unittests should suffice, mocked by an in memory
> DictionaryPrefStore rather than having to use IO).

Moved to its own file.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:416: private:
On 2014/09/08 21:29:25, gab wrote:
> space above.

Done.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:418: };
On 2014/09/08 21:29:25, gab wrote:
> DISALLOW_COPY_AND_ASSIGN above.

Done.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:447:
scoped_ptr<base::DictionaryValue> app_window_placement;
On 2014/09/08 21:29:25, gab wrote:
> Add a comment about how those pref used to look like for future readers to
> understand this migration code without knowing about the old state of the
world.

Done.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:536: new
BrowserUIPrefsMigrationObserver(user_pref_store));
On 2014/09/08 21:29:25, gab wrote:
> Add a comment above that the BrowserUIPrefsMigrationObserver deletes itself
when
> done.

Done.

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/511393003/diff/260001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.cc:83: if
(prefs->FindPreference(window_name.c_str())) {
On 2014/09/08 21:29:26, gab wrote:
> Add a comment here about the fact that you're first looking for non-app
windows
> (i.e. browser/popup) as I was confused by this originally even in the context
of
> this CL.

Done.

[Bernhard Bauer, 2014-09-09 09:12:53]

This looks very nice, thank you!

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator.cc (right):

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.cc:30: if
(!pref_store_->GetMutableValue("browser", &browser_value)) {
No braces for one-line bodies.

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator.h (right):

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:12: // previously-dynamically-registered
preferences to their new home in a
Comment nit: I think this is fine without the dashes, and slightly more
readable.

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:27: virtual
~BrowserUIPrefsMigrationObserver();
Make the destructor private if the object deletes itself.

[dgrogan, 2014-09-09 23:34:51]

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator.cc (right):

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.cc:30: if
(!pref_store_->GetMutableValue("browser", &browser_value)) {
On 2014/09/09 09:12:53, Bernhard Bauer wrote:
> No braces for one-line bodies.

Done.

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator.h (right):

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:12: // previously-dynamically-registered
preferences to their new home in a
On 2014/09/09 09:12:53, Bernhard Bauer wrote:
> Comment nit: I think this is fine without the dashes, and slightly more
> readable.

Done.

https://codereview.chromium.org/511393003/diff/280001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:27: virtual
~BrowserUIPrefsMigrationObserver();
On 2014/09/09 09:12:53, Bernhard Bauer wrote:
> Make the destructor private if the object deletes itself.

Done.

[dgrogan, 2014-09-09 23:37:30]

Patchset #15 (id:280001) has been deleted

[Bernhard Bauer, 2014-09-10 09:15:48]

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator.h (right):

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:22: class BrowserUIPrefsMigrationObserver :
public PrefStore::Observer {
Class name and file name should correspond, to make it easier to find where a
class is defined. I would be fine with either class name (PrefsMigrator is nice
and short), although this class only handles migrating some preferences, so
maybe we should qualify the name to reflect that (so, BrowserUIPrefsMigrator as
a middle ground?).

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:26: virtual void OnPrefValueChanged(const
std::string& key) OVERRIDE {}
Add a blank line before these methods, followed by a comment like
"PrefStore::Observer implementation".

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator_unittest.cc (right):

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:25: // Get the value for a given
preference |key| and stores it in |*result|.
Nit: I don't think these comments are necessary.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:96: }  // unnamed namespace
Nit: Usually the closing comment for a namespace is exactly the declaration that
opened it, i.e. just "namespace" here.

https://codereview.chromium.org/511393003/diff/320001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/511393003/diff/320001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:829: 'browser/ui/prefs_migrator.cc',
Move this into prefs/?

[gab, 2014-09-10 14:40:05]

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator.h (right):

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:22: class BrowserUIPrefsMigrationObserver :
public PrefStore::Observer {
On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> Class name and file name should correspond, to make it easier to find where a
> class is defined. I would be fine with either class name (PrefsMigrator is
nice
> and short), although this class only handles migrating some preferences, so
> maybe we should qualify the name to reflect that (so, BrowserUIPrefsMigrator
as
> a middle ground?). 

+1 on keeping the specific name (BrowserUIPrefsMigrator).

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:30: friend struct
base::DefaultDeleter<BrowserUIPrefsMigrationObserver>;
Meh.. this also lets anybody own this and delete it from a scoped_ptr, but I
guess that's better than a public destructor... at least it may raise a flag to
a future reader that this isn't your typical ownership model.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:34: scoped_refptr<WriteablePrefStore>
pref_store_;
Use a weak pointer here; the PrefStore shouldn't *have* to stay alive until the
read is complete and the migration has been done.

All other post processing steps hooked to the PrefStore do not own it per se.

Actually, a raw pointer should be fine here since OnInitializationCompleted is
called synchronously from the PrefStore itself (which thus must still exist...).

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator_unittest.cc (right):

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:25: // Get the value for a given
preference |key| and stores it in |*result|.
On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> Nit: I don't think these comments are necessary.

+1, no need to copy method comments from derived APIs.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:36: return true;
Doesn't

return prefs_.Get(key, result);

do the same thing as lines 30-36?

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:47: if (!old_value ||
!value->Equals(old_value)) {
Do you really need these checks? Seems overkill to optimize this for tests.

prefs_.Set(key, new_value);
ReportValueChanged(key);

seems sufficient to me.

https://codereview.chromium.org/511393003/diff/320001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/511393003/diff/320001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:829: 'browser/ui/prefs_migrator.cc',
On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> Move this into prefs/?

Agreed since its only user is chrome_pref_service_factory.cc it should live
right beside it.

[dgrogan, 2014-09-10 23:10:00]

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator.h (right):

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:22: class BrowserUIPrefsMigrationObserver :
public PrefStore::Observer {
On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> Class name and file name should correspond, to make it easier to find where a
> class is defined. I would be fine with either class name (PrefsMigrator is
nice
> and short), although this class only handles migrating some preferences, so
> maybe we should qualify the name to reflect that (so, BrowserUIPrefsMigrator
as
> a middle ground?). 

Done.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:22: class BrowserUIPrefsMigrationObserver :
public PrefStore::Observer {
On 2014/09/10 14:40:04, gab wrote:
> On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> > Class name and file name should correspond, to make it easier to find where
a
> > class is defined. I would be fine with either class name (PrefsMigrator is
> nice
> > and short), although this class only handles migrating some preferences, so
> > maybe we should qualify the name to reflect that (so, BrowserUIPrefsMigrator
> as
> > a middle ground?). 
> 
> +1 on keeping the specific name (BrowserUIPrefsMigrator).

Done.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:26: virtual void OnPrefValueChanged(const
std::string& key) OVERRIDE {}
On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> Add a blank line before these methods, followed by a comment like
> "PrefStore::Observer implementation".

Done.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:30: friend struct
base::DefaultDeleter<BrowserUIPrefsMigrationObserver>;
On 2014/09/10 14:40:04, gab wrote:
> Meh.. this also lets anybody own this and delete it from a scoped_ptr, but I
> guess that's better than a public destructor... at least it may raise a flag
to
> a future reader that this isn't your typical ownership model.

Acknowledged.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator.h:34: scoped_refptr<WriteablePrefStore>
pref_store_;
On 2014/09/10 14:40:04, gab wrote:
> Use a weak pointer here; the PrefStore shouldn't *have* to stay alive until
the
> read is complete and the migration has been done.
> 
> All other post processing steps hooked to the PrefStore do not own it per se.
> 
> Actually, a raw pointer should be fine here since OnInitializationCompleted is
> called synchronously from the PrefStore itself (which thus must still
exist...).

Switched to raw.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs_migrator_unittest.cc (right):

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:25: // Get the value for a given
preference |key| and stores it in |*result|.
On 2014/09/10 14:40:04, gab wrote:
> On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> > Nit: I don't think these comments are necessary.
> 
> +1, no need to copy method comments from derived APIs.

Done.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:25: // Get the value for a given
preference |key| and stores it in |*result|.
On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> Nit: I don't think these comments are necessary.

Done.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:36: return true;
On 2014/09/10 14:40:05, gab wrote:
> Doesn't
> 
> return prefs_.Get(key, result);
> 
> do the same thing as lines 30-36?

I copied this from JsonPrefStore and was wondering the same thing.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:47: if (!old_value ||
!value->Equals(old_value)) {
On 2014/09/10 14:40:04, gab wrote:
> Do you really need these checks? Seems overkill to optimize this for tests.
> 
> prefs_.Set(key, new_value);
> ReportValueChanged(key);
> 
> seems sufficient to me.

Done. Again, I had just copy/pasted from JsonPrefStore.

https://codereview.chromium.org/511393003/diff/320001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs_migrator_unittest.cc:96: }  // unnamed namespace
On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> Nit: Usually the closing comment for a namespace is exactly the declaration
that
> opened it, i.e. just "namespace" here.

Done.

https://codereview.chromium.org/511393003/diff/320001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/511393003/diff/320001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:829: 'browser/ui/prefs_migrator.cc',
On 2014/09/10 14:40:05, gab wrote:
> On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> > Move this into prefs/?
> 
> Agreed since its only user is chrome_pref_service_factory.cc it should live
> right beside it.

Done.

https://codereview.chromium.org/511393003/diff/320001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:829: 'browser/ui/prefs_migrator.cc',
On 2014/09/10 09:15:47, Bernhard Bauer wrote:
> Move this into prefs/?

Done.

[dgrogan, 2014-09-11 00:54:44]

This is ready for another look.

Suggestions welcome for how to better handle the linker errors encountered by
the android bots in patch 17. I don't see how to get that symbol defined when
the source file holding its definition
(chrome/browser/devtools/devtools_window.cc) is excluded on android in
chrome/chrome_debugger.gypi.

[dgrogan, 2014-09-11 00:57:09]

For ease of reference, the linker error on android is
../../chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc:126:error:
undefined reference to 'DevToolsWindow::kDevToolsApp'

[Bernhard Bauer, 2014-09-11 09:48:28]

LGTM!

https://codereview.chromium.org/511393003/diff/360001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc (right):

https://codereview.chromium.org/511393003/diff/360001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc:89: #if
!defined(OS_ANDROID)
I would be okay with excluding the whole test on Android, as we don't have
windows there anyway.

[dgrogan, 2014-09-11 18:19:40]

dgrogan@chromium.org changed reviewers:
+ pkasting@chromium.org, vsevik@chromium.org

[dgrogan, 2014-09-11 18:19:41]

pkasting, can you review as OWNER of chrome/browser/ui ?
vsevik, can you review as OWNER of chrome/browser/devtools?

[Peter Kasting, 2014-09-11 20:31:50]

LGTM

https://codereview.chromium.org/511393003/diff/400001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/511393003/diff/400001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.cc:89: .PassAs<DictionaryPrefUpdate>();
Nit: Just do

  return scoped_ptr<DictionaryPrefUpdate>(
      new WindowPlacementPrefUpdate(prefs, window_name));

(add .Pass() if necessary)

make_scoped_ptr() becomes not worth it semantic sugar-wise once you have to
modify the returned type.

https://codereview.chromium.org/511393003/diff/400001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.h (right):

https://codereview.chromium.org/511393003/diff/400001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.h:33: const Browser* browser);
It seems a bit curious that these methods would take such seemingly-unrelated
arguments.  Is making them match impossible?

Consider adding comments on these regardless (what is a window placement
dictionary?  What do the args mean?  When can these fail?)

[dgrogan, 2014-09-12 01:23:03]

https://codereview.chromium.org/511393003/diff/400001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.cc (right):

https://codereview.chromium.org/511393003/diff/400001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.cc:89: .PassAs<DictionaryPrefUpdate>();
On 2014/09/11 20:31:50, Peter Kasting wrote:
> Nit: Just do
> 
>   return scoped_ptr<DictionaryPrefUpdate>(
>       new WindowPlacementPrefUpdate(prefs, window_name));
> 
> (add .Pass() if necessary)
> 
> make_scoped_ptr() becomes not worth it semantic sugar-wise once you have to
> modify the returned type.

Done.

https://codereview.chromium.org/511393003/diff/400001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window_state.h (right):

https://codereview.chromium.org/511393003/diff/400001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window_state.h:33: const Browser* browser);
On 2014/09/11 20:31:50, Peter Kasting wrote:
> It seems a bit curious that these methods would take such seemingly-unrelated
> arguments.  Is making them match impossible?

I'd wanted to avoid the FindPreference call in the implementation of
"ReadWrite". But since it's in "ReadWrite" it might as well be in "ReadOnly" as
well. Changed.

> Consider adding comments on these regardless (what is a window placement
> dictionary?  What do the args mean?  When can these fail?)
Done.

[gab, 2014-09-12 13:47:57]

lgtm w/ test nits/recommendations

Woot!
Gab

https://codereview.chromium.org/511393003/diff/440001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc (right):

https://codereview.chromium.org/511393003/diff/440001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc:84: const char*
kAppName("localhost_/chrome/6/src/chrome/test/data/title1.html");
Why such a specific test name? Where does this come from?

https://codereview.chromium.org/511393003/diff/440001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc:100:
prefs::kBrowserWindowPlacement + std::string("_") + kAppName, NULL));
To test the test itself (i.e. to make sure this returns false because the item
was removed not because you have a typo in this query): how about saving
"prefs::kBrowserWindowPlacement + std::string("_") + kAppName" into a const
string and using it to set, test pre-migration, test post-migration?

https://codereview.chromium.org/511393003/diff/440001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc:125: EXPECT_EQ(1000,
out_value);
Also test kBrowserWindowPlacementPopup to have full coverage?

[dgrogan, 2014-09-12 19:05:40]

https://codereview.chromium.org/511393003/diff/440001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc (right):

https://codereview.chromium.org/511393003/diff/440001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc:84: const char*
kAppName("localhost_/chrome/6/src/chrome/test/data/title1.html");
On 2014/09/12 13:47:56, gab wrote:
> Why such a specific test name? Where does this come from?

I copy/pasted that from my local Preferences file after running chrome with
--app=http://localhost/chrome/6/src/chrome/test/data/title1.html.

Changed to a more generic name.

https://codereview.chromium.org/511393003/diff/440001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc:100:
prefs::kBrowserWindowPlacement + std::string("_") + kAppName, NULL));
On 2014/09/12 13:47:56, gab wrote:
> To test the test itself (i.e. to make sure this returns false because the item
> was removed not because you have a typo in this query): how about saving
> "prefs::kBrowserWindowPlacement + std::string("_") + kAppName" into a const
> string and using it to set, test pre-migration, test post-migration?

Done.

https://codereview.chromium.org/511393003/diff/440001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_ui_prefs_migrator_unittest.cc:125: EXPECT_EQ(1000,
out_value);
On 2014/09/12 13:47:56, gab wrote:
> Also test kBrowserWindowPlacementPopup to have full coverage?

Done.

[dgrogan, 2014-09-12 19:13:42]

dgrogan@chromium.org changed reviewers:
+ yurys@chromium.org

[dgrogan, 2014-09-12 19:13:43]

Yury, can you review as OWNER of chrome/browser/devtools if vsevik@ can't get to
it?

[dgrogan, 2014-09-16 04:12:27]

dgrogan@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgrogan, 2014-09-16 04:12:29]

dgozman, you're next up in the search for an OWNER of chrome/browser/devtools

[dgozman, 2014-09-16 04:28:02]

chrome/browser/devtools lgtm

[dgrogan, 2014-09-16 05:34:27]

The CQ bit was checked by dgrogan@chromium.org

[commit-bot: I haz the power, 2014-09-16 05:34:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/511393003/460001

[commit-bot: I haz the power, 2014-09-16 05:37:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-16 05:37:58]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[dgrogan, 2014-09-16 21:13:50]

The CQ bit was checked by dgrogan@chromium.org

[commit-bot: I haz the power, 2014-09-16 21:15:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/511393003/480001

[commit-bot: I haz the power, 2014-09-16 22:41:19]

Committed patchset #24 (id:480001) as 20789e1e6fd130b27ac991b13a52c2d6b23552aa

[commit-bot: I haz the power, 2014-09-16 22:41:58]

Patchset 24 (id:??) landed as
https://crrev.com/e30f8f0957be8ffa94a70dc9e8e6f61c9b2990c5
Cr-Commit-Position: refs/heads/master@{#295163}