Introduce ChromeZoomLevelPref, make zoom level prefs independent of profile prefs.

Introduce ChromeZoomLevelPrefs, make zoom level prefs independent of profile prefs.

This CL splits the management of zoom level prefs (per-host
zoom levels and default zoom level) out of the profile
prefs and into its own class, ChromeZoomLevelPrefs. This is
in preparation for moving zoom-level prefs into StoragePartition so that WebViews and Apps can manage them
separately from the main browser.

Note that while the new zoom level prefs are still stored
in the profile prefs, default_zoom_level becomes a
Dictionary of doubles, instead of being a single double.
This is needed since now each StoragePartition will
have its own default zoom level. Similarly, per-host zoom
levels is now a Dictionary of Dictionaries, with each
StoragePartition having its own set of per-host zoom
levels.

This CL includes code that detects legacy profile zoom
preferences, and migrates them to the new mechanism,
clearing the legacy values once the migration is complete.
This mechanism will be removed (at least) one
full milestone after the new zoom preferences go stable,
when analysis of the migration statistics suggests that most users have had their preferences migrated.

BUG=335317
Committed: https://crrev.com/e530aa74d7615e92810410b451471802cd79edda
Cr-Commit-Position: refs/heads/master@{#299559}

[wjmaclean, 2014-09-04 20:41:17]

wjmaclean@chromium.org changed reviewers:
+ jam@chromium.org, sky@chromium.org

[wjmaclean, 2014-09-04 20:41:17]

jam@chromium.org: Please review changes in content/public (small CL)

sky@chromium.org: Please review changes in chrome/
(larger CL)

For a look at the "bigger picture" you may refer to
https://codereview.chromium.org/393133002/ as it shows the full migration path.

[sky, 2014-09-04 21:52:24]

I only got a little ways through this patch. Can you make sure there aren't
going to be any sync issues related to this? Say you migrate the prefs on
machine 1, and machine 2 syncs (but is on an older version) will it lose all the
zoom prefs?

https://codereview.chromium.org/541103002/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:769: // As part of the migration from
per-profile to per-partiion HostZoomMaps,
partition

[wjmaclean, 2014-09-05 01:42:41]

On 2014/09/04 21:52:24, sky wrote:
> I only got a little ways through this patch. Can you make sure there aren't
> going to be any sync issues related to this? Say you migrate the prefs on
> machine 1, and machine 2 syncs (but is on an older version) will it lose all
the
> zoom prefs?
> 
>
https://codereview.chromium.org/541103002/diff/1/chrome/browser/profiles/prof...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://codereview.chromium.org/541103002/diff/1/chrome/browser/profiles/prof...
> chrome/browser/profiles/profile_impl.cc:769: // As part of the migration from
> per-profile to per-partiion HostZoomMaps,
> partition

My initial impression (based on the many Chrome instances I use) is that zoom
settings aren't part of what gets sync'd, which makes sense to me in that zoom
settings don't necessarily mean the same thing across different screens on
different devices.

That being said, I'll look through the code and try and come up with something a
little more authoritative.

[wjmaclean, 2014-09-05 12:04:12]

On 2014/09/05 01:42:41, wjmaclean wrote:
>
> That being said, I'll look through the code and try and come up with something
a
> little more authoritative.

So looking at

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

I see that both default zoom level and per-host zoom levels are marked as

user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF

which I assume prevents them from being sync'd.

[wjmaclean, 2014-09-05 16:12:46]

Fixed broken test (needed to temporarily add ZoomLevelPrefs to TestingProfile
... this will eventually be removed as this will be automatically instantiated
with the TestingProfile's default StoragePartition).

[sky, 2014-09-05 19:28:08]

I realized when looking through all this that I'm not that familiar with host
zoom maps. Is there a better reviewer you can find? Maybe dbeam?

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:247: const double
kOriginalDefaultZoomLevel;
AFAICT there is no reason for this to be a member. Make it static.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:769: // As part of the migration from
per-profile to per-partiion HostZoomMaps,
partition

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc (right):

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:27: void
ZoomLevelPrefsStoreImpl::SetInitCallback(const base::Closure& callback) {
Make order match header.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:49: 0.0,
Why is the default 0 and not 1?

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:71: if (key ==
prefs::kDefaultZoomLevel)
nit: I would use {} as it's multiple lines.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:77: if (!succeeded) {
nit: no {}

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:80: double
default_zoom_level =
Add a comment as to what this does.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:141: if
(zoom_level_prefs_.get())
Why the conditional here?

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h (right):

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:29: void
OnZoomLevelChanged(const content::HostZoomMap::ZoomLevelChange& change);
nit: newline between 28/29.
Why is this public?

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:45: 
nit: remove newline.

https://codereview.chromium.org/541103002/diff/40001/content/public/browser/z...
File content/public/browser/zoom_level_prefs_store.h (right):

https://codereview.chromium.org/541103002/diff/40001/content/public/browser/z...
content/public/browser/zoom_level_prefs_store.h:21: class ZoomLevelPrefsStore {
Why does this need to be in content?

https://codereview.chromium.org/541103002/diff/40001/content/public/browser/z...
content/public/browser/zoom_level_prefs_store.h:25: virtual
~ZoomLevelPrefsStore() {}
nit: constructor/destructor comes before other functions.

[wjmaclean, 2014-09-05 21:01:48]

Addressed comments.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:247: const double
kOriginalDefaultZoomLevel;
On 2014/09/05 19:28:07, sky wrote:
> AFAICT there is no reason for this to be a member. Make it static.

Done.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:769: // As part of the migration from
per-profile to per-partiion HostZoomMaps,
On 2014/09/05 19:28:07, sky wrote:
> partition

Done.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc (right):

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:27: void
ZoomLevelPrefsStoreImpl::SetInitCallback(const base::Closure& callback) {
On 2014/09/05 19:28:07, sky wrote:
> Make order match header.

Done.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:49: 0.0,
On 2014/09/05 19:28:07, sky wrote:
> Why is the default 0 and not 1?

Zoom levels are logarithmic, so level = 0 => zoom factor = 1, factors > 1 have
levels > 0, and factors < 1 have levels < 0.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:71: if (key ==
prefs::kDefaultZoomLevel)
On 2014/09/05 19:28:07, sky wrote:
> nit: I would use {} as it's multiple lines.

Done. Thanks for catching that!

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:77: if (!succeeded) {
On 2014/09/05 19:28:07, sky wrote:
> nit: no {}

Done. Ditto!

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:80: double
default_zoom_level =
On 2014/09/05 19:28:07, sky wrote:
> Add a comment as to what this does.

Done.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:141: if
(zoom_level_prefs_.get())
On 2014/09/05 19:28:07, sky wrote:
> Why the conditional here?

Done. Looking at this again, this is leftover from an older version of the CL,
and is unecessary.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h (right):

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:29: void
OnZoomLevelChanged(const content::HostZoomMap::ZoomLevelChange& change);
On 2014/09/05 19:28:07, sky wrote:
> nit: newline between 28/29.
> Why is this public?

Done.

https://codereview.chromium.org/541103002/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:45: 
On 2014/09/05 19:28:07, sky wrote:
> nit: remove newline.

Done.

https://codereview.chromium.org/541103002/diff/40001/content/public/browser/z...
File content/public/browser/zoom_level_prefs_store.h (right):

https://codereview.chromium.org/541103002/diff/40001/content/public/browser/z...
content/public/browser/zoom_level_prefs_store.h:21: class ZoomLevelPrefsStore {
On 2014/09/05 19:28:07, sky wrote:
> Why does this need to be in content?

Ultimately this is an interface that will be accessed in content; I included
this here now to minimize changes later when we go to use it.

https://codereview.chromium.org/541103002/diff/40001/content/public/browser/z...
content/public/browser/zoom_level_prefs_store.h:25: virtual
~ZoomLevelPrefsStore() {}
On 2014/09/05 19:28:07, sky wrote:
> nit: constructor/destructor comes before other functions.

Done.

[Dan Beam, 2014-09-09 22:47:18]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2014-09-09 22:47:19]

i can look but i've always dealt with *showing* the zoom level, not storing or
communicating it really...  I'd recommend more blame sleuthing.  my guess would
be jam@ or creis@ maybe?

[wjmaclean, 2014-09-09 22:54:09]

I've asked jam@ to see what he recommends

On Tue, Sep 9, 2014 at 6:47 PM, <dbeam@chromium.org> wrote:

> i can look but i've always dealt with *showing* the zoom level, not
> storing or
> communicating it really...  I'd recommend more blame sleuthing.  my guess
> would
> be jam@ or creis@ maybe?
>
> https://codereview.chromium.org/541103002/
>

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

[jam, 2014-09-10 15:35:33]

why is content changing? see
http://www.chromium.org/developers/content-module/content-api

for stuff in chrome, i believe pkasting is familiar with this

[wjmaclean, 2014-09-10 15:45:20]

On 2014/09/10 15:35:33, jam wrote:
> why is content changing? see
> http://www.chromium.org/developers/content-module/content-api

When all the move-HostZoomMap-to_StoragePartition CLs have landed, one will need
to be able to store a prefs management object within the StoragePartition, and
ZoomLevelPrefsStore is the interface for that. I can delayed introducing this
until the CL that actually modifies StoragePartition, but I thought it would
create less churn if I introduced it at the same time as
ZoomLevelPrefsStoreImpl.

> 
> for stuff in chrome, i believe pkasting is familiar with this

Thanks, I'll ask pkasting@ to take a look.

[wjmaclean, 2014-09-10 15:45:45]

wjmaclean@chromium.org changed reviewers:
+ pkasting@chromium.org
- sky@chromium.org

[wjmaclean, 2014-09-10 15:48:16]

pkasting@ - would you be willing to take at look at the chrome/* parts of this
CL?

There's an overview doc (see link below) to help explain where this CL fits into
the larger picture, or I'm happy to give you an overview on VC if that's easier.

https://docs.google.com/a/chromium.org/document/d/1SsWXDI0QGbRU1EQ6NQy8l1YLrZ...

[Peter Kasting, 2014-09-11 00:45:48]

Unfortunately, I haven't worked with the zoom code since it was first written
years ago... I think you want bauerb or jochen maybe?  In the interest of not
being totally useless, here are some review comments anyway (but I'm not
qualified to really review what you're doing very deeply).

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/signin/merge_session_load_page.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/signin/merge_session_load_page.cc:107:
profile)->GetDefaultZoomLevel();
What about
ZoomController::FromWebContents(web_contents_)->GetDefaultZoomLevel()?  That's
what you use in chrome_page_zoom.cc.

Same comment applies elsewhere you do this.

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:146: std::string
prefs_filename_;
Nit: Rather than making these protected, have the constructor set them.  (The
Google style guide discourages protected members, and you only need them
protected so the subclass constructors can write to them.)

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:777: // it goes stable. This means
removing this entire function.
You're going to want to wait quite a bit longer than one milestone.  In general
we tend to leave migration code in for many milestones in case people don't use
Chrome for a while and then come back to it.

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:818: // remember them, so that we can
later erase them from Prefs.
This comment seems out of date?

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:836: // We're done imigrating the
profile per-host zoom level values, so we clear
Nit: Spelling.  (I'd just say "migrating", that's clearer and easier to spell.)

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/prefs/...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/prefs/...
chrome/browser/ui/prefs/prefs_tab_helper.cc:338: // have finished initializing,
causing a stale pointer via the callback?
I would worry that it can; imagine shutting down Chrome as it's still starting
up.  Not sure though.

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:30: // Implement
ZoomLevelPrefsStore.
Tiny nit: I'd make these comments look like "// content::ZoomLevelPrefsStore:",
which is about as short as possible and matches the superclass name exactly.

Also, generally try to give override blocks in the same order as the actual
superclasses are listed above.

[wjmaclean, 2014-09-11 21:00:52]

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/signin/merge_session_load_page.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/signin/merge_session_load_page.cc:107:
profile)->GetDefaultZoomLevel();
On 2014/09/11 00:45:47, Peter Kasting wrote:
> What about
> ZoomController::FromWebContents(web_contents_)->GetDefaultZoomLevel()?  That's
> what you use in chrome_page_zoom.cc.
> 
> Same comment applies elsewhere you do this.

Good catch, better to keep it consistent (and this eliminates the need for the
TODO as well).

Done.

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:146: std::string
prefs_filename_;
On 2014/09/11 00:45:47, Peter Kasting wrote:
> Nit: Rather than making these protected, have the constructor set them.  (The
> Google style guide discourages protected members, and you only need them
> protected so the subclass constructors can write to them.)

Done.

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:777: // it goes stable. This means
removing this entire function.
On 2014/09/11 00:45:47, Peter Kasting wrote:
> You're going to want to wait quite a bit longer than one milestone.  In
general
> we tend to leave migration code in for many milestones in case people don't
use
> Chrome for a while and then come back to it.

Sure, I was wondering about that. Is there any accepted guideline?

Done.

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:818: // remember them, so that we can
later erase them from Prefs.
On 2014/09/11 00:45:47, Peter Kasting wrote:
> This comment seems out of date?

Done.

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:836: // We're done imigrating the
profile per-host zoom level values, so we clear
On 2014/09/11 00:45:47, Peter Kasting wrote:
> Nit: Spelling.  (I'd just say "migrating", that's clearer and easier to
spell.)

Ha ha ... the 'i' was from vi ... 'migrating was intended all along ... no need
to get immigration involved in all this ;-)

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/prefs/...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/prefs/...
chrome/browser/ui/prefs/prefs_tab_helper.cc:338: // have finished initializing,
causing a stale pointer via the callback?
On 2014/09/11 00:45:47, Peter Kasting wrote:
> I would worry that it can; imagine shutting down Chrome as it's still starting
> up.  Not sure though.

I've attempted to address this concern by using a WeakPtr in the callback. I'm
not sure how to definitively test this ... is it the correct approach?

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:30: // Implement
ZoomLevelPrefsStore.
On 2014/09/11 00:45:48, Peter Kasting wrote:
> Tiny nit: I'd make these comments look like "//
content::ZoomLevelPrefsStore:",
> which is about as short as possible and matches the superclass name exactly.
> 
> Also, generally try to give override blocks in the same order as the actual
> superclasses are listed above.

Done.

[Peter Kasting, 2014-09-11 21:06:55]

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

[Peter Kasting, 2014-09-11 21:06:55]

I don't have problems with this change, but I don't think my review is good
enough for me to approve it, so I'm going to leave you in the hands of bauerb in
hope he understands zoom better as I previously suggested.

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/prefs/...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/prefs/...
chrome/browser/ui/prefs/prefs_tab_helper.cc:338: // have finished initializing,
causing a stale pointer via the callback?
On 2014/09/11 21:00:51, wjmaclean wrote:
> On 2014/09/11 00:45:47, Peter Kasting wrote:
> > I would worry that it can; imagine shutting down Chrome as it's still
starting
> > up.  Not sure though.
> 
> I've attempted to address this concern by using a WeakPtr in the callback. I'm
> not sure how to definitively test this ... is it the correct approach?

Sadly, I think we really have to know definitively whether this is a potential
problem or not.

If we understand whether this can happen, it should become clearer whether it's
testable.  If we don't know for sure, we shouldn't try and introduce safety
measures on the off chance something goes wrong.

Maybe jam or avi know enough about startup control flow to know whether there's
a problem?

[Bernhard Bauer, 2014-09-12 10:28:50]

How is this going to work in the long term? Is every app going to have its own
PrefService (and JSON prefs file) to store zoom settings?

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/prefs/...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/60001/chrome/browser/ui/prefs/...
chrome/browser/ui/prefs/prefs_tab_helper.cc:338: // have finished initializing,
causing a stale pointer via the callback?
On 2014/09/11 21:06:55, Peter Kasting wrote:
> On 2014/09/11 21:00:51, wjmaclean wrote:
> > On 2014/09/11 00:45:47, Peter Kasting wrote:
> > > I would worry that it can; imagine shutting down Chrome as it's still
> starting
> > > up.  Not sure though.
> > 
> > I've attempted to address this concern by using a WeakPtr in the callback.
I'm
> > not sure how to definitively test this ... is it the correct approach?
> 
> Sadly, I think we really have to know definitively whether this is a potential
> problem or not.
> 
> If we understand whether this can happen, it should become clearer whether
it's
> testable.  If we don't know for sure, we shouldn't try and introduce safety
> measures on the off chance something goes wrong.
> 
> Maybe jam or avi know enough about startup control flow to know whether
there's
> a problem?

Well, I don't know of anything that would make this _not_ a problem... You're
bringing up your own PrefService which loads its preferences asynchronously, so
it might call you back at any time.

Of course, it's difficult to _prove_ that this actually happens (because it
might not happen every time). What you could do is try to (empirically) prove
the opposite via DCHECK(!weak_ptr_factory_.HasWeakPtrs()) when this object is
destroyed.

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:142:
HostZoomMapBrowserTestWithPrefs(std::string prefs_data,
Pass the strings by (const)reference?

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:172: :  // Zoom-related
preferences demonstrating the two problems that could
Indentation / line breaks are off here. Also, do you actually need to squeeze
the comment in between the class declaration and its superclass? You could put
it e.g. before the actual string.

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc (right):

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:7: //#include
"base/files/file_path.h"
Remove

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile.h:224: virtual PrefService* GetZoomLevelPrefs();
Instead of adding a new method to Profile, could you make this a KeyedService?

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc (right):

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:28:
zoom_subscription_.reset();
Why do you need this if you're being destroyed anyway?

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:119:
PrefServiceSyncableFactory factory;
Do we really want a syncable prefservice?

https://codereview.chromium.org/541103002/diff/80001/content/public/browser/z...
File content/public/browser/zoom_level_prefs_store.h (right):

https://codereview.chromium.org/541103002/diff/80001/content/public/browser/z...
content/public/browser/zoom_level_prefs_store.h:18: // An interface to manage a
prefs store for (1) default zoom level, and
What exactly is a "prefs store" in this context? I'm asking because there is the
PrefStore class which is used by PrefService, but this isn't quite the same
thing, right?

https://codereview.chromium.org/541103002/diff/80001/content/public/browser/z...
content/public/browser/zoom_level_prefs_store.h:27: virtual PrefService*
GetPrefs() = 0;
This is a layering violation; content doesn't know about PrefService. Why does
this need to be in content anyway?

[wjmaclean, 2014-09-16 18:44:44]

While I work on (1) making the pref service non-syncable, and (2) exposing
GetPrefs() only from ZoomLevelPrefStoreImpl, I'd like to reply to the other
comments in your review to get further feedback.

> How is this going to work in the long term? Is every app
> going to have its own PrefService (and JSON prefs file) to
> store zoom settings?

Not every app per-se, but every storage partition. Any apps that share a storage
partition will share zoom prefs as well. WebViews also get their own storage
partitions, which allows us to isolate the zoom levels in a WebView embedded in
an app from being affected by zoom level changes in, for example, the main
browser window, or another app.

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:142:
HostZoomMapBrowserTestWithPrefs(std::string prefs_data,
On 2014/09/12 10:28:49, Bernhard Bauer wrote:
> Pass the strings by (const)reference?

Done.

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:172: :  // Zoom-related
preferences demonstrating the two problems that could
On 2014/09/12 10:28:49, Bernhard Bauer wrote:
> Indentation / line breaks are off here. Also, do you actually need to squeeze
> the comment in between the class declaration and its superclass? You could put
> it e.g. before the actual string.

Done.

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc (right):

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:7: //#include
"base/files/file_path.h"
On 2014/09/12 10:28:49, Bernhard Bauer wrote:
> Remove

Opps, removed!

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile.h:224: virtual PrefService* GetZoomLevelPrefs();
On 2014/09/12 10:28:49, Bernhard Bauer wrote:
> Instead of adding a new method to Profile, could you make this a KeyedService?

I could make it a keyede service, but hopefully this will be removed within a
week (or at most two) from when this CL lands, so is it worth it?

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc (right):

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:28:
zoom_subscription_.reset();
On 2014/09/12 10:28:49, Bernhard Bauer wrote:
> Why do you need this if you're being destroyed anyway?

Because zoom_level_prefs_ gets destructed before this, and has a DCHECK in it's
destructor that complains if the observer list isn't empty at time of
destruction.

https://codereview.chromium.org/541103002/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:119:
PrefServiceSyncableFactory factory;
On 2014/09/12 10:28:49, Bernhard Bauer wrote:
> Do we really want a syncable prefservice?

Possibly not ... I did this originally to be consistent with the main profile
prefs. I can't think however of a case where we would sync zoom levels though,
so I'll convert it to non-syncable.

https://codereview.chromium.org/541103002/diff/80001/content/public/browser/z...
File content/public/browser/zoom_level_prefs_store.h (right):

https://codereview.chromium.org/541103002/diff/80001/content/public/browser/z...
content/public/browser/zoom_level_prefs_store.h:18: // An interface to manage a
prefs store for (1) default zoom level, and
On 2014/09/12 10:28:49, Bernhard Bauer wrote:
> What exactly is a "prefs store" in this context? I'm asking because there is
the
> PrefStore class which is used by PrefService, but this isn't quite the same
> thing, right?

I guess the best way to describe this is to say that this is an interface for a
delegate that looks after initiallizing per-host zoom-level prefs (as was done
previously in profile), and also gives the embedder the option to persist them
(though content doesn't need to know about that part).

Is the name of the class problematic for content?

https://codereview.chromium.org/541103002/diff/80001/content/public/browser/z...
content/public/browser/zoom_level_prefs_store.h:27: virtual PrefService*
GetPrefs() = 0;
On 2014/09/12 10:28:49, Bernhard Bauer wrote:
> This is a layering violation; content doesn't know about PrefService.

Ok. I suppose I can remove this, and have clients in chrome convert to
ZoomLevelPrefsStoreImpl to get access.

> Why does this need to be in content anyway?

I assume by "this" you mean this entire interface? Because when HostZoomMap
moves to StoragePartition, there needs to be a way to allow the embedder to
initialize (and possibly persist) zoom level values. Since the lifetime of the
HostZoomMap will managed by StoragePartition, it makes sense for the delegate to
live there also.

An overview of the full design is available at
https://docs.google.com/a/chromium.org/document/d/1SsWXDI0QGbRU1EQ6NQy8l1YLrZ...

[jam, 2014-09-18 01:15:10]

On 2014/09/10 15:45:20, wjmaclean wrote:
> On 2014/09/10 15:35:33, jam wrote:
> > why is content changing? see
> > http://www.chromium.org/developers/content-module/content-api
> 
> When all the move-HostZoomMap-to_StoragePartition CLs have landed, one will
need
> to be able to store a prefs management object within the StoragePartition, and
> ZoomLevelPrefsStore is the interface for that. I can delayed introducing this
> until the CL that actually modifies StoragePartition, but I thought it would
> create less churn if I introduced it at the same time as
> ZoomLevelPrefsStoreImpl.

yes please don't add it until content uses it
> 
> > 
> > for stuff in chrome, i believe pkasting is familiar with this
> 
> Thanks, I'll ask pkasting@ to take a look.

[wjmaclean, 2014-09-30 20:31:36]

Patchset #7 (id:120001) has been deleted

[wjmaclean, 2014-09-30 21:04:19]

Conversion to using only profile-prefs done; PTAL

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

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:79:
browser()->profile()->GetZoomLevelPrefs()->SetDefaultZoomLevel(level);
If you want to, you could pull these changes out into a separate CL.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:180: "{'partition': {"
Could you put this together with the comment into a constant, so you don't have
to inline it here?

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:146: prefs::kProfileDefaultZoomLevel,
This is what I meant in my email: You're registering a preference which is a
prefix of another preference. That is very dangerous and might break at any
time. For example, someone could write a dictionary to
profile.default_zoom_level which doesn't contain a double for the key "0". Then
when you're trying to read profile.default_zoom_level.0, you'll get back the
wrong type, which will DCHECK. I think you'll need to implement the type
checking and default value logic anyway for arbitrary subdictionaries under
profile.default_zoom_level, so registering profile.default_zoom_level.0 doesn't
get you anything you couldn't get otherwise.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:43: namespace chrome {
Nit: I'm not sure exactly why, but I feel like this should come before
chrome_browser_net.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:786: base::StatsCounter
zoom_level_preferences_migration_counter(
Any reason why you're doing this instead of
base::RecordAction(UserMetricsAction(...))?

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:323:
web_contents->GetBrowserContext())->GetDefaultZoomLevel();
Align this just four spaces w/r/t the previous line.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:344:
zoom_level_prefs->GetDefaultZoomLevelPath().c_str(),
Hm, this is problematic. Like I said above you can't register keys that are
prefixes of other keys (or at least you can't rely on the fact that they share
subdictionaries), but that means you can't easily observe changes. You might
have to watch for changes to the top-level dictionary.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:392: registrar_.Add(
Indent is off here.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:48:
web_contents_.get())->GetDefaultZoomLevel();
Also indent just four spaces w/r/t the previous line here.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:55: key_stream << int_key;
I don't think we use streams.

Could you use base::IntToString (from base/strings/string_number_conversions.h)?

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:21: // PrefStore::Observer so
that it can track changes to the prefs.
This comment doesn't seem to be correct.

[wjmaclean, 2014-10-01 20:32:58]

wjmaclean@chromium.org changed reviewers:
+ asvitkine@chromium.org, gab@chromium.org

[wjmaclean, 2014-10-01 20:33:00]

asvitkine@chromium.org: Please review changes in tools/metrics/actions

gab@ - bauerb@ suggested you might take over the chrome/browser/prefs part of
this (I've addressed a couple of rounds of comments from bauerb@ so hopefully it
won't be too difficult).

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:180: "{'partition': {"
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> Could you put this together with the comment into a constant, so you don't
have
> to inline it here?

Done.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:146: prefs::kProfileDefaultZoomLevel,
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> This is what I meant in my email: You're registering a preference which is a
> prefix of another preference. That is very dangerous and might break at any
> time. For example, someone could write a dictionary to
> profile.default_zoom_level which doesn't contain a double for the key "0".
Then
> when you're trying to read profile.default_zoom_level.0, you'll get back the
> wrong type, which will DCHECK. I think you'll need to implement the type
> checking and default value logic anyway for arbitrary subdictionaries under
> profile.default_zoom_level, so registering profile.default_zoom_level.0
doesn't
> get you anything you couldn't get otherwise.

I assume this comment was intended for line 151 (below) ... this line of code is
part of the legacy prefs for default zoom level, with a re-name of the constant
(prefs::kProfileDefaultZoomLevel).

But I understand that I cannot register both prefs::kDefaultZoomLevel and
prefs::kDefaultZoomLevel + ".0".

The main reason that I wanted to register partition.default_zoom_level.0 was to
get notifications if somewhere else in the code modifies this value (this is the
default zoom level for the main browser window). So *not* registering this means
I will have to add custom notifications for this value.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:43: namespace chrome {
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> Nit: I'm not sure exactly why, but I feel like this should come before
> chrome_browser_net.

You're right ... sorting order is wrong here ... fixed.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:786: base::StatsCounter
zoom_level_preferences_migration_counter(
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> Any reason why you're doing this instead of
> base::RecordAction(UserMetricsAction(...))?

When I looked at UserAction() it seemed to be about things users initiate
(consciously). I'll look again.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:323:
web_contents->GetBrowserContext())->GetDefaultZoomLevel();
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> Align this just four spaces w/r/t the previous line.

Done.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:344:
zoom_level_prefs->GetDefaultZoomLevelPath().c_str(),
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> Hm, this is problematic. Like I said above you can't register keys that are
> prefixes of other keys (or at least you can't rely on the fact that they share
> subdictionaries), but that means you can't easily observe changes. You might
> have to watch for changes to the top-level dictionary.

As per our off-line discussion, this means I will have to force *all* access to
this parameter to go through ChromeZoomLevelPrefs so it can manage notifications
for any interested clients.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:392: registrar_.Add(
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> Indent is off here.

Done.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:48:
web_contents_.get())->GetDefaultZoomLevel();
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> Also indent just four spaces w/r/t the previous line here.

Done. The style-guide isn't terribly clear in this case, and auto-format has led
me astray here I guess.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:55: key_stream << int_key;
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> I don't think we use streams.
> 
> Could you use base::IntToString (from
base/strings/string_number_conversions.h)?

Done.

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:21: // PrefStore::Observer so
that it can track changes to the prefs.
On 2014/10/01 15:09:14, Bernhard Bauer wrote:
> This comment doesn't seem to be correct.

Done.

[Alexei Svitkine (slow), 2014-10-02 20:40:20]

https://codereview.chromium.org/541103002/diff/280001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:790:
UserMetricsAction("Chrome_ZoomLevelPreferencesMigration"));
I'm not convinced a user action is the best way to measure this.

For one, you don't have a baseline (just an absolute number - but you can't
answer questions such as what percentage of starts have migration).

Instead, I suggest using an enumerated histograms and recording both migrated
and non-migrated cases. That way, in addition to the total volume of these, you
can see how frequently it happens (i.e. vs. the non-migrated case).

Plus, histograms are better supported by our dashboards, so you can filter them
by different criteria, for example.

https://codereview.chromium.org/541103002/diff/280001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/280001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:57: LOG(WARNING) << "wjm: Hash
= " << partition_key_;
You probably want to remove this.

[gab, 2014-10-02 20:58:43]

I did a run through of this entire CL (skipped tests for now). I dropped
comments as I went, but I don't yet fully grasp it (perhaps because none of this
-- besides the prefs API which is used but not modified -- is under my area of
ownership).

I suggest:
1) Trying to break it down into smaller CLs if possible(?)
2) Expanding the CL description so that I understand what I'm looking at in
details.

For now it seems to me like you're migrating from one pref to another and it's
not clear why (and even how as there isn't a single migration point, making it
even harder to follow).

I also don't know much (if anything) about the UI code so I only glanced at
that; focusing on the prefs usage instead.

Lastly, I'm away starting tomorrow night and trying to wrap things up before I
leave so it's probably best if you can find yet another reviewer... sorry...
(perhaps mnissler can help provide guidance for prefs, but this is far from
being a prefs only CL; unless you manage to break it down, it will be hard to
find somebody to understands it all IMO..).

Cheers,
Gab

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/renderer...
File chrome/browser/renderer_preferences_util.cc (left):

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/renderer...
chrome/browser/renderer_preferences_util.cc:37: prefs->default_zoom_level =
pref_service->GetDouble(prefs::kDefaultZoomLevel);
It was kind of nice to have this in a single place; now it's initialized all
over the place -- is there a way to have a single cross-platform place to
initialize prefs->default_zoom_level now?

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:341: // Not all tests will create
zoom_level_prefs.
Why is this talking about tests in non-test code?

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:34:
zoom_subscription_.reset();
Why explicitly reset it? Doesn't letting it go out of scope naturally do the
right thing implicitly?

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:41: DCHECK((partition_path ==
profile_path_) ||
Remove unnecessary surrounding ()

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:44: host_zoom_map_ =
host_zoom_map;
DCHECK(!host_zoom_map_);
? (to make sure Init is only called once)

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:64:
pref_service_->GetDictionary(prefs::kPerHostZoomLevels);
I'm confused, so you migrate things out of kProfilePerHostZoomLevels into
another pref (kPerHostZoomLevels) in the same PrefService??

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:128: if
(!default_zoom_level_dictionary ||
PrefService::GetDictionary() never returns NULL.

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:33: // interface.
Mention that |pref_service| must outlive this class.

https://codereview.chromium.org/541103002/diff/240001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/541103002/diff/240001/chrome/common/pref_name...
chrome/common/pref_names.cc:1029: const char kProfileDefaultZoomLevel[] =
"profile.default_zoom_level";
s/kProfileDefaultZoomLevel/kDefaultZoomLevelInProfile

? Sounds better to me. (here and below).

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:761: host_zoom_map);
nit: indent

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:770: if (per_profile_default_zoom_level
!= default_default_zoom_level) {
Use prefs_->GetUserPrefValue() instead which will return NULL unless a user
value is explicitly set (this is better because it also accounts for the case
where the user explicitly chose to have the current default default as his
default).

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:774: default_default_zoom_level);
prefs_->ClearPref(prefs::kProfileDefaultZoomLevel);

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:781: // Careful: The returned value
could be NULL if the pref has never been set.
Incorrect, PrefService will return the default value if no user-value is set,
which for dictionaries is typically an empty dictionary [1]

(not to be confused with DictionaryValue::GetDictionary() which may return NULL
for absent inner-dicts).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_re...

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:786:
UserMetricsAction("Chrome_ZoomLevelPreferencesMigration"));
Typically I use BooleanHit histograms [1] rather than actions for this. One
advantage could be that they provide nicer dashboards (Usage dashboard,
timeline, etc.).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...

Alexei will know which one is best.

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:789: for
(base::DictionaryValue::Iterator i(*host_zoom_dictionary); !i.IsAtEnd();
s/i/it
is more common/better for iterators IMO

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:820:
host_zoom_dictionary_update->Clear();
The above 2 lines can be replaced by

update->Clear();

(in which case it may make sense to rename |update| to be more descriptive)

[wjmaclean, 2014-10-03 18:53:23]

gab@ - I don't know if you'll have time to take another look, but I've addressed
all your comments.

asvitkine@ - PTAL

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/renderer...
File chrome/browser/renderer_preferences_util.cc (left):

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/renderer...
chrome/browser/renderer_preferences_util.cc:37: prefs->default_zoom_level =
pref_service->GetDouble(prefs::kDefaultZoomLevel);
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> It was kind of nice to have this in a single place; now it's initialized all
> over the place -- is there a way to have a single cross-platform place to
> initialize prefs->default_zoom_level now?

This was originally done since not all callsites had a WebContents, but that has
changed so this value now onlys gets set in this function.

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:341: // Not all tests will create
zoom_level_prefs.
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> Why is this talking about tests in non-test code?

It's an explanation of why we cannot assume that there will always be a
|zoom_level_prefs_| available. We routinely put comments about testing in
non-test code (e.g. functions made protected for testing, etc.), so I didn't
think this was inappropriate.

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:34:
zoom_subscription_.reset();
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> Why explicitly reset it? Doesn't letting it go out of scope naturally do the
> right thing implicitly?

You're right ... there may once have been a reason for this, but it's certainly
gone now.

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:41: DCHECK((partition_path ==
profile_path_) ||
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> Remove unnecessary surrounding ()

Done.

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:44: host_zoom_map_ =
host_zoom_map;
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> DCHECK(!host_zoom_map_);
> ? (to make sure Init is only called once)

Done.

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:64:
pref_service_->GetDictionary(prefs::kPerHostZoomLevels);
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> I'm confused, so you migrate things out of kProfilePerHostZoomLevels into
> another pref (kPerHostZoomLevels) in the same PrefService??

Yes, although it's a little more than that.

Prior to this CL, there was (for example), only a single default zoom level
stored in prefs. Now every storage partition will have one (outside the scope of
this CL), and all the default zoom levels are stored in a Dictionary in the main
prefs (I would rather have had a separate PrefService for the individual
StoragePartitions, but bauerb@ felt that would incur too much additional
overhead).

Similarly, prior to this CL we had a single Dictionary storing per-host zoom
levels for everyone. After, we will have the capability to store separate
per-host zoom levels for each StoragePartition (in a follow-on CL we migrate
ownership of this class out of ProfileImpl and into StoragePartition).

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:128: if
(!default_zoom_level_dictionary ||
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> PrefService::GetDictionary() never returns NULL.

Done.

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:33: // interface.
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> Mention that |pref_service| must outlive this class.

Done.

https://codereview.chromium.org/541103002/diff/240001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/541103002/diff/240001/chrome/common/pref_name...
chrome/common/pref_names.cc:1029: const char kProfileDefaultZoomLevel[] =
"profile.default_zoom_level";
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> s/kProfileDefaultZoomLevel/kDefaultZoomLevelInProfile
> 
> ? Sounds better to me. (here and below).

Done.

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:761: host_zoom_map);
On 2014/10/02 20:58:42, gab (OOO Oct. 4 - Oct. 28) wrote:
> nit: indent

Done.

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:770: if (per_profile_default_zoom_level
!= default_default_zoom_level) {
On 2014/10/02 20:58:43, gab (OOO Oct. 4 - Oct. 28) wrote:
> Use prefs_->GetUserPrefValue() instead which will return NULL unless a user
> value is explicitly set (this is better because it also accounts for the case
> where the user explicitly chose to have the current default default as his
> default).

Thanks, that makes this code a lot better.

Done.

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:774: default_default_zoom_level);
On 2014/10/02 20:58:43, gab (OOO Oct. 4 - Oct. 28) wrote:
> prefs_->ClearPref(prefs::kProfileDefaultZoomLevel);

Done.

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:781: // Careful: The returned value
could be NULL if the pref has never been set.
On 2014/10/02 20:58:43, gab (OOO Oct. 4 - Oct. 28) wrote:
> Incorrect, PrefService will return the default value if no user-value is set,
> which for dictionaries is typically an empty dictionary [1]
> 
> (not to be confused with DictionaryValue::GetDictionary() which may return
NULL
> for absent inner-dicts).
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_re...

This was actually in the legacy version of the code, but good to know as we can
correct it here.

Done.

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:786:
UserMetricsAction("Chrome_ZoomLevelPreferencesMigration"));
On 2014/10/02 20:58:43, gab (OOO Oct. 4 - Oct. 28) wrote:
> Typically I use BooleanHit histograms [1] rather than actions for this. One
> advantage could be that they provide nicer dashboards (Usage dashboard,
> timeline, etc.).
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
> 
> Alexei will know which one is best.

Done.

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:789: for
(base::DictionaryValue::Iterator i(*host_zoom_dictionary); !i.IsAtEnd();
On 2014/10/02 20:58:43, gab (OOO Oct. 4 - Oct. 28) wrote:
> s/i/it
> is more common/better for iterators IMO

Done.

https://codereview.chromium.org/541103002/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:820:
host_zoom_dictionary_update->Clear();
On 2014/10/02 20:58:43, gab (OOO Oct. 4 - Oct. 28) wrote:
> The above 2 lines can be replaced by
> 
> update->Clear();
> 
> (in which case it may make sense to rename |update| to be more descriptive)

Done.

https://codereview.chromium.org/541103002/diff/280001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:790:
UserMetricsAction("Chrome_ZoomLevelPreferencesMigration"));
On 2014/10/02 20:40:20, Alexei Svitkine wrote:
> I'm not convinced a user action is the best way to measure this.
> 
> For one, you don't have a baseline (just an absolute number - but you can't
> answer questions such as what percentage of starts have migration).
> 
> Instead, I suggest using an enumerated histograms and recording both migrated
> and non-migrated cases. That way, in addition to the total volume of these,
you
> can see how frequently it happens (i.e. vs. the non-migrated case).
> 
> Plus, histograms are better supported by our dashboards, so you can filter
them
> by different criteria, for example.

Done.

https://codereview.chromium.org/541103002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:790:
UserMetricsAction("Chrome_ZoomLevelPreferencesMigration"));
On 2014/10/02 20:40:20, Alexei Svitkine wrote:
> I'm not convinced a user action is the best way to measure this.
> 
> For one, you don't have a baseline (just an absolute number - but you can't
> answer questions such as what percentage of starts have migration).
> 
> Instead, I suggest using an enumerated histograms and recording both migrated
> and non-migrated cases. That way, in addition to the total volume of these,
you
> can see how frequently it happens (i.e. vs. the non-migrated case).
> 
> Plus, histograms are better supported by our dashboards, so you can filter
them
> by different criteria, for example.

Done.

https://codereview.chromium.org/541103002/diff/280001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/280001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:57: LOG(WARNING) << "wjm: Hash
= " << partition_key_;
On 2014/10/02 20:40:20, Alexei Svitkine wrote:
> You probably want to remove this.

He he ... you inadvertently got an experimental version of the CL where I was
trying to sort out differences between Windows and non-Windows hash functions
(to make the tests work). Sorry about that! It will be gone before this lands.

[Alexei Svitkine (slow), 2014-10-03 18:58:06]

https://codereview.chromium.org/541103002/diff/340001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/541103002/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35720: +<histogram
name="UMA.ZoomLevelPreferencesMigrated" enum="BooleanSuccess">
Use enum="BooleanMigrated".

Also, the "UMA." prefix is reserved for metrics related to the UMA subsystem
itself.

Can you find a better prefix? Maybe "Settings."?

https://codereview.chromium.org/541103002/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35722: +  <summary>Tracks migration to
per-partition zoom-level preferences.</summary>
Please mention when this is logged. (On profile login? On startup? When settings
is opened?)

[wjmaclean, 2014-10-03 19:16:08]

wjmaclean@chromium.org changed reviewers:
+ battre@chromium.org

[wjmaclean, 2014-10-03 19:16:09]

asvitkine@ - PTAL

battre@ - All your colleagues are going on vacation! Would you be able to take
over the prefs/ aspects of this? I'd be happy to do a quick VC to bring you up
to speed on it.

https://codereview.chromium.org/541103002/diff/340001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/541103002/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35720: +<histogram
name="UMA.ZoomLevelPreferencesMigrated" enum="BooleanSuccess">
On 2014/10/03 18:58:05, Alexei Svitkine wrote:
> Use enum="BooleanMigrated".
> 
> Also, the "UMA." prefix is reserved for metrics related to the UMA subsystem
> itself.
> 
> Can you find a better prefix? Maybe "Settings."?

Done.

https://codereview.chromium.org/541103002/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35722: +  <summary>Tracks migration to
per-partition zoom-level preferences.</summary>
On 2014/10/03 18:58:05, Alexei Svitkine wrote:
> Please mention when this is logged. (On profile login? On startup? When
settings
> is opened?)

Done.

[Alexei Svitkine (slow), 2014-10-03 19:34:15]

histogram lgtm % comment below - I'll leave the rest of the review to others

https://codereview.chromium.org/541103002/diff/360001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/541103002/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29896: +<histogram
name="Settings.ZoomLevelPreferencesMigrated" enum="BooleanSuccess">
Change enum="BooleanSuccess" to enum="BooleanMigrated"

[wjmaclean, 2014-10-03 20:49:57]

https://codereview.chromium.org/541103002/diff/340001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/541103002/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35720: +<histogram
name="UMA.ZoomLevelPreferencesMigrated" enum="BooleanSuccess">
On 2014/10/03 18:58:05, Alexei Svitkine wrote:
> Use enum="BooleanMigrated".
> 
> Also, the "UMA." prefix is reserved for metrics related to the UMA subsystem
> itself.
> 
> Can you find a better prefix? Maybe "Settings."?

Done.

Sorry, I missed that part of the original comment. Have made the change; it will
be in the next patch I upload.

https://codereview.chromium.org/541103002/diff/360001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/541103002/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29896: +<histogram
name="Settings.ZoomLevelPreferencesMigrated" enum="BooleanSuccess">
On 2014/10/03 19:34:15, Alexei Svitkine wrote:
> Change enum="BooleanSuccess" to enum="BooleanMigrated"

Done.

[battre, 2014-10-06 14:33:51]

I did not get all the way through, yet. Only started on the HostZoomLevelPrefs.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/profiles...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:154: prefs::kDefaultZoomLevel,
This looks like pretty perilous: You are changing the type of kDefualtZoomLevel.
Please don't do that.

I suggest to rename kDefaultZoomLevel to kDefaultZoomLevelDeprecated, introduce
a *new* kPartitionDefaultZoomLevel, and then add a TODO to rename
kPartitionDefaultZoomLevel to kDefaultZoomLevels later.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:70: const
base::DictionaryValue* host_zoom_dictionary;
= 0;

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:81: DCHECK(success);
Preference files might be modified manually. I would not DCHECK on this.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:100: &host_zoom_dictionary)) {
Is this branch at all possible? Doesn't line 72/72 prevent it?

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:103: }
I don't see a purpose for storing empty dictionaries. I think they get purged on
disk anyway.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:107: ++it) {
nit: you can use C++11 syntax here

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:117: if
(host_zoom_map_->GetDefaultZoomLevel() == level)
Don't compare zoom levels with ==. Use content::ZoomValuesEqual.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:137:
SetDefaultZoomLevelPref(default_zoom_level);
Why do you need this? I don't think that a getter should persist anything. Also
default values should not be persisted so that they can be changed later.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:158: base::DictionaryValue*
host_zoom_dictionary;
= NULL;

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:162:
host_zoom_dictionaries->Set(partition_key_, host_zoom_dictionary);
Why?

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:24: // from HostZoomMap.
Can you work on the comment to better express what this class is in charge of?
Where authoritative data is stored and where it may be changed (i.e. what is the
flow of information)?

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:45: void
SetDefaultZoomLevelPref(double level);
Currently both ChromeZoomLevelPrefs and HostZoomMap have setters. Can we have a
single API for this? I think it should be HostZoomMap.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:46: double
GetDefaultZoomLevelPref();
const

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:51: void
OnZoomLevelChanged(const content::HostZoomMap::ZoomLevelChange& change);
Please add a comment, who calls this and what it does.

https://codereview.chromium.org/541103002/diff/380001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/common/pref_name...
chrome/common/pref_names.cc:1032: // TODO(wjmaclean): Remove this.
Please add a bug number and explain why to remove this.

[wjmaclean, 2014-10-06 16:58:13]

battre@ - I've updated to address your initial comments.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/profiles...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:154: prefs::kDefaultZoomLevel,
On 2014/10/06 14:33:51, battre wrote:
> This looks like pretty perilous: You are changing the type of
kDefualtZoomLevel.
> Please don't do that.
> 
> I suggest to rename kDefaultZoomLevel to kDefaultZoomLevelDeprecated,
introduce
> a *new* kPartitionDefaultZoomLevel, and then add a TODO to rename
> kPartitionDefaultZoomLevel to kDefaultZoomLevels later.

Done.

Might it be useful to just leave it as kPartitionDefaultZoomLevel, to remind
people that it's per-partition?

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:70: const
base::DictionaryValue* host_zoom_dictionary;
On 2014/10/06 14:33:51, battre wrote:
> = 0;

Done.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:81: DCHECK(success);
On 2014/10/06 14:33:51, battre wrote:
> Preference files might be modified manually. I would not DCHECK on this.

This was part of the legacy initialization code, though I see your point.
Updated.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:100: &host_zoom_dictionary)) {
On 2014/10/06 14:33:51, battre wrote:
> Is this branch at all possible? Doesn't line 72/72 prevent it?

Done.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:103: }
On 2014/10/06 14:33:51, battre wrote:
> I don't see a purpose for storing empty dictionaries. I think they get purged
on
> disk anyway.

My experience from looking at prefs files is that empty dictionaries do get
stored.

I guess if we should only ever be removing here, then if we have a missing
sub-dictionary then something's wrong.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:107: ++it) {
On 2014/10/06 14:33:51, battre wrote:
> nit: you can use C++11 syntax here

Done.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:117: if
(host_zoom_map_->GetDefaultZoomLevel() == level)
On 2014/10/06 14:33:51, battre wrote:
> Don't compare zoom levels with ==. Use content::ZoomValuesEqual.

Done.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:137:
SetDefaultZoomLevelPref(default_zoom_level);
On 2014/10/06 14:33:51, battre wrote:
> Why do you need this? I don't think that a getter should persist anything.
Also
> default values should not be persisted so that they can be changed later.

I agree the getter should be const, but wasn't sure where else to put this. I've
removede it (see end of this comment).

I had though the default values would need to be persisted since the
per-partition default zoom levels cannot be registered (due to the prefix
property, see bauerb@'s review comments), which is (in my possibly limited
knowledge) the only other way to set a default value.

If it's an acceptable alternative, we can use the value on line 129 to act as
the (non-persisted) default value.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:158: base::DictionaryValue*
host_zoom_dictionary;
On 2014/10/06 14:33:51, battre wrote:
> = NULL;

Done.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:162:
host_zoom_dictionaries->Set(partition_key_, host_zoom_dictionary);
On 2014/10/06 14:33:51, battre wrote:
> Why?

Won't we hit an error if we try to add an entry to a non-existent
sub-dictionary? The value of host_zoom_dictionary can be NULL (since the
sub-dictionaries aren't registered, and this sub-dictionary might never have
been accessed before).

I've modified the code to only create the sub-dictionary if we are doing an
insertion, as for a removal it should already exist.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:24: // from HostZoomMap.
On 2014/10/06 14:33:51, battre wrote:
> Can you work on the comment to better express what this class is in charge of?
> Where authoritative data is stored and where it may be changed (i.e. what is
the
> flow of information)?

Done.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:45: void
SetDefaultZoomLevelPref(double level);
On 2014/10/06 14:33:51, battre wrote:
> Currently both ChromeZoomLevelPrefs and HostZoomMap have setters. Can we have
a
> single API for this? I think it should be HostZoomMap.

It is my understanding that the preferences are treated as a chrome content,
whereas HostZoomMap is purely a content/ construct. In content/ we use
HostZoomMap, but there needs to be a place in chrome to update the change in
preference. It is my intention that this will we that place, and all code in
chrome/ that wants to change default zoom level gors through here, and does not
talk directly to HostZoomMap.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:46: double
GetDefaultZoomLevelPref();
On 2014/10/06 14:33:51, battre wrote:
> const

Done.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:51: void
OnZoomLevelChanged(const content::HostZoomMap::ZoomLevelChange& change);
On 2014/10/06 14:33:51, battre wrote:
> Please add a comment, who calls this and what it does.

Done.

https://codereview.chromium.org/541103002/diff/380001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/common/pref_name...
chrome/common/pref_names.cc:1032: // TODO(wjmaclean): Remove this.
On 2014/10/06 14:33:51, battre wrote:
> Please add a bug number and explain why to remove this.

Done.

[wjmaclean, 2014-10-06 19:24:07]

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:147: bool
modification_is_removal =
I'd like to use this value as a key to when to expect that the sub-dictionary
already exists, but initial testing suggests that (in tests at least) this may
be challenging. I'm still thinking about how this might work though.

[battre, 2014-10-08 11:27:11]

I have only a single major concern left, whether this breaks for OTR profiles
such as the guest mode on windows because no ChromeZoomLevelPrefs exists.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/profiles...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:154: prefs::kDefaultZoomLevel,
On 2014/10/06 16:58:12, wjmaclean wrote:
> On 2014/10/06 14:33:51, battre wrote:
> > This looks like pretty perilous: You are changing the type of
> kDefualtZoomLevel.
> > Please don't do that.
> > 
> > I suggest to rename kDefaultZoomLevel to kDefaultZoomLevelDeprecated,
> introduce
> > a *new* kPartitionDefaultZoomLevel, and then add a TODO to rename
> > kPartitionDefaultZoomLevel to kDefaultZoomLevels later.
> 
> Done.
> 
> Might it be useful to just leave it as kPartitionDefaultZoomLevel, to remind
> people that it's per-partition?

Acknowledged.

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/380001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:162:
host_zoom_dictionaries->Set(partition_key_, host_zoom_dictionary);
On 2014/10/06 16:58:12, wjmaclean wrote:
> On 2014/10/06 14:33:51, battre wrote:
> > Why?
> 
> Won't we hit an error if we try to add an entry to a non-existent
> sub-dictionary? The value of host_zoom_dictionary can be NULL (since the
> sub-dictionaries aren't registered, and this sub-dictionary might never have
> been accessed before).
> 
> I've modified the code to only create the sub-dictionary if we are doing an
> insertion, as for a removal it should already exist.

Acknowledged. - I misunderstood the code.

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:751: void ProfileImpl::InitHostZoomMap()
{
What do you think of moving the migration code to browser_prefs.cc ->
MigrateUserPrefs? This is where we typically trigger it.

I would then move the cleanup code out of Profile and into to the new
ChromeZoomLevelPrefs.

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:774: double
per_profile_default_zoom_level;
nit: = 0.0;

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:331: // Not all tests will create
zoom_level_prefs.
Also no incognito tabs. Are you sure this is ok?

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:802:
DCHECK(zoom_level_prefs);
Can you please check whether this might fail for the new Windows Guest mode?
IIRC it offers a chrome://settings page but is based on incognito mode.

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:100: for (std::string s :
keys_to_remove)
for (const std::string& s : keys_to_remove)

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:55: // zoom levels (if any)
managed by this class (for its associated  partition).
nit: 2 spaces

[Dan Beam, 2014-10-09 01:10:31]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[wjmaclean, 2014-10-10 14:35:28]

Patchset #21 (id:420001) has been deleted

[wjmaclean, 2014-10-10 14:40:29]

battre@ - PTAL?

I think I've addressed your concerns about the guest mode browsing (there was a
minor issue there, but I think I've resolved it).

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:751: void ProfileImpl::InitHostZoomMap()
{
On 2014/10/08 11:27:10, battre wrote:
> What do you think of moving the migration code to browser_prefs.cc ->
> MigrateUserPrefs? This is where we typically trigger it.
> 
> I would then move the cleanup code out of Profile and into to the new
> ChromeZoomLevelPrefs.

Done.

I didn't realize there was similar code elsewhere I could group this with ...
thanks for mentioning it.

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:774: double
per_profile_default_zoom_level;
On 2014/10/08 11:27:10, battre wrote:
> nit: = 0.0;

Done.

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:331: // Not all tests will create
zoom_level_prefs.
On 2014/10/08 11:27:10, battre wrote:
> Also no incognito tabs. Are you sure this is ok?

I think so. Certainly the zoom levels in incognito are explicitly set to not
update the existing per-host prefs, and you can't edit any zoom settings in an
incognito window.

In this file, if the zoom level prefs don't exist, then all that happens is no
callback is registered on pref changes, but that's ok for everything except
default-zoom level. But in incognito mode changes to default zoom level
propagate to render views via a different mechanism, so this CL seems to
preserve that behaviour too. In guest mode, the changes don't propagate anyways.

I have updated the comment above though to indicate that incognito windows won't
have one either.

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:802:
DCHECK(zoom_level_prefs);
On 2014/10/08 11:27:10, battre wrote:
> Can you please check whether this might fail for the new Windows Guest mode?
> IIRC it offers a chrome://settings page but is based on incognito mode.

I had to make minor modifications here so the DCHECK wouldn't fire in Guest
mode, but it is OK for Guest mode to not have a zoom level prefs.

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:100: for (std::string s :
keys_to_remove)
On 2014/10/08 11:27:10, battre wrote:
> for (const std::string& s : keys_to_remove)

Done.

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/400001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:55: // zoom levels (if any)
managed by this class (for its associated  partition).
On 2014/10/08 11:27:10, battre wrote:
> nit: 2 spaces

Done.

[battre, 2014-10-10 15:41:09]

LGTM

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:657: chrome::ChromeZoomLevelPrefs*
zoom_level_prefs = profile->GetZoomLevelPrefs();
DCHECK(zoom_level_prefs); ?

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:666: base::Value::TYPE_DOUBLE);
I would just clear the value in case this is not TYPE_DOUBLE. No need to brick
the user's Chrome installation.

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:759:
chrome::MigrateProfileZoomLevelPrefs(this);
How about moving this call to browser_prefs.cc? This is where the other Migrate
calls live.

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:73:
ExtractPerHostZoomLevels(host_zoom_dictionary);
Here is a time dependency that should be documented in order to prevent
concurrent modifications while iterating over a dictionary:

ExtractPerHostZoomLevels takes the values from the dictionary and updates the
host_zoom_map_. If the subscription of line 75 were to be placed before line 71,
we would iterate over a dictionary that is indirectly modified by the
OnZoomLevelChanged handler.

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:158:
host_zoom_map_->SetZoomLevelForHost(host, zoom_level);
Note that this may modify the host_zoom_dictionary. Maybe we should create a
copy of host_zoom_dictionary before iterating over it.

[wjmaclean, 2014-10-10 15:55:15]

On 2014/10/10 15:41:09, battre wrote:
> LGTM

Thanks battre@! I'll get these last items cleaned up shortly and then track down
OWNERS for the rest.

[wjmaclean, 2014-10-10 17:14:10]

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:657: chrome::ChromeZoomLevelPrefs*
zoom_level_prefs = profile->GetZoomLevelPrefs();
On 2014/10/10 15:41:08, battre wrote:
> DCHECK(zoom_level_prefs); ?

Done.

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:666: base::Value::TYPE_DOUBLE);
On 2014/10/10 15:41:08, battre wrote:
> I would just clear the value in case this is not TYPE_DOUBLE. No need to brick
> the user's Chrome installation.

Done.

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:759:
chrome::MigrateProfileZoomLevelPrefs(this);
On 2014/10/10 15:41:08, battre wrote:
> How about moving this call to browser_prefs.cc? This is where the other
Migrate
> calls live.

I'm (more than) a bit nervous about that, since calling this from either
MigrateUserPrefs() or MigrateBrowserPrefs() means changing the call to
InitHostZoomMap() to be *much earlier* than it is at present. When I land the
final CLs to put HostZoomMap into StoragePartition I can revisit this idea, but
for this CL I'd like to leave it where it is.

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:73:
ExtractPerHostZoomLevels(host_zoom_dictionary);
On 2014/10/10 15:41:08, battre wrote:
> Here is a time dependency that should be documented in order to prevent
> concurrent modifications while iterating over a dictionary:
> 
> ExtractPerHostZoomLevels takes the values from the dictionary and updates the
> host_zoom_map_. If the subscription of line 75 were to be placed before line
71,
> we would iterate over a dictionary that is indirectly modified by the
> OnZoomLevelChanged handler.

I've added a comment here, but the important change is iterating over a copy in
the extract function (see comment for line 158)

https://codereview.chromium.org/541103002/diff/620001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:158:
host_zoom_map_->SetZoomLevelForHost(host, zoom_level);
On 2014/10/10 15:41:08, battre wrote:
> Note that this may modify the host_zoom_dictionary. Maybe we should create a
> copy of host_zoom_dictionary before iterating over it.

I think making a copy is the safest way to avoid the time dependency, since we
have no guarantee that this function isn't called after the HostZoomMap
subscription is setup on line 75.

Done.

[wjmaclean, 2014-10-10 18:21:13]

wjmaclean@chromium.org changed reviewers:
+ erg@chromium.org

[wjmaclean, 2014-10-10 18:21:14]

erg@ - could you please look at the chrome/browser/profiles/* files? The prefs
reviewers have already given them a good look over, but they still need OWNERS
approval.

[Elliot Glaysher, 2014-10-10 19:05:11]

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:980: chrome::ChromeZoomLevelPrefs*
ProfileImpl::GetZoomLevelPrefs() {
So, on what sort of timescale do you plan to remove this, and the other changes
to profile? I'm fine with these changes if the end result is to get all the zoom
stuff out of Profile.

[wjmaclean, 2014-10-10 19:18:40]

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:980: chrome::ChromeZoomLevelPrefs*
ProfileImpl::GetZoomLevelPrefs() {
On 2014/10/10 19:05:11, Elliot Glaysher wrote:
> So, on what sort of timescale do you plan to remove this, and the other
changes
> to profile? I'm fine with these changes if the end result is to get all the
zoom
> stuff out of Profile.

The timescale is intended to be "as quickly as possible", basically measured by
when we feel a sufficient number of users have (likely) migrated to the new
prefs structure, so we don't discard their prefs. There are a couple of CLs to
land after this one (to get HostZoomMap into StoragePartition) but they won't
extend the timeline for migrating prefs.

[sky, 2014-10-10 19:43:57]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2014-10-10 19:43:59]

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:655: void
MigrateProfileZoomLevelPrefs(Profile* profile) {
Is there test coverage of this?

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:669:
per_profile_default_zoom_level_value->GetAsDouble(
Add a temp here and DCHECK returns true.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:139: std::vector<std::string>
keys_to_remove;
Is there a particular reason you build up a vector and then apply? More
specifically, why not remove as you're going?

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: style guide says don't use the (c) for new files.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:27: // levels in HostZoomMap
and Chrome's preference persistence system. All changes
nit: preferences imply persistence. I don't think you need to say persistence
here.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:31: class ChromeZoomLevelPrefs
{
Is there a reason you don't have a unit test for this file? Is it covered by
other tests?

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:37: static std::string
GetHash(const base::FilePath& relative_path);
nit: constructor/destructor before any other functions.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:37: static std::string
GetHash(const base::FilePath& relative_path);
GetHasForTesting.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:66: base::FilePath
profile_path_;
nit: const.

[wjmaclean, 2014-10-10 20:23:02]

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:655: void
MigrateProfileZoomLevelPrefs(Profile* profile) {
On 2014/10/10 19:43:59, sky wrote:
> Is there test coverage of this?

Yes, a browser_test:

HostZoomMapMigrationBrowserTest.MigrateProfileZoomPreferences

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:669:
per_profile_default_zoom_level_value->GetAsDouble(
On 2014/10/10 19:43:59, sky wrote:
> Add a temp here and DCHECK returns true.

Done.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc (right):

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:139: std::vector<std::string>
keys_to_remove;
On 2014/10/10 19:43:59, sky wrote:
> Is there a particular reason you build up a vector and then apply? More
> specifically, why not remove as you're going?

In the original code (some of this existed in profile_impl.cc previously) the
intent would have been to avoid modifying a Dictionary while iterating over it
(I'm guessing, not sure - the previous code didn't make a copy of the
dictionary). I kept it because I thought it was cleaner this way, but I'm not
wed to that and can change it if you prefer.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/chrome_zoom_level_prefs.h (right):

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/10/10 19:43:59, sky wrote:
> nit: style guide says don't use the (c) for new files.

Done.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:27: // levels in HostZoomMap
and Chrome's preference persistence system. All changes
On 2014/10/10 19:43:59, sky wrote:
> nit: preferences imply persistence. I don't think you need to say persistence
> here.

Done.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:31: class ChromeZoomLevelPrefs
{
On 2014/10/10 19:43:59, sky wrote:
> Is there a reason you don't have a unit test for this file? Is it covered by
> other tests?

Yes, through HostZoomMap browser tests, and any other browser test that involves
zoom functionality. A specific HostZoomMap browser test is included to test the
migration aspect (and is the reason for the GetHashForTesting() function.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:37: static std::string
GetHash(const base::FilePath& relative_path);
On 2014/10/10 19:43:59, sky wrote:
> nit: constructor/destructor before any other functions.

Done.

Sorry, I thought static functions went first, but I should have checked.

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:37: static std::string
GetHash(const base::FilePath& relative_path);
On 2014/10/10 19:43:59, sky wrote:
> GetHashForTesting.

Done. But it's also used internally in the class, is that OK? It looks a little
weird there. I suppose we could add an internal function GetHash() and an
external accessor named GetHashForTesting, if that's not overkill?

https://codereview.chromium.org/541103002/diff/850001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:66: base::FilePath
profile_path_;
On 2014/10/10 19:43:59, sky wrote:
> nit: const.

Done.

[sky, 2014-10-13 16:42:50]

LGTM

[wjmaclean, 2014-10-14 16:13:57]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-10-14 16:15:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/541103002/900002

[commit-bot: I haz the power, 2014-10-14 17:01:37]

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

[commit-bot: I haz the power, 2014-10-14 17:01:39]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2014-10-14 17:09:01]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-10-14 17:09:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/541103002/1020001

[Elliot Glaysher, 2014-10-14 17:27:49]

lgtm

[commit-bot: I haz the power, 2014-10-14 18:49:17]

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

[commit-bot: I haz the power, 2014-10-14 18:49:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2014-10-14 18:57:32]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-10-14 18:58:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/541103002/1020001

[wjmaclean, 2014-10-14 19:33:22]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-10-14 19:33:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/541103002/1040001

[commit-bot: I haz the power, 2014-10-14 21:43:48]

Committed patchset #26 (id:1040001)

[commit-bot: I haz the power, 2014-10-14 21:51:39]

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