Migrate HostZoomMap to live in StoragePartition.

Migrate HostZoomMap to live in StoragePartition.

This CL changes the persistence of host zoom levels to be on
a per-storage-partition basis, as opposed to (the current)
per-profile basis. This is needed to allow WebView content
(withing apps) to keep their zoom levels independent of those
in the main browser window.

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

[wjmaclean, 2014-08-11 18:42:15]

ajwong@ Would you be willing to take the first look at this?

[awong, 2014-08-11 22:36:34]

I like where this is going, but am mostly confused by why there's still an
accessor for the zoom level exposed in BrowserContext.

If the data is in a StoragePartition, it's because we need to have different
behaviors based on that partitioning. Having an API exposed which gets to hot
this boundary sounds like a recipe for getting wires crossed...

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

https://codereview.chromium.org/393133002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:98:
base::RunLoop run_loop;
Is it possible to use something like:
 
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

?

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

https://codereview.chromium.org/393133002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile.cc:270: // partitions, e.g. those used by
WebViewGuests.
Can we explain why? This is confusing to me...

https://codereview.chromium.org/393133002/diff/80001/content/browser/browser_...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/80001/content/browser/browser_...
content/browser/browser_context.cc:297: double
BrowserContext::GetDefaultZoomLevel() const {
This concept is odd to me. If the zoom level is moved to per-storage-partition,
why are we defining one per BrowserContext?

https://codereview.chromium.org/393133002/diff/80001/content/browser/host_zoo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/80001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.cc:53:
BrowserContext::GetDefaultStoragePartition(context);
How does API behave in the face of Chrome Apps? Specifically <webview>?

https://codereview.chromium.org/393133002/diff/80001/content/browser/storage_...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/393133002/diff/80001/content/browser/storage_...
content/browser/storage_partition_impl_map.h:26: class HostZoomMap;
Is this needed?

https://codereview.chromium.org/393133002/diff/80001/content/public/browser/h...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/393133002/diff/80001/content/public/browser/h...
content/public/browser/host_zoom_map.h:60: CONTENT_EXPORT static HostZoomMap*
GetForSite(SiteInstance* instance);
What does this mean semantically? 

Not saying this is bad necessarily, but a SiteInstance is a way to approximate
the grouping of "sites" that need to have scripting relations to one another.
Having a HostZoomMap attached to that without an explanation of why the
SiteInstance is the key seems a little odd...

https://codereview.chromium.org/393133002/diff/80001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:205: }
newline?

[wjmaclean, 2014-08-12 16:57:44]

I've addressed your first round of comments below.

Here's a link to a design doc that I should have included in the initial review
... it may be more detailed than you want, but explains some of the reasons
behind my decisions.

https://docs.google.com/a/chromium.org/document/d/1UisZw9O2qEUZyVWy-UTFmhoOsF...

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

https://codereview.chromium.org/393133002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:98:
base::RunLoop run_loop;
On 2014/08/11 22:36:33, awong wrote:
> Is it possible to use something like:
>  
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
> 
> ?

Actually, don't even need that. This is cruft from the attempts to fix this
test, and I just forgot to remove it.

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

https://codereview.chromium.org/393133002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile.cc:270: // partitions, e.g. those used by
WebViewGuests.
On 2014/08/11 22:36:33, awong wrote:
> Can we explain why? This is confusing to me...

The incognito profile, when created, basically tracks the zoom levels of its
parent profile (but any changes in the OTR profile's window do not propagate
back).

At present I'm assuming everything *except* apps (and thus WebViews) is using
the main profile's default storage partition, but perhaps that assumption is
wrong?

https://codereview.chromium.org/393133002/diff/80001/content/browser/browser_...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/80001/content/browser/browser_...
content/browser/browser_context.cc:297: double
BrowserContext::GetDefaultZoomLevel() const {
On 2014/08/11 22:36:33, awong wrote:
> This concept is odd to me. If the zoom level is moved to
per-storage-partition,
> why are we defining one per BrowserContext?

So far I've made the decision to keep the notion of the default zoom level as
per-profile. Only host-specific zoom-level storage has moved to
StoragePartitions.

I'm certainly open to the idea of persisting default-zoom levels
per-StoragePartition, but there is one issue to consider then: at present we
allow the user to edit the default zoom level in the settings page. How will
this work when there are multiple (many?) StoragePartitions? Certainly it's ok
to assume the user will never edit this for apps (which don't support zoom), but
for cross-site isolation purposes it may be unreasonable to make the user set
default levels for more than one StoragePartition.

https://codereview.chromium.org/393133002/diff/80001/content/browser/host_zoo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/80001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.cc:53:
BrowserContext::GetDefaultStoragePartition(context);
On 2014/08/11 22:36:33, awong wrote:
> How does API behave in the face of Chrome Apps? Specifically <webview>?

Chrome apps/WebView access their HostZoomMap via a per-WebContents
ZoomController. Access via this function is typically done by UI elements, like
the wrench menu, that don't concern themselves with the zoom levels of apps.

https://codereview.chromium.org/393133002/diff/80001/content/browser/storage_...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/393133002/diff/80001/content/browser/storage_...
content/browser/storage_partition_impl_map.h:26: class HostZoomMap;
On 2014/08/11 22:36:33, awong wrote:
> Is this needed?

Ooops, no - cruft from revising. Removed.

https://codereview.chromium.org/393133002/diff/80001/content/public/browser/h...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/393133002/diff/80001/content/public/browser/h...
content/public/browser/host_zoom_map.h:60: CONTENT_EXPORT static HostZoomMap*
GetForSite(SiteInstance* instance);
On 2014/08/11 22:36:33, awong wrote:
> What does this mean semantically? 
> 
> Not saying this is bad necessarily, but a SiteInstance is a way to approximate
> the grouping of "sites" that need to have scripting relations to one another.
> Having a HostZoomMap attached to that without an explanation of why the
> SiteInstance is the key seems a little odd...

I'm assuming there's a one-to-one relationship between SiteInstances and
StoragePartition, hence a SiteInstance should identify a unique HostZoomMap.
Then the GetForWebContents() accessor is just a convenience.

The accessor via BrowserContext is for the default storage partition, which (at
least at present) is where we expect non-app/WebView pages to end up. UI
elements like wrench menu depend on this. *If* there are tabs displayed in the
main browser window that do not use the default storage partition, then some
issues may arise.

Is my assumption not correct?

Or is this more of a comment-documentation concern?

https://codereview.chromium.org/393133002/diff/80001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/80001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:205: }
On 2014/08/11 22:36:33, awong wrote:
> newline?

Done.

[awong, 2014-08-12 21:07:07]

Overall looks pretty solid.

Gave a few suggestions regarding removing refcounts. Otherwise, the main
suggestion is beefing up the comments. Currently, they're focused on "what"
rather than "why". My litmus test is assume someone pickups up StoragePartition
ownership 5 years from now. Will they be able to redo the evluation of whether
or not this API and ownership structure make sense? If not, then there's not
enough info.

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

https://codereview.chromium.org/393133002/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:392: void TrackZoomLevelsFromParent(Profile*
parent);
nit: Name this TrackZoomLevelsFromParentForTest if it's only for testing.

https://codereview.chromium.org/393133002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/393133002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:519: // and we're
only interested in non-temporary, host-only zoom levels.
Needs more "why" in the comment. :)

https://codereview.chromium.org/393133002/diff/100001/content/browser/browser...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/100001/content/browser/browser...
content/browser/browser_context.cc:91: DCHECK(partition);
IIRC, if you're just going to crash on the next line, we don't usually bother
with a DCHECK().

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:25: : public
base::RefCountedThreadSafe<HostZoomLevelContext> {
Can this be expressed as single-ownership? The rest of the stuff is
RefCountedThreadSafe for legacy reasons (before base::Bind(), there was pressure
to make things refcounted due to NewRunnableMethod quirks).

If possible, avoiding refcounts is nearly always the way to go.

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:66: // scheme+host zoom levels are
included. Caller assumes ownership of the
"Caller assumes ownership..." is redundant when scoped_ptr<> is in the return
type...there's no other possible meaning.

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:68: // TODO(wjmaclean) Should we return an
owned pointer here?
Unsure what this TODO means. Seems like you already do this?

https://codereview.chromium.org/393133002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/393133002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1117:
static_cast<HostZoomMapImpl*>(HostZoomMap::GetForSite(GetSiteInstance()));
GetForSite() is incorrectly named...This is a SiteInstance, not a Site...

A "Site" actually has specific meaning of the top-level-domain + 1 level and the
url scheme. So http://docs.google.com:123 http://www.google.com are the same
site but https://docs.google.com isn't. There's also a trick with
www.google.co.uk (and a few other multi-component top level domains) where tld +
1 == google.co.uk.

Maybe just leave it as HostZoomMap::Get(xxx)?

https://codereview.chromium.org/393133002/diff/100001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/393133002/diff/100001/content/public/browser/...
content/public/browser/browser_context.h:121: // zoom levels are not persisted.
The caller takes ownership of the delegate.
This comment doesn't give me enough info to understand why we'd want to call
this, or why one would override the default. Can we expand a bit?

https://codereview.chromium.org/393133002/diff/100001/content/public/browser/...
File content/public/browser/zoom_level_prefs_delegate.h (right):

https://codereview.chromium.org/393133002/diff/100001/content/public/browser/...
content/public/browser/zoom_level_prefs_delegate.h:20: class
ZoomLevelPrefsDelegate : public base::RefCounted<ZoomLevelPrefsDelegate> {
Does this need to be refcounted?

[wjmaclean, 2014-08-13 17:16:04]

+ fsamuel@ for look at storage partitions code.

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

https://codereview.chromium.org/393133002/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:392: void TrackZoomLevelsFromParent(Profile*
parent);
On 2014/08/12 21:07:06, awong wrote:
> nit: Name this TrackZoomLevelsFromParentForTest if it's only for testing.

This is also used to setup HostZoomMap tracking from the parent in non-test code
also.

https://codereview.chromium.org/393133002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/393133002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:519: // and we're
only interested in non-temporary, host-only zoom levels.
On 2014/08/12 21:07:06, awong wrote:
> Needs more "why" in the comment. :)

Done.

https://codereview.chromium.org/393133002/diff/100001/content/browser/browser...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/100001/content/browser/browser...
content/browser/browser_context.cc:91: DCHECK(partition);
On 2014/08/12 21:07:06, awong wrote:
> IIRC, if you're just going to crash on the next line, we don't usually bother
> with a DCHECK().

Done.

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:25: : public
base::RefCountedThreadSafe<HostZoomLevelContext> {
On 2014/08/12 21:07:06, awong wrote:
> Can this be expressed as single-ownership? The rest of the stuff is
> RefCountedThreadSafe for legacy reasons (before base::Bind(), there was
pressure
> to make things refcounted due to NewRunnableMethod quirks).
> 
> If possible, avoiding refcounts is nearly always the way to go.

Done.

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:66: // scheme+host zoom levels are
included. Caller assumes ownership of the
On 2014/08/12 21:07:06, awong wrote:
> "Caller assumes ownership..." is redundant when scoped_ptr<> is in the return
> type...there's no other possible meaning.

Done.

https://codereview.chromium.org/393133002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:68: // TODO(wjmaclean) Should we return an
owned pointer here?
On 2014/08/12 21:07:06, awong wrote:
> Unsure what this TODO means. Seems like you already do this?

Ha, probably a stale TODO() comment.

Done.

https://codereview.chromium.org/393133002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/393133002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1117:
static_cast<HostZoomMapImpl*>(HostZoomMap::GetForSite(GetSiteInstance()));
On 2014/08/12 21:07:06, awong wrote:
> GetForSite() is incorrectly named...This is a SiteInstance, not a Site...
> 
> A "Site" actually has specific meaning of the top-level-domain + 1 level and
the
> url scheme. So http://docs.google.com:123 http://www.google.com are the same
> site but https://docs.google.com isn't. There's also a trick with
> http://www.google.co.uk (and a few other multi-component top level domains)
where tld +
> 1 == google.co.uk.
> 
> Maybe just leave it as HostZoomMap::Get(xxx)?

I'm ok with this, though I suppose it does break the GetForX() pattern elsewhere
in HostZoomMap retrievers. An alternative could be GetForSiteInstance()?

I've changed it to plain-old "Get()".

Done.

https://codereview.chromium.org/393133002/diff/100001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/393133002/diff/100001/content/public/browser/...
content/public/browser/browser_context.h:121: // zoom levels are not persisted.
The caller takes ownership of the delegate.
On 2014/08/12 21:07:06, awong wrote:
> This comment doesn't give me enough info to understand why we'd want to call
> this, or why one would override the default. Can we expand a bit?

Done.

https://codereview.chromium.org/393133002/diff/100001/content/public/browser/...
File content/public/browser/zoom_level_prefs_delegate.h (right):

https://codereview.chromium.org/393133002/diff/100001/content/public/browser/...
content/public/browser/zoom_level_prefs_delegate.h:20: class
ZoomLevelPrefsDelegate : public base::RefCounted<ZoomLevelPrefsDelegate> {
On 2014/08/12 21:07:07, awong wrote:
> Does this need to be refcounted?

Hmmm, no.

Done.

[Fady Samuel, 2014-08-13 19:45:43]

Some initial thoughts in addition to my comments in your doc.

https://codereview.chromium.org/393133002/diff/80001/content/public/browser/h...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/393133002/diff/80001/content/public/browser/h...
content/public/browser/host_zoom_map.h:60: CONTENT_EXPORT static HostZoomMap*
GetForSite(SiteInstance* instance);
On 2014/08/12 16:57:44, wjmaclean wrote:
> On 2014/08/11 22:36:33, awong wrote:
> > What does this mean semantically? 
> > 
> > Not saying this is bad necessarily, but a SiteInstance is a way to
approximate
> > the grouping of "sites" that need to have scripting relations to one
another.
> > Having a HostZoomMap attached to that without an explanation of why the
> > SiteInstance is the key seems a little odd...
> 
> I'm assuming there's a one-to-one relationship between SiteInstances and
> StoragePartition, hence a SiteInstance should identify a unique HostZoomMap.
> Then the GetForWebContents() accessor is just a convenience.
> 
> The accessor via BrowserContext is for the default storage partition, which
(at
> least at present) is where we expect non-app/WebView pages to end up. UI
> elements like wrench menu depend on this. *If* there are tabs displayed in the
> main browser window that do not use the default storage partition, then some
> issues may arise.
> 
> Is my assumption not correct?
> 
> Or is this more of a comment-documentation concern?

The signin page currently uses a different storage partition. This just occurred
to me.

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

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:269: if (GetProfileType() !=
INCOGNITO_PROFILE ||
This seems like an assumption that may be violated in the future. Let's make
this a DCHECK instead?

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:282:
base::Bind(&Profile::TrackZoomLevelChanged, base::Unretained(this)));
I worry about the lifetime implications here. What if "this" is deleted prior to
deleting the parent. Is it possible we end up calling TrackZoomLevelChanged with
an invalid "this"? I would use a WeakPtrFactory and get rid of the unretained.

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_level_prefs_delegate_impl.h (right):

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_level_prefs_delegate_impl.h:19: class
ZoomLevelPrefsDelegateImpl : public PrefStore::Observer,
ZoomLevelPrefsStoreImpl

https://codereview.chromium.org/393133002/diff/120001/content/browser/browser...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/120001/content/browser/browser...
content/browser/browser_context.cc:115: void
BrowserContext::SetDefaultZoomLevel(BrowserContext* context,
I'm not sure we want to do this. This will affect the default zoom level for
<webview>s as well, which may be unexpected.

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:24: class HostZoomLevelContext {
Explain the purpose of this class here.

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:89:
HostZoomMapImpl::HostZoomMapImpl(double default_zoom_level)
Why do we take in a default zoom level here?

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:163: if (!ZoomValuesEqual(i->second,
default_zoom_level_)) {
if (ZoomValuesEqual(i->second, default_zoom_level_))
  continue;

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:67: scoped_ptr<base::DictionaryValue>
ZoomLevelDictionary() const;
GetZoomLevelDictionary

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
File content/public/browser/host_zoom_map.h (left):

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/host_zoom_map.h:55: CONTENT_EXPORT static HostZoomMap*
GetForBrowserContext(
GetDefaultForBrowserContext to be more explicit

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/host_zoom_map.h:60: CONTENT_EXPORT static HostZoomMap*
Get(SiteInstance* instance);
Comment on what this does.

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/host_zoom_map.h:62: CONTENT_EXPORT static HostZoomMap*
GetForWebContents(
Comment on what this does. Indicate that if multiple WebContents share the same
SiteInstance, then they'll share the same HostZoomMap.

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
File content/public/browser/zoom_level_prefs_delegate.h (right):

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/zoom_level_prefs_delegate.h:8: #include
"base/memory/ref_counted.h"
Why is this here?

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/zoom_level_prefs_delegate.h:20: class
ZoomLevelPrefsDelegate {
This name is odd as it doesn't expose any particular operations to delegate
except "Init" and it's unclear what Init does.

Perhaps this should be called ZoomLevelPrefsStore?

[wjmaclean, 2014-08-14 18:18:22]

I'm currently working on making DefaultZoomLevel be per-partition, and will
upload a patch for that (hopefully) this afternoon.

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

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:269: if (GetProfileType() !=
INCOGNITO_PROFILE ||
On 2014/08/13 19:45:42, Fady Samuel wrote:
> This seems like an assumption that may be violated in the future. Let's make
> this a DCHECK instead?

Done.

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:282:
base::Bind(&Profile::TrackZoomLevelChanged, base::Unretained(this)));
On 2014/08/13 19:45:42, Fady Samuel wrote:
> I worry about the lifetime implications here. What if "this" is deleted prior
to
> deleting the parent. Is it possible we end up calling TrackZoomLevelChanged
with
> an invalid "this"? I would use a WeakPtrFactory and get rid of the unretained.

The HostZoomMap lives in a StoragePartitionImpl inside a StoragePartitionImplMap
owned by 'this'. If 'this' is deleted, the HostZoomMap is guaranteed to be
destructed first.

This is essentially code removed from OffTheRecordProfileImpl and moved here.

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_level_prefs_delegate_impl.h (right):

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_level_prefs_delegate_impl.h:19: class
ZoomLevelPrefsDelegateImpl : public PrefStore::Observer,
On 2014/08/13 19:45:42, Fady Samuel wrote:
> ZoomLevelPrefsStoreImpl

Done.

https://codereview.chromium.org/393133002/diff/120001/content/browser/browser...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/120001/content/browser/browser...
content/browser/browser_context.cc:115: void
BrowserContext::SetDefaultZoomLevel(BrowserContext* context,
On 2014/08/13 19:45:42, Fady Samuel wrote:
> I'm not sure we want to do this. This will affect the default zoom level for
> <webview>s as well, which may be unexpected.

Hmm, yes, I forgot about that.

I've been leaning towards converting DefaultZoomLevel to be in the
ZoomLevelPrefs too ... on the assumption (for now) that all non-apps will be in
the default storage partition. That change will be in the next patch I upload,
later this afternoon.

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:24: class HostZoomLevelContext {
On 2014/08/13 19:45:42, Fady Samuel wrote:
> Explain the purpose of this class here.

Done.

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:89:
HostZoomMapImpl::HostZoomMapImpl(double default_zoom_level)
On 2014/08/13 19:45:43, Fady Samuel wrote:
> Why do we take in a default zoom level here? 

Since we know the default zoom level at construction time, I figured it is
clearer to include this in the constructor; it eliminates the need for a
default-default-zoom level.

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:163: if (!ZoomValuesEqual(i->second,
default_zoom_level_)) {
On 2014/08/13 19:45:42, Fady Samuel wrote:
> if (ZoomValuesEqual(i->second, default_zoom_level_))
>   continue;

Done.

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/393133002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:67: scoped_ptr<base::DictionaryValue>
ZoomLevelDictionary() const;
On 2014/08/13 19:45:43, Fady Samuel wrote:
> GetZoomLevelDictionary

Done.

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
File content/public/browser/host_zoom_map.h (left):

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/host_zoom_map.h:55: CONTENT_EXPORT static HostZoomMap*
GetForBrowserContext(
On 2014/08/13 19:45:43, Fady Samuel wrote:
> GetDefaultForBrowserContext to be more explicit

Done.

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/host_zoom_map.h:60: CONTENT_EXPORT static HostZoomMap*
Get(SiteInstance* instance);
On 2014/08/13 19:45:43, Fady Samuel wrote:
> Comment on what this does.

Done.

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/host_zoom_map.h:62: CONTENT_EXPORT static HostZoomMap*
GetForWebContents(
On 2014/08/13 19:45:43, Fady Samuel wrote:
> Comment on what this does. Indicate that if multiple WebContents share the
same
> SiteInstance, then they'll share the same HostZoomMap.

Done.

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
File content/public/browser/zoom_level_prefs_delegate.h (right):

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/zoom_level_prefs_delegate.h:8: #include
"base/memory/ref_counted.h"
On 2014/08/13 19:45:43, Fady Samuel wrote:
> Why is this here?

Cruft, removed.

https://codereview.chromium.org/393133002/diff/120001/content/public/browser/...
content/public/browser/zoom_level_prefs_delegate.h:20: class
ZoomLevelPrefsDelegate {
On 2014/08/13 19:45:43, Fady Samuel wrote:
> This name is odd as it doesn't expose any particular operations to delegate
> except "Init" and it's unclear what Init does.
> 
> Perhaps this should be called ZoomLevelPrefsStore?

Done.

[Fady Samuel, 2014-08-14 18:56:27]

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

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:282:
base::Bind(&Profile::TrackZoomLevelChanged, base::Unretained(this)));
On 2014/08/14 18:18:21, wjmaclean wrote:
> On 2014/08/13 19:45:42, Fady Samuel wrote:
> > I worry about the lifetime implications here. What if "this" is deleted
prior
> to
> > deleting the parent. Is it possible we end up calling TrackZoomLevelChanged
> with
> > an invalid "this"? I would use a WeakPtrFactory and get rid of the
unretained.
> 
> The HostZoomMap lives in a StoragePartitionImpl inside a
StoragePartitionImplMap
> owned by 'this'. If 'this' is deleted, the HostZoomMap is guaranteed to be
> destructed first.
> 
> This is essentially code removed from OffTheRecordProfileImpl and moved here.

That doesn't seem to be the case looking at this code.

parent_host_zoom_map is owned by the parent profile, right? Thus "this" does not
own parent_host_zoom_map, and may be destroyed prior to parent_host_zoom_map's
destruction?

https://codereview.chromium.org/393133002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/393133002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:526:
base::Unretained(this)));
Is there a lifetime issue here?

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

https://codereview.chromium.org/393133002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:17: namespace browser {
namespace chrome

https://codereview.chromium.org/393133002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:20: public
content::ZoomLevelPrefsStore {
Alignment.

[wjmaclean, 2014-08-14 19:12:20]

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

https://codereview.chromium.org/393133002/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:282:
base::Bind(&Profile::TrackZoomLevelChanged, base::Unretained(this)));
On 2014/08/14 18:56:26, Fady Samuel wrote:
> On 2014/08/14 18:18:21, wjmaclean wrote:
> > On 2014/08/13 19:45:42, Fady Samuel wrote:
> > > I worry about the lifetime implications here. What if "this" is deleted
> prior
> > to
> > > deleting the parent. Is it possible we end up calling
TrackZoomLevelChanged
> > with
> > > an invalid "this"? I would use a WeakPtrFactory and get rid of the
> unretained.
> > 
> > The HostZoomMap lives in a StoragePartitionImpl inside a
> StoragePartitionImplMap
> > owned by 'this'. If 'this' is deleted, the HostZoomMap is guaranteed to be
> > destructed first.
> > 
> > This is essentially code removed from OffTheRecordProfileImpl and moved
here.
> 
> That doesn't seem to be the case looking at this code.
> 
> parent_host_zoom_map is owned by the parent profile, right? Thus "this" does
not
> own parent_host_zoom_map, and may be destroyed prior to parent_host_zoom_map's
> destruction?

My mistake, but we're still OK. Wrt to parent's HostZoomMap, notice that
track_zoom_subscription_ is a member var on the child (INCOGNITO) profile. I
assume (as the code did previously) that the parent outlives the child
*routinely*, but since the subscription is destroyed when the child dies, no
reference to 'this' will occur since the subscription has been un-registered
from the parent HostZoomMap.

https://codereview.chromium.org/393133002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/393133002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:526:
base::Unretained(this)));
On 2014/08/14 18:56:26, Fady Samuel wrote:
> Is there a lifetime issue here?

I don't think so. Assuming the previous code was correct, there is no
fundamental lifetime change for the HostZoomMap: it used to be directly a
UserData element associated with the profile, now its contained inside a data
structure chain whose lifetime is still controlled by the 
StoragePartitionImplMap being UserData of the profile.

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

https://codereview.chromium.org/393133002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:17: namespace browser {
On 2014/08/14 18:56:26, Fady Samuel wrote:
> namespace chrome

Done.

https://codereview.chromium.org/393133002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.h:20: public
content::ZoomLevelPrefsStore {
On 2014/08/14 18:56:26, Fady Samuel wrote:
> Alignment.

Done.

[wjmaclean, 2014-08-14 19:42:18]

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

https://codereview.chromium.org/393133002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_level_prefs_store_impl.cc:22: namespace browser {
Ooops, this needs to be chrome too ...

[Fady Samuel, 2014-08-15 15:03:02]

Another set of comments. If you feel it would be more productive to chat over VC
about these then I'm available if you would like.

https://codereview.chromium.org/393133002/diff/140001/content/browser/browser...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/140001/content/browser/browser...
content/browser/browser_context.cc:115: void
BrowserContext::SetDefaultZoomLevel(BrowserContext* context,
This is unnecessary here. It isn't used as far as I can tell in content.

https://codereview.chromium.org/393133002/diff/140001/content/browser/browser...
content/browser/browser_context.cc:118:
base::Bind(ChangeDefaultZoomForStoragePartition, level));
In profile, just set the default partition's default zoom level.

https://codereview.chromium.org/393133002/diff/140001/content/browser/browser...
content/browser/browser_context.cc:296: double
BrowserContext::GetDefaultZoomLevel() const {
Remove this.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:5: #ifndef
CONTENT_BROWSER_HOST_ZOOM_LEVEL_CONTEXT
CONTENT_BROWSER_HOST_ZOOM_LEVEL_CONTEXT_H_ here and elsewhere.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:8: #include <string>
Is this necessary?

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:10: #include "base/files/file_path.h"
Is this necesary? You can probably forward declare.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:12: #include
"base/memory/ref_counted.h"
Is this necessary?

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:16: class JsonPrefStore;
Is this necessary?

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:18: class PrefServiceSyncable;
Is this necessary?

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:26: // if one is provided.
Put this ABOVE the class. Also, this description doesn't explain to me the need
for this class at all. Why can't Profile have these two? Is it a lifetime
management issue as described below or is there more to it than that?

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:33: HostZoomMapImpl* GetHostZoomMap()
const { return host_zoom_map_impl_.get(); }
host_zoom_map

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:34: PrefService* GetZoomLevelPrefs() {
zoom_level_prefs

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:45: };  // HostZoomLevelContext
Remove this comment.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:157: base::DictionaryValue* dictionary =
new base::DictionaryValue();
The convention is to create it within a scoped_ptr and then pass that along.
Also, it would be nice if the variable was more descriptive.

scoped_ptr<base::DictionaryValue> zoom_levels(new base::DictionaryValue());
{
 base::AutoLock auto_lock(lock_);
 for (HostZoomLevels::const_iterator i = host_zoom_levels_.begin();
      i != host_zoom_levels_.end();
      ++i) {
   if (ZoomValuesEqual(i->second, default_zoom_level_))
     continue;
   zoom_levels->SetDoubleWithoutPathExpansion(i->first /* host */,
                                              i->second /* level */);
 }
 return zoom_levels.Pass();
}

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:425: 
Remove extra space.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:33: explicit HostZoomMapImpl(double
default_zoom_level);
Remove this parameter. The default "default zoom level" should always be 1.0.

https://codereview.chromium.org/393133002/diff/140001/content/public/browser/...
File content/public/browser/zoom_level_prefs_store.h (right):

https://codereview.chromium.org/393133002/diff/140001/content/public/browser/...
content/public/browser/zoom_level_prefs_store.h:18: class ZoomLevelPrefsStore {
Description of what this class is.

https://codereview.chromium.org/393133002/diff/140001/content/public/browser/...
content/public/browser/zoom_level_prefs_store.h:20: virtual void
InitHostZoomMap(const base::FilePath& file_path,
GetZoomLevels seems like a more appropriate name. Please also add a description
of what it does.

[wjmaclean, 2014-08-15 22:13:51]

I've moved default zoom level preferences to be per-partition. This isn't
working completely (seem to be trying to process changes made to the default
zoom level from the settings page on an invalid thread?). Some tests are broken
again, but I'm working on both these items.

In the meantime, I think the overall structure for per-partition default zoom is
in place, so please take a look. Sorry for the delay, but this was a little more
detailed than I originally thought it would be.

https://codereview.chromium.org/393133002/diff/140001/content/browser/browser...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/393133002/diff/140001/content/browser/browser...
content/browser/browser_context.cc:115: void
BrowserContext::SetDefaultZoomLevel(BrowserContext* context,
On 2014/08/15 15:03:00, Fady Samuel wrote:
> This is unnecessary here. It isn't used as far as I can tell in content.

This is being removed anyway with moving default zoom level to per-partition.

https://codereview.chromium.org/393133002/diff/140001/content/browser/browser...
content/browser/browser_context.cc:118:
base::Bind(ChangeDefaultZoomForStoragePartition, level));
On 2014/08/15 15:03:01, Fady Samuel wrote:
> In profile, just set the default partition's default zoom level. 

n/a

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:5: #ifndef
CONTENT_BROWSER_HOST_ZOOM_LEVEL_CONTEXT
On 2014/08/15 15:03:01, Fady Samuel wrote:
> CONTENT_BROWSER_HOST_ZOOM_LEVEL_CONTEXT_H_ here and elsewhere.

Already fixed in WIP patch.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:8: #include <string>
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Is this necessary?

No, I think it's a debugging artifact ... removed.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:10: #include "base/files/file_path.h"
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Is this necesary? You can probably forward declare.

Again, debugging artifact, thanks for catching it.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:12: #include
"base/memory/ref_counted.h"
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Is this necessary?

Done.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:16: class JsonPrefStore;
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Is this necessary?

Done.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:18: class PrefServiceSyncable;
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Is this necessary?

Done.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:26: // if one is provided.
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Put this ABOVE the class. Also, this description doesn't explain to me the
need
> for this class at all. Why can't Profile have these two? Is it a lifetime
> management issue as described below or is there more to it than that?

Profile? This is owned by StoragePartitionImpl.

The rest of StoragePartitionImpl seems to be organised in a manner such that
each functionality is encapsulated into its own context class, and in the
interests of keeping the structure of StoragePartitionImpl simple and
consistent, I opted to do the same here.

If you strongly feel it has to go into StoragePartitionImpl, let me know.

Done.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:33: HostZoomMapImpl* GetHostZoomMap()
const { return host_zoom_map_impl_.get(); }
On 2014/08/15 15:03:01, Fady Samuel wrote:
> host_zoom_map

Done.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:34: PrefService* GetZoomLevelPrefs() {
On 2014/08/15 15:03:01, Fady Samuel wrote:
> zoom_level_prefs

Done.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:45: };  // HostZoomLevelContext
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Remove this comment.

Done.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:157: base::DictionaryValue* dictionary =
new base::DictionaryValue();
On 2014/08/15 15:03:01, Fady Samuel wrote:
> The convention is to create it within a scoped_ptr and then pass that along.
> Also, it would be nice if the variable was more descriptive.
> 
> scoped_ptr<base::DictionaryValue> zoom_levels(new base::DictionaryValue());
> {
>  base::AutoLock auto_lock(lock_);
>  for (HostZoomLevels::const_iterator i = host_zoom_levels_.begin();
>       i != host_zoom_levels_.end();
>       ++i) {
>    if (ZoomValuesEqual(i->second, default_zoom_level_))
>      continue;
>    zoom_levels->SetDoubleWithoutPathExpansion(i->first /* host */,
>                                               i->second /* level */);
>  }
>  return zoom_levels.Pass();
> }

Done.

For the record, I understood what you meant without the code example ...

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:425: 
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Remove extra space.

Done.

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/393133002/diff/140001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:33: explicit HostZoomMapImpl(double
default_zoom_level);
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Remove this parameter. The default "default zoom level" should always be 1.0.

Actually, it should always be 0.0

This was removed anyway in changing default zoom level to be per-partition.

https://codereview.chromium.org/393133002/diff/140001/content/public/browser/...
File content/public/browser/zoom_level_prefs_store.h (right):

https://codereview.chromium.org/393133002/diff/140001/content/public/browser/...
content/public/browser/zoom_level_prefs_store.h:18: class ZoomLevelPrefsStore {
On 2014/08/15 15:03:01, Fady Samuel wrote:
> Description of what this class is.

Done.

https://codereview.chromium.org/393133002/diff/140001/content/public/browser/...
content/public/browser/zoom_level_prefs_store.h:20: virtual void
InitHostZoomMap(const base::FilePath& file_path,
On 2014/08/15 15:03:01, Fady Samuel wrote:
> GetZoomLevels seems like a more appropriate name. Please also add a
description
> of what it does.

Since we are (1) initializing the zoom-level-prefs, and (2) copying to the
HostZoomMap, how about InitPrefsAndCopyToHostZoomMap()?

I think GetZoomLevels() is *less* descriptive than InitHostZoomMap().

Done.

[wjmaclean, 2014-08-15 22:19:17]

On 2014/08/15 22:13:51, wjmaclean wrote:
> I've moved default zoom level preferences to be per-partition. This isn't
> working completely (seem to be trying to process changes made to the default
> zoom level from the settings page on an invalid thread?). Some tests are
broken
> again, but I'm working on both these items.
> 

Belay that comment about setting the default zoom level, it does in fact seem to
be working in the uploaded patch.

So I'm continuing to work on fixing tests and verify the new changes are
correct.

[wjmaclean, 2014-08-19 15:05:32]

Modified to convert two ZoomController unit tests to browser tests (they now
require more machinery than is available in the unit tests).

Also, in an effort to put HostZoomMap more into control of default zoom levels,
and reduce the need for ZoomController to track this value through prefs,
HostZoomMap now generates a ZoomLevelChanged event when the default zoom level
changes. This makes conceptual sense to me since the zoom level for (presumably
most) pages can change when the default changes.

[awong, 2014-08-20 19:56:59]

Slightly uncomfortable with adding logic to Profile. You'll want to double check
with a profile owner for that.

Otherwise, only a few comments.

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:187: }
nit: why delete this line?

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

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:284: // Observe parenti profile's HostZoomMap
changes so they can also be applied
parenti -> parent

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:287:
base::Bind(&Profile::TrackZoomLevelChanged, base::Unretained(this)));
Should add a note in TrackZoomLevelChanged() that it must never be made virtual
without also moving the subscription down to the leaf nodes.

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

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:392: // Convenience method to retrieve the
zool level prefs for the default storage
zool -> zoom

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

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:258: // then we should notify
observers here without calling update state.
If this shouldn't be possible, shouldn't we bias on a CHECK() fail so we can fix
the problem?

[wjmaclean, 2014-08-20 20:15:45]

I'll have a new version of the CL up soon, but wanted to reply quickly so I
could ask about the following comment:

> Slightly uncomfortable with adding logic to Profile. You'll > want to double
check with a profile owner for that.

I had considered putting the logic into a separate class [e.g.
ZoomLevelCoupler(Profile* src, Profile* dst), or something similar]. This could
be owned directly by OffTheRecordProfileImpl (which implies duplicating the
ownership in TestingProfile) ... *or* ... have Profile look after owning a
pointer to it, e.g. Profile::SetZoomLevelCoupler(scoped_ptr<ZoomLevelCoupler>),
but no logic beyond that in Profile.

Does something like that seem reasonable?

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:187: }
On 2014/08/20 19:56:58, awong wrote:
> nit: why delete this line?

Not intentional. Fixed.

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

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:284: // Observe parenti profile's HostZoomMap
changes so they can also be applied
On 2014/08/20 19:56:58, awong wrote:
> parenti -> parent

Done.

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:287:
base::Bind(&Profile::TrackZoomLevelChanged, base::Unretained(this)));
On 2014/08/20 19:56:58, awong wrote:
> Should add a note in TrackZoomLevelChanged() that it must never be made
virtual
> without also moving the subscription down to the leaf nodes.

I will add this in the next CL.

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

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:392: // Convenience method to retrieve the
zool level prefs for the default storage
On 2014/08/20 19:56:58, awong wrote:
> zool -> zoom

Done.

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

https://codereview.chromium.org/393133002/diff/240001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:258: // then we should notify
observers here without calling update state.
On 2014/08/20 19:56:58, awong wrote:
> If this shouldn't be possible, shouldn't we bias on a CHECK() fail so we can
fix
> the problem?

Actually, this code is gone in my current working version, so this shouldn't
ever be an issue.

[wjmaclean, 2014-08-22 15:43:03]

ajwong@ I've addressed your last set of comments in this patch.

ajwong@, fsamuel@ - before we open the review up to a wider audience, could you
please check what I've done in this latest patch, as it involves some "new
territory" for me. An overview of the changes is as follows:

Also, after realizing that the ResourceContextImpl referenced in
StoragePartitionImpl is not exclusive to the StoragePartition, I realized that
it is inappropriate for it to be holding a pointer to the default HostZoomMap,
so I removed this.

This necessitated passing the HostZoomLevelContext via ResourceMessageFilter in
order to satisfy AsyncResourceHandler's need to have access to the partition's
HostZoomMap.

This, in turn, necessitates (I think, but please check) making
HostZoomLevelContext be RefCountedThreadSafe.

With this change it now seems that WebView contents are properly isolated from
the HostZoomMap governing general tab contents.

[wjmaclean, 2014-08-26 15:36:21]

Revised CL to remove a lingering reference to default zoom levels in profiles.

https://codereview.chromium.org/393133002/diff/360001/chrome/browser/renderer...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/393133002/diff/360001/chrome/browser/renderer...
chrome/browser/renderer_preferences_util.cc:37: prefs->default_zoom_level =
profile->GetDefaultZoomLevel();
Hmm, I think this may be wrong ... should be site-specific.

Removed.

[wjmaclean, 2014-09-03 17:58:04]

Updating reference CL after landing https://codereview.chromium.org/508263002/

[Fady Samuel, 2014-11-03 18:57:45]

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

https://codereview.chromium.org/393133002/diff/480001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1541: //
TODO9wjmaclean) Verify this is still correct.
typo

https://codereview.chromium.org/393133002/diff/480001/content/browser/loader/...
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/393133002/diff/480001/content/browser/loader/...
content/browser/loader/resource_message_filter.cc:59: HostZoomMap*
ResourceMessageFilter::GetHostZoomMap() const {
const HostZoomMap*

[wjmaclean, 2014-11-03 21:53:59]

CL updated.

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

https://codereview.chromium.org/393133002/diff/480001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1541: //
TODO9wjmaclean) Verify this is still correct.
On 2014/11/03 18:57:45, Fady Samuel wrote:
> typo

Done.

Removed Todo.

https://codereview.chromium.org/393133002/diff/480001/content/browser/loader/...
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/393133002/diff/480001/content/browser/loader/...
content/browser/loader/resource_message_filter.cc:59: HostZoomMap*
ResourceMessageFilter::GetHostZoomMap() const {
On 2014/11/03 18:57:45, Fady Samuel wrote:
> const HostZoomMap*

Done.

[wjmaclean, 2014-11-03 22:11:07]

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:50: // The prefs_store pointer needs
to be released before the destructor for the
This comment is stale, I'll remove it in the next CL.

[Fady Samuel, 2014-11-03 22:11:11]

A few more thoughts

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

https://codereview.chromium.org/393133002/diff/480001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:401: double GetDefaultZoomLevel();
nit: Can this be a const method?

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

https://codereview.chromium.org/393133002/diff/480001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:57: void
InitPrefsAndCopyToHostZoomMap(
nit: Call something that doesn't mention Prefs in content?

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:28: HostZoomLevelContextDeleter> {
Is the HostZoomLevelContextDeleter necessary?

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:50: // The prefs_store pointer needs
to be released before the destructor for the
nit: don't mention prefs in content.

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:14: #include "base/supports_user_data.h"
Is this still necessary?

https://codereview.chromium.org/393133002/diff/480001/content/public/browser/...
File content/public/browser/zoom_level_delegate.h (right):

https://codereview.chromium.org/393133002/diff/480001/content/public/browser/...
content/public/browser/zoom_level_delegate.h:21: virtual void
InitPrefsAndCopyToHostZoomMap(const base::FilePath& file_path,
Remove prefs from name. Prefs should not be a concept in content.

[wjmaclean, 2014-11-04 19:34:23]

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

[wjmaclean, 2014-11-04 19:34:25]

creis@chromium.org: Please review changes in content/

I think this is ready to go ... thanks for taking a look!

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

https://codereview.chromium.org/393133002/diff/480001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:401: double GetDefaultZoomLevel();
On 2014/11/03 22:11:11, Fady Samuel wrote:
> nit: Can this be a const method?

Sadly, no, as the profile has to pass |this| to GetDefaultStoragePartition,
which is non-const (since it might actually create the storage partition I
suppose).

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

https://codereview.chromium.org/393133002/diff/480001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.h:57: void
InitPrefsAndCopyToHostZoomMap(
On 2014/11/03 22:11:11, Fady Samuel wrote:
> nit: Call something that doesn't mention Prefs in content?

Done.

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:28: HostZoomLevelContextDeleter> {
On 2014/11/03 22:11:11, Fady Samuel wrote:
> Is the HostZoomLevelContextDeleter necessary?

After a quick experiment, apparently no! Good to know ...

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.h (right):

https://codereview.chromium.org/393133002/diff/480001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:14: #include "base/supports_user_data.h"
On 2014/11/03 22:11:11, Fady Samuel wrote:
> Is this still necessary?

Done.

https://codereview.chromium.org/393133002/diff/480001/content/public/browser/...
File content/public/browser/zoom_level_delegate.h (right):

https://codereview.chromium.org/393133002/diff/480001/content/public/browser/...
content/public/browser/zoom_level_delegate.h:21: virtual void
InitPrefsAndCopyToHostZoomMap(const base::FilePath& file_path,
On 2014/11/03 22:11:11, Fady Samuel wrote:
> Remove prefs from name. Prefs should not be a concept in content.

Done.

[Charlie Reis, 2014-11-04 23:43:17]

Overall I think this makes sense, but I'm having trouble following a fair amount
of the details.  Some questions and comments below to get started.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:154: 
nit: These are all part of the same block of Getter overrides above, so no blank
lines between them and keep on one line when possible.

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

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:347: //  2. Child host zoom
map inherites zoom level at construction.
nit: inherits

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:349: //  4. Change of zoom
level propagate from parent to child.
nit: propagates

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:352: using
content::HostZoomMap;
Is it common to do this within a test or block?  I'm used to seeing it at the
top of the file.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:399: "Parent change should
propagate to child.";
Huh.  I was surprised that this was Chrome's behavior (normal profile changes
affect incognito but not vice versa).  I suppose you're just maintaining that
behavior, though, so no change needed.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:203:
DCHECK(profile_->GetProfileType() != INCOGNITO_PROFILE);
DCHECK_NE

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:214: // Important: this
function should never be made virtual without moving the
I don't fully understand this comment, but it also sounds like it belongs in the
.h file, not buried in the body.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:215: // subscription down
to the leaf nodes(s).
nit: node(s)

(I think.  Node is an unusual term for an overridden method.  Is this suggesting
there may be a full tree of profiles, with children of incognito profiles?)

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.h:116: void
TrackParentZoomLevelChanged(
The original OnZoomLevelChanged name sounds better to me, or maybe
OnParentZoomLevelChanged.  "Track" is a present-tense verb instructing this
class to start tracking change, not an action to take when the change has
occurred.

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

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:400: double GetDefaultZoomLevel();
Default has two meanings here, and I worry that will lead to this being called
incorrectly.  Getting the default zoom level sounds like default among the sites
in your zoom map, but this also means from the default StoragePartition.  What
if code associated with an app or <webview> caused this to be called?

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:23: // This class manages a
HostZoomMap and, associates it with a PrefsStore,
no comma

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:25: // deparate from the owning
StoragePartitionImpl.
separate

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:30:
HostZoomLevelContext(scoped_ptr<ZoomLevelDelegate> zoom_level_delegate);
explicit

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:56: struct HostZoomLevelContextDeleter
{
Generally, "each interface/struct/enum should be in a separate file:"
http://www.chromium.org/developers/content-module/content-api

Do we really need this struct?  Is there a precedent for this approach that I
haven't seen?

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:257: // no guarantee setting a double is
an atomic op on all platforms.
Why do we need this to be called from multiple threads?

Also, this is the getter, not the setter, so the comment doesn't make sense.  Do
we also need a guard on SetDefaultZoomLevel?

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/browser_context.h:115: // Override to provide for
creation of a delegate that can be used to persist
This is too wordy to follow.  Maybe:

Creates a delegate to initialize a HostZoomMap and persist its information. 
This is called...

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/browser_context.h:120: const base::FilePath&
partition_path);
This should be pure abstract, like all the other methods here.

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/host_zoom_map.h:70: // Returns the HostZoomMap associated
with this WebContents. If multiple
These descriptions don't work out for out-of-process iframes.  A WebContents can
have frames from multiple different SiteInstances.

Do we expect the top-level frame's SiteInstance to dictate the zoom level for
the entire page, including all frames?  (That's how things work today, right?) 
In that case, we should clarify these comments to refer to the SiteInstance of
the WebContents's main frame.

(No need to make that work for OOPIF in this CL.)

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/storage_partition.h:82: static const uint32
REMOVE_DATA_MASK_SERVICE_WORKERS = 1 << 8;
Do we need a remove data mask for zoom info?  (I'm not sure how that data was
removed before.)

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
File content/public/browser/zoom_level_delegate.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/zoom_level_delegate.h:9: class FilePath;
Unused?

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/zoom_level_delegate.h:17: // (2) per-host zoom levels. It
can be used to load the latter into a
"It can be used" -> how?  The only method on this interface is Init.  I'm not
sure I understand this comment.

[wjmaclean, 2014-11-05 21:55:43]

creis@ - I've addressed all your first-round comments. Please let me know your
thoughts on this next version of the CL. (Feel free to grab me on VC for more
in-depth discussion should you have questions.)

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:154: 
On 2014/11/04 23:43:16, Charlie Reis wrote:
> nit: These are all part of the same block of Getter overrides above, so no
blank
> lines between them and keep on one line when possible.

Done.

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

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:347: //  2. Child host zoom
map inherites zoom level at construction.
On 2014/11/04 23:43:16, Charlie Reis wrote:
> nit: inherits

Done.

Ahh, the dangers of transferring comments without carefully reading them! ;-)

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:349: //  4. Change of zoom
level propagate from parent to child.
On 2014/11/04 23:43:16, Charlie Reis wrote:
> nit: propagates

Done.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:352: using
content::HostZoomMap;
On 2014/11/04 23:43:16, Charlie Reis wrote:
> Is it common to do this within a test or block?  I'm used to seeing it at the
> top of the file.

Done.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/host_zoom_map_browsertest.cc:399: "Parent change should
propagate to child.";
On 2014/11/04 23:43:16, Charlie Reis wrote:
> Huh.  I was surprised that this was Chrome's behavior (normal profile changes
> affect incognito but not vice versa).  I suppose you're just maintaining that
> behavior, though, so no change needed.

Acknowledged.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:203:
DCHECK(profile_->GetProfileType() != INCOGNITO_PROFILE);
On 2014/11/04 23:43:16, Charlie Reis wrote:
> DCHECK_NE

Done.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:214: // Important: this
function should never be made virtual without moving the
On 2014/11/04 23:43:16, Charlie Reis wrote:
> I don't fully understand this comment, but it also sounds like it belongs in
the
> .h file, not buried in the body.

Actually, I'm removing it ... it used to apply when this code was living in
profile.h, but it is now stale.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:215: // subscription down
to the leaf nodes(s).
On 2014/11/04 23:43:16, Charlie Reis wrote:
> nit: node(s)
> 
> (I think.  Node is an unusual term for an overridden method.  Is this
suggesting
> there may be a full tree of profiles, with children of incognito profiles?)

Acknowledged.

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.h:116: void
TrackParentZoomLevelChanged(
On 2014/11/04 23:43:16, Charlie Reis wrote:
> The original OnZoomLevelChanged name sounds better to me, or maybe
> OnParentZoomLevelChanged.  "Track" is a present-tense verb instructing this
> class to start tracking change, not an action to take when the change has
> occurred.

Done.

I chose OnParentZoomLevelChanged ... I wanted something to clearly indicate that
we're monitor parent profile zoom, and not our own.

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

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:400: double GetDefaultZoomLevel();
On 2014/11/04 23:43:16, Charlie Reis wrote:
> Default has two meanings here, and I worry that will lead to this being called
> incorrectly.  Getting the default zoom level sounds like default among the
sites
> in your zoom map, but this also means from the default StoragePartition.  What
> if code associated with an app or <webview> caused this to be called?

How about we rename it to GetDefaultZoomLevelForProfile() ?

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:23: // This class manages a
HostZoomMap and, associates it with a PrefsStore,
On 2014/11/04 23:43:17, Charlie Reis wrote:
> no comma

Done.

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:25: // deparate from the owning
StoragePartitionImpl.
On 2014/11/04 23:43:16, Charlie Reis wrote:
> separate

Done.

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:30:
HostZoomLevelContext(scoped_ptr<ZoomLevelDelegate> zoom_level_delegate);
On 2014/11/04 23:43:16, Charlie Reis wrote:
> explicit

Done.

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:56: struct HostZoomLevelContextDeleter
{
On 2014/11/04 23:43:17, Charlie Reis wrote:
> Generally, "each interface/struct/enum should be in a separate file:"
> http://www.chromium.org/developers/content-module/content-api
> 
> Do we really need this struct?  Is there a precedent for this approach that I
> haven't seen?

Yes, I modeled this after examples I found elsewhere. Without this struct, there
are situations where the HostZoomMap can be deleted on the wrong thread (this is
caught by various tests).

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/540001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:257: // no guarantee setting a double is
an atomic op on all platforms.
On 2014/11/04 23:43:17, Charlie Reis wrote:
> Why do we need this to be called from multiple threads?
> 
> Also, this is the getter, not the setter, so the comment doesn't make sense. 
Do
> we also need a guard on SetDefaultZoomLevel?

Comment update to "getting". Albert Wong asked for this ... while I know of no
instance where this particular function is called on another thread,
HostZoomMaps *do* get accessed on other threads, and I would hate for this to
become a future (hard to diagnose) bug.

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/browser_context.h:115: // Override to provide for
creation of a delegate that can be used to persist
On 2014/11/04 23:43:17, Charlie Reis wrote:
> This is too wordy to follow.  Maybe:
> 
> Creates a delegate to initialize a HostZoomMap and persist its information. 
> This is called...

Done.

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/browser_context.h:120: const base::FilePath&
partition_path);
On 2014/11/04 23:43:17, Charlie Reis wrote:
> This should be pure abstract, like all the other methods here.

Done.

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/host_zoom_map.h:70: // Returns the HostZoomMap associated
with this WebContents. If multiple
On 2014/11/04 23:43:17, Charlie Reis wrote:
> These descriptions don't work out for out-of-process iframes.  A WebContents
can
> have frames from multiple different SiteInstances.
> 
> Do we expect the top-level frame's SiteInstance to dictate the zoom level for
> the entire page, including all frames?  (That's how things work today, right?)

> In that case, we should clarify these comments to refer to the SiteInstance of
> the WebContents's main frame.
> 
> (No need to make that work for OOPIF in this CL.)

Done.

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/storage_partition.h:82: static const uint32
REMOVE_DATA_MASK_SERVICE_WORKERS = 1 << 8;
On 2014/11/04 23:43:17, Charlie Reis wrote:
> Do we need a remove data mask for zoom info?  (I'm not sure how that data was
> removed before.)

Hmm, good question. In the past, there has been no notion of 'removing' zoom
levels, short of manually via the Content-Settings page, or by setting the zoom
level to the current default zoom level, in which case it gets removed
automatically.

This is something to think about going forward ... we'd need to define what use
case would need it.

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
File content/public/browser/zoom_level_delegate.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/zoom_level_delegate.h:9: class FilePath;
On 2014/11/04 23:43:17, Charlie Reis wrote:
> Unused?

Done.

Thanks for spotting!

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/zoom_level_delegate.h:17: // (2) per-host zoom levels. It
can be used to load the latter into a
On 2014/11/04 23:43:17, Charlie Reis wrote:
> "It can be used" -> how?  The only method on this interface is Init.  I'm not
> sure I understand this comment.

I've revised the comment to make it clearer.

[Charlie Reis, 2014-11-06 01:10:20]

Thanks.  I wasn't able to get to this today, so I may have to return to it on
Monday after our team's hackathon (Thurs/Fri).  Apologies for the delay!

[Charlie Reis, 2014-11-11 05:29:22]

Ok, I think this is probably ready as far as I understand it.  

My last question is about the threading concerns in host_zoom_map_impl.cc, since
I'm not very comfortable with tossing in an extra lock in one place without
understanding why it's needed there and not elsewhere.  Feel free to point me to
a comment about the locking strategy for that class if I missed one.

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

https://codereview.chromium.org/393133002/diff/540001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:400: double GetDefaultZoomLevel();
On 2014/11/05 21:55:42, wjmaclean wrote:
> On 2014/11/04 23:43:16, Charlie Reis wrote:
> > Default has two meanings here, and I worry that will lead to this being
called
> > incorrectly.  Getting the default zoom level sounds like default among the
> sites
> > in your zoom map, but this also means from the default StoragePartition. 
What
> > if code associated with an app or <webview> caused this to be called?
> 
> How about we rename it to GetDefaultZoomLevelForProfile() ?

Acknowledged.

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/393133002/diff/540001/content/public/browser/...
content/public/browser/storage_partition.h:82: static const uint32
REMOVE_DATA_MASK_SERVICE_WORKERS = 1 << 8;
On 2014/11/05 21:55:42, wjmaclean wrote:
> On 2014/11/04 23:43:17, Charlie Reis wrote:
> > Do we need a remove data mask for zoom info?  (I'm not sure how that data
was
> > removed before.)
> 
> Hmm, good question. In the past, there has been no notion of 'removing' zoom
> levels, short of manually via the Content-Settings page, or by setting the
zoom
> level to the current default zoom level, in which case it gets removed
> automatically.
> 
> This is something to think about going forward ... we'd need to define what
use
> case would need it.

Acknowledged.

https://codereview.chromium.org/393133002/diff/600001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/393133002/diff/600001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.h:107: 
nit: Remove extra line.

https://codereview.chromium.org/393133002/diff/600001/content/browser/downloa...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/393133002/diff/600001/content/browser/downloa...
content/browser/download/download_manager_impl_unittest.cc:57: // Create a
specialization so we can mock an override for
This looks questionable to me, but it's well beyond my knowledge.  Is it
necessary for the mock framework?

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit: No (c)

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:17: class FilePath;
Looks unnecessary.

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:257: // no guarantee setting a double is
an atomic op on all platforms.
I think you missed the change here, since this still says "setting."

Also, your comment that this class does get accessed from multiple threads has
me worried.  Why is the setter safe without a lock when the getter isn't?  The
same is true of a lot of methods here.  I'm assuming there's a policy for which
methods are safe to call from multiple threads and which aren't, but I don't see
it.

[wjmaclean, 2014-11-11 19:20:52]

creis@ - I think I've addressed your latest comments, ptal.

https://codereview.chromium.org/393133002/diff/600001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/393133002/diff/600001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.h:107: 
On 2014/11/11 05:29:22, Charlie Reis wrote:
> nit: Remove extra line.

Done.

https://codereview.chromium.org/393133002/diff/600001/content/browser/downloa...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/393133002/diff/600001/content/browser/downloa...
content/browser/download/download_manager_impl_unittest.cc:57: // Create a
specialization so we can mock an override for
On 2014/11/11 05:29:22, Charlie Reis wrote:
> This looks questionable to me, but it's well beyond my knowledge.  Is it
> necessary for the mock framework?

It seems to be necessary.

I think the main issue here is that gmock cannot instantiate ZoomLevelDelegate
directly in order to try and create the desired return value.

The mock function complains if you try to override a function and change its
return type (i.e. ask the mock to return a scoped_ptr to a local implementation
of ZoomLevelDelegate), and this was the only way I could find to get it to
return a null scoped_ptr<ZoomLevelDelegate>. Nicely enough, to get a non-null
value in this framework, one can implement MockZoomLevelDelegate and have the
specialization instantiate it for you, so it's extensible.

If there's a better way, I'm happy to learn how to do it. :-)

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
File content/browser/host_zoom_level_context.h (right):

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/11/11 05:29:22, Charlie Reis wrote:
> nit: No (c)

Done. (I thought I had cleaned all those up ... oh well)

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
content/browser/host_zoom_level_context.h:17: class FilePath;
On 2014/11/11 05:29:22, Charlie Reis wrote:
> Looks unnecessary.

Done. (Similarly, thought I had removed this too ...)

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:257: // no guarantee setting a double is
an atomic op on all platforms.
On 2014/11/11 05:29:22, Charlie Reis wrote:
> I think you missed the change here, since this still says "setting."

I must be losing my mind ... thought I had fixed this too, but done! :-)

> Also, your comment that this class does get accessed from multiple threads has
> me worried.  Why is the setter safe without a lock when the getter isn't?  The
> same is true of a lot of methods here.  I'm assuming there's a policy for
which
> methods are safe to call from multiple threads and which aren't, but I don't
see
> it.

I don't know what, if any, policy exists. This is how I found the code, and I
must admit I am not aware of any cases where any of the default zoom level
values are accessed/changed on other threads. There is one instance of reading
per-host zoom levels on the IO thread that I am aware of.

Perhaps we don't need this, but perhaps we should (in absence of any
thread-checking) always lock for default zoom level as well?

Let's try thread checking for now. That sends a clear message about policy as
well.

[wjmaclean, 2014-11-11 19:27:46]

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

[wjmaclean, 2014-11-11 19:27:47]

bauerb@chromium.org: Please review changes in browser/profile ? Thanks!

[wjmaclean, 2014-11-11 19:29:36]

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

[wjmaclean, 2014-11-11 19:29:38]

sky@chromium.org: Please review changes in 
chrome/browser/ui
chrome/test

? Thanks!

[wjmaclean, 2014-11-11 19:56:09]

wjmaclean@chromium.org changed reviewers:
- ajwong@chromium.org

[sky, 2014-11-11 20:21:56]

LGTM

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/test/fake_profile.cc (right):

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/test/fake_profile.cc:30: return
scoped_ptr<content::ZoomLevelDelegate>();
return nullptr (see
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/scoped...
).

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

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:57: void
ChromeZoomLevelPrefs::InitHostZoomMap(
Make declaration/definition position match.

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller_browsertest.cc (right):

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:21: #include
"testing/gmock/include/gmock/gmock.h"
Generally we try to avoid mock methods in chrome test code. See discussions in
chromium-dev.

https://codereview.chromium.org/393133002/diff/640001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/393133002/diff/640001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:639: return
scoped_ptr<content::ZoomLevelDelegate>();
nullptr

[wjmaclean, 2014-11-11 20:48:05]

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

[wjmaclean, 2014-11-11 20:48:12]

boliu@chromium.org: Please review changes in 

android_webview/browser/

Thanks!

[wjmaclean, 2014-11-11 20:59:09]

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/test/fake_profile.cc (right):

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/test/fake_profile.cc:30: return
scoped_ptr<content::ZoomLevelDelegate>();
On 2014/11/11 20:21:56, sky wrote:
> return nullptr (see
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/scoped...
> ).

Done. (thanks for the link to the discussion, much nicer code!)

I've fixed this throughout the rest of the CL.

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

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc:57: void
ChromeZoomLevelPrefs::InitHostZoomMap(
On 2014/11/11 20:21:56, sky wrote:
> Make declaration/definition position match.

Done.

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller_browsertest.cc (right):

https://codereview.chromium.org/393133002/diff/640001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:21: #include
"testing/gmock/include/gmock/gmock.h"
On 2014/11/11 20:21:56, sky wrote:
> Generally we try to avoid mock methods in chrome test code. See discussions in
> chromium-dev.

Acknowledged.

https://codereview.chromium.org/393133002/diff/640001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/393133002/diff/640001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:639: return
scoped_ptr<content::ZoomLevelDelegate>();
On 2014/11/11 20:21:56, sky wrote:
> nullptr

Done.

[boliu, 2014-11-11 21:47:11]

android_webview rs lgtm

https://codereview.chromium.org/393133002/diff/680001/content/browser/host_zo...
File content/browser/host_zoom_level_context.cc (right):

https://codereview.chromium.org/393133002/diff/680001/content/browser/host_zo...
content/browser/host_zoom_level_context.cc:16:
zoom_level_delegate_(zoom_level_delegate.release()) {
s/release/Pass/

[Charlie Reis, 2014-11-11 23:52:01]

Thanks!  content/ LGTM.

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
File content/browser/host_zoom_map_impl.cc (right):

https://codereview.chromium.org/393133002/diff/600001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:257: // no guarantee setting a double is
an atomic op on all platforms.
On 2014/11/11 19:20:52, wjmaclean wrote:
> On 2014/11/11 05:29:22, Charlie Reis wrote:
> > I think you missed the change here, since this still says "setting."
> 
> I must be losing my mind ... thought I had fixed this too, but done! :-)
> 
> > Also, your comment that this class does get accessed from multiple threads
has
> > me worried.  Why is the setter safe without a lock when the getter isn't? 
The
> > same is true of a lot of methods here.  I'm assuming there's a policy for
> which
> > methods are safe to call from multiple threads and which aren't, but I don't
> see
> > it.
> 
> I don't know what, if any, policy exists. This is how I found the code, and I
> must admit I am not aware of any cases where any of the default zoom level
> values are accessed/changed on other threads. There is one instance of reading
> per-host zoom levels on the IO thread that I am aware of.
> 
> Perhaps we don't need this, but perhaps we should (in absence of any
> thread-checking) always lock for default zoom level as well?
> 
> Let's try thread checking for now. That sends a clear message about policy as
> well.

Thanks-- that sounds good to me.

[Bernhard Bauer, 2014-11-12 10:39:20]

Profiles LGTM w/ some nits (keeping in mind that I'm not a profiles OWNER, but
you seem to have sufficient coverage).

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:202:
DCHECK_NE(profile_->GetProfileType(), INCOGNITO_PROFILE);
Put the expected (well, non-expected here) value first, for nicer error messages
on failure.

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.h:109: // Allows a profile
to track changes in zoom levels in its parent profile.
Nit: Blank lines before comments?

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

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:823: return
scoped_ptr<content::ZoomLevelDelegate>(
I think make_scoped_ptr() should work here.

[wjmaclean, 2014-11-12 15:12:55]

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:202:
DCHECK_NE(profile_->GetProfileType(), INCOGNITO_PROFILE);
On 2014/11/12 10:39:20, Bernhard Bauer wrote:
> Put the expected (well, non-expected here) value first, for nicer error
messages
> on failure.

Done.

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.h:109: // Allows a profile
to track changes in zoom levels in its parent profile.
On 2014/11/12 10:39:20, Bernhard Bauer wrote:
> Nit: Blank lines before comments?

Done.

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

https://codereview.chromium.org/393133002/diff/680001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:823: return
scoped_ptr<content::ZoomLevelDelegate>(
On 2014/11/12 10:39:20, Bernhard Bauer wrote:
> I think make_scoped_ptr() should work here.

Done.

https://codereview.chromium.org/393133002/diff/680001/content/browser/host_zo...
File content/browser/host_zoom_level_context.cc (right):

https://codereview.chromium.org/393133002/diff/680001/content/browser/host_zo...
content/browser/host_zoom_level_context.cc:16:
zoom_level_delegate_(zoom_level_delegate.release()) {
On 2014/11/11 21:47:11, boliu wrote:
> s/release/Pass/

Done.

[wjmaclean, 2014-11-12 15:13:14]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-11-12 15:13:23]

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

[commit-bot: I haz the power, 2014-11-12 16:42:26]

Committed patchset #36 (id:700001)

[commit-bot: I haz the power, 2014-11-12 16:42:58]

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