Fix page zoom to be frame-centric for out-of-process frames.

content/browser/host_zoom_map_impl.cc

Index: content/browser/host_zoom_map_impl.cc
diff --git a/content/browser/host_zoom_map_impl.cc b/content/browser/host_zoom_map_impl.cc
index 298a66a8fc401fbfd5a3350b0ff125db9ec8d98d..0f2dd62eb60a15b458c748852dda23bee50b0a70 100644
--- a/content/browser/host_zoom_map_impl.cc
+++ b/content/browser/host_zoom_map_impl.cc
@@ -278,7 +278,60 @@ double HostZoomMapImpl::GetDefaultZoomLevel() const {
 
 void HostZoomMapImpl::SetDefaultZoomLevel(double level) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  if (ZoomValuesEqual(level, default_zoom_level_))
+      return;
+
   default_zoom_level_ = level;
+
+  // First, remove all entries that match the new default zoom level.

[alexmos, 2016/04/05 18:00:53]

Does this need to be done for scheme_host_zoom_levels_ too?  (Side question:
when are those used instead of host_zoom_levels_?)

[wjmaclean, 2016/04/05 20:21:59]

In the old code-path this isn't done for scheme+host settings, so I maintained
that here. In all honesty, I don't really know what the use case is for them (I
don't think they're persisted in the preferences like the host-only ones). I
think they may be used by printing for example.

[alexmos, 2016/04/07 01:20:56]

Where was this done in the old codepath?  I saw that
HostZoomMapImpl::SetZoomLevelForHost erases the levels that match
default_zoom_level_, but that seems to still be present after your changes.

[wjmaclean, 2016/04/07 12:55:36]

As I mentioned before, the old codepath involved an arc through the renderer and
back, which called SetZoomLevelForView and which subsequently called
SetZoomLevelForHost.

The one in SetZoomLevelForHost should remain, as it will remove entries when a
page with a non-default zoom level is changed to have the default zoom level by
the user.

+  {
+    base::AutoLock auto_lock(lock_);
+    for (auto it = host_zoom_levels_.begin(); it != host_zoom_levels_.end(); ) {
+      if (ZoomValuesEqual(it->second, default_zoom_level_))
+        it = host_zoom_levels_.erase(it);
+      else
+        it++;
+    }
+  }
+
+  // Second, update zoom levels for all pages that do not have an overriding
+  // entry.
+  for (auto web_contents : WebContentsImpl::GetAllWebContents()) {
+    int render_process_id = web_contents->GetRenderProcessHost()->GetID();
+    int routing_id = web_contents->GetRenderViewHost()->GetRoutingID();
+
+    // Get the url from the navigation controller directly, as calling
+    // WebContentsImpl::GetLastCommittedURL() may give us a virtual url that
+    // is different than is stored in the map.

[alexmos, 2016/04/05 18:00:53]

nit: s/than is stored/than the one stored/

[wjmaclean, 2016/04/05 20:22:00]

Done.

+    GURL url;
+    NavigationEntry* entry =
+        web_contents->GetController().GetLastCommittedEntry();
+    // It is possible for a WebContent's zoom level to be queried before
+    // a navigation has occurred.
+    if (entry)
+      url = GetURLFromEntry(entry);
+
+    bool uses_default_zoom =
+        !HasZoomLevel(url.scheme(), net::GetHostOrSpecFromURL(url)) &&
+        !UsesTemporaryZoomLevel(render_process_id, routing_id);
+
+    if (GetForWebContents(web_contents) == this && uses_default_zoom) {

[alexmos, 2016/04/05 18:00:53]

Help me understand: why do we want to filter this by
"GetForWebContents(web_contents) == this"?  Is this because HostZoomMap is
per-profile and GetAllWebContents() isn't?  A comment about this would be nice.

[wjmaclean, 2016/04/05 20:22:00]

I'll add a comment.

The answer is that HostZoomMaps are per-storage partition, but GetAllWebContents
isn't, so we only want to change the default for WebContents that are using this
storage partition.

[alexmos, 2016/04/07 01:20:56]

Acknowledged.

+      web_contents->UpdateZoom(level);
+
+      NavigationEntry* entry =
+          web_contents->GetController().GetLastCommittedEntry();

[alexmos, 2016/04/05 18:00:53]

It seems |entry| is already set to last committed entry above, any reason to
redo it here?

[wjmaclean, 2016/04/05 20:22:00]

Done.

+
+      HostZoomMap::ZoomLevelChange change;
+      change.mode = HostZoomMap::ZOOM_CHANGED_FOR_SCHEME_AND_HOST;
+      change.host =
+          entry ? net::GetHostOrSpecFromURL(HostZoomMap::GetURLFromEntry(entry))
+                : std::string();
+      change.zoom_level = level;
+
+      zoom_level_changed_callbacks_.Notify(change);

[alexmos, 2016/04/05 18:00:53]

I didn't see this notification used previously for default zoom level changes. 
Did I miss it, or is this a new thing you're adding? (and if so, why is it
necessary?)  Also, it seems weird that change.mode is set to scheme/host whereas
this is changing default zoom.

[wjmaclean, 2016/04/05 20:22:00]

It was done previously, but via a rather roundabout route: changes in default
zoom level would trigger new renderer_prefs to be sent (via a Prefs observer) to
the renderer, and if the zoom level changed it would send another IPC back which
would set the zoom level for the view to a value matching the default and
triggering the notification.

I'm not sure about the SCHEME_AND_HOST, but I'll try it with just HOST (which
does make more sense).

[alexmos, 2016/04/07 01:20:56]

Ah, I see.  Was this the one that went through RenderViewHostImpl::OnDidZoomURL
-> SetZoomLevelForView -> SetZoomLevelForHost?  That one uses
ZOOM_CHANGED_FOR_HOST, so yes, it makes sense that this now matches that.

+    }
+  }
 }
 
 scoped_ptr<HostZoomMap::Subscription>
@@ -405,9 +458,10 @@ void HostZoomMapImpl::SetTemporaryZoomLevel(int render_process_id,
     temporary_zoom_levels_[key] = level;
   }
 
-  RenderViewHost* host =
-      RenderViewHost::FromID(render_process_id, render_view_id);
-  host->Send(new ViewMsg_SetZoomLevelForView(render_view_id, true, level));
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(WebContents::FromRenderViewHost(
+          RenderViewHost::FromID(render_process_id, render_view_id)));
+  web_contents->SetTemporaryZoomLevel(level, true);
 
   HostZoomMap::ZoomLevelChange change;
   change.mode = HostZoomMap::ZOOM_CHANGED_TEMPORARY_ZOOM;
@@ -457,29 +511,29 @@ void HostZoomMapImpl::ClearTemporaryZoomLevel(int render_process_id,
       return;
     temporary_zoom_levels_.erase(it);
   }
-  RenderViewHost* host =
-      RenderViewHost::FromID(render_process_id, render_view_id);
-  DCHECK(host);
-  // Send a new zoom level, host-specific if one exists.
-  host->Send(new ViewMsg_SetZoomLevelForView(
-      render_view_id,
-      false,
-      GetZoomLevelForHost(
-          GetHostFromProcessView(render_process_id, render_view_id))));
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(WebContents::FromRenderViewHost(
+          RenderViewHost::FromID(render_process_id, render_view_id)));
+  web_contents->SetTemporaryZoomLevel(GetZoomLevelForHost(
+          GetHostFromProcessView(render_process_id, render_view_id)), false);
 }
 
 void HostZoomMapImpl::SendZoomLevelChange(const std::string& scheme,
                                           const std::string& host,
                                           double level) {
-  for (RenderProcessHost::iterator i(RenderProcessHost::AllHostsIterator());
-       !i.IsAtEnd(); i.Advance()) {
-    RenderProcessHost* render_process_host = i.GetCurrentValue();
-    // TODO(wjmaclean) This will need to be cleaned up when
-    // RenderProcessHost::GetStoragePartition() goes away. Perhaps have
-    // RenderProcessHost expose a GetHostZoomMap() function?
-    if (render_process_host->GetStoragePartition()->GetHostZoomMap() == this) {
-      render_process_host->Send(
-          new ViewMsg_SetZoomLevelForCurrentURL(scheme, host, level));
+  // We'll only send to WebContents not using temporary zoom levels. The one
+  // other case of interest is where the renderer is hosting a plugin document;
+  // that should be reflected in our temporary zoom level map, but we will
+  // double check on the renderer side to avoid the possibility of any races.
+  for (auto web_contents : WebContentsImpl::GetAllWebContents()) {
+    // Only send zoom level changes to WebContents that are using this
+    // HostZoomMap.
+    int render_process_id = web_contents->GetRenderProcessHost()->GetID();
+    int routing_id = web_contents->GetRenderViewHost()->GetRoutingID();

[alexmos, 2016/04/05 18:00:53]

nit: render_view_id or view_routing_id might be a bit more readable.

[wjmaclean, 2016/04/05 20:22:00]

Done.

Note: routing_id is in use elsewhere in the file, so at some point the var names
will need to be harmonized, but that's not something we should fix in this CL.

+
+    if (GetForWebContents(web_contents) == this &&
+        !UsesTemporaryZoomLevel(render_process_id, routing_id)) {
+      web_contents->UpdateZoomIfNecessary(scheme, host, level);
     }
   }
 }