Remove ViewMsg_SetZoomLevel

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 97a2ac7816d10a66751af9bc6f937313047db003..c50d24645fc510acfc7e66a669697b61b1daae6c 100644
--- a/content/browser/host_zoom_map_impl.cc
+++ b/content/browser/host_zoom_map_impl.cc
@@ -4,13 +4,16 @@
 
 #include "content/browser/host_zoom_map_impl.h"
 
+#include <algorithm>
 #include <cmath>
 
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ -34,6 +37,20 @@ HostZoomMap* HostZoomMap::GetForBrowserContext(BrowserContext* context) {
   return rv;
 }
 
+// Helper function for setting/getting zoom levels for WebContents without
+// having to import HostZoomMapImpl everywhere.
+double HostZoomMap::GetZoomLevel(const WebContents* web_contents) {
+  HostZoomMapImpl* host_zoom_map = static_cast<HostZoomMapImpl*>(
+      HostZoomMap::GetForBrowserContext(web_contents->GetBrowserContext()));
+  return host_zoom_map->GetZoomLevelForWebContents(*web_contents);
+}
+
+void HostZoomMap::SetZoomLevel(const WebContents* web_contents, double level) {
+  HostZoomMapImpl* host_zoom_map = static_cast<HostZoomMapImpl*>(
+      HostZoomMap::GetForBrowserContext(web_contents->GetBrowserContext()));
+  host_zoom_map->SetZoomLevelForWebContents(*web_contents, level);
+}
+
 HostZoomMapImpl::HostZoomMapImpl()
     : default_zoom_level_(0.0) {
   registrar_.Add(
@@ -194,6 +211,83 @@ HostZoomMapImpl::AddZoomLevelChangedCallback(
   return zoom_level_changed_callbacks_.Add(callback);
 }
 
+double HostZoomMapImpl::GetZoomLevelForWebContents(
+    const WebContents& web_contents) const {
+  const WebContentsImpl& web_contents_impl =
+      static_cast<const WebContentsImpl&>(web_contents);

[Peter Kasting, 2014/05/28 22:49:58]

These unconditional downcasts scare me.

The API should be changed so that the functions that do this take a const
WebContentsImpl& directly.

[wjmaclean, 2014/05/29 14:49:45]

I've moved the downcasts to the static accessor functions, but they haven't
really gone away. Since the static functions are implemented in
web_contents_impl.cc, is that OK? The goal is to avoid having to know about the
impl interface at the call sites for the static accessors.

[Peter Kasting, 2014/05/29 18:16:08]

What do we really gain by making those callsites not know about the "impl"
class?  Is the "impl" code defined in a different module than the callsites, so
they don't have access to the header?  If not, it seems OK for them to use it. 
If so, then perhaps we should add the relevant methods to the abstract base, so
we can avoid these downcasts?

Certainly what you have now is better than before, and if this is truly the best
way I'll concede, but it makes me uneasy.

[jam, 2014/05/30 16:22:20]

casting interfaces to objects that are only implemented inside content is
legitimate and an often-used pattern, inherited from the webkit api. it's needed
because the embedder doesn't know about the implementation objects, and it needs
to call us back with pointers to objects that we created.

+  int render_process_id = web_contents_impl.GetRenderProcessHost()->GetID();
+  int routing_id = web_contents_impl.GetRenderViewHost()->GetRoutingID();
+
+  if (UsesTemporaryZoomLevel(render_process_id, routing_id)) {
+    return GetTemporaryZoomLevel(render_process_id, routing_id);
+  } else {

[Peter Kasting, 2014/05/28 22:49:58]

Nit: No else after return

[wjmaclean, 2014/05/29 14:49:45]

Done.

+    GURL url;
+    NavigationEntry* entry =
+        web_contents_impl.GetController().GetLastCommittedEntry();
+    // Since zoom map is updated using rewritten URL, use rewritten URL to get
+    // the zoom level.

[Peter Kasting, 2014/05/28 22:49:58]

(1) I don't know what this comment means.  What's a "rewritten" URL?
(2) This should be above the code that sets |entry|, at least, since that code
is relevant.

[wjmaclean, 2014/05/29 14:49:45]

Done.

In all honesty, I don't even recall writing the comment. But I think the idea is
that the URL representation as stored in the controller is what we originally
used for the key, so that's what we should use for lookup. I've amended the
comment, let me know what you think.

+    url = entry ? entry->GetURL() : GURL::EmptyGURL();

[Peter Kasting, 2014/05/28 22:49:58]

This use of EmptyGURL() is incorrect, and |url| is already empty anyway.  So do
this:

  if (entry)
    url = entry->GetURL();

[wjmaclean, 2014/05/29 14:49:45]

Done.

+    return GetZoomLevelForHostAndScheme(url.scheme(),
+                                        net::GetHostOrSpecFromURL(url));
+  }
+}
+
+void HostZoomMapImpl::SetZoomLevelForWebContents(
+    const WebContents& web_contents,
+    double level) {
+  const WebContentsImpl& web_contents_impl =
+      static_cast<const WebContentsImpl&>(web_contents);
+  int render_process_id = web_contents_impl.GetRenderProcessHost()->GetID();
+  int render_view_id = web_contents_impl.GetRenderViewHost()->GetRoutingID();
+  if (UsesTemporaryZoomLevel(render_process_id, render_view_id)) {
+
+    SetTemporaryZoomLevel(render_process_id, render_view_id, level);
+  } else {
+    SetZoomLevelForHost(
+        net::GetHostOrSpecFromURL(web_contents_impl.GetLastCommittedURL()),
+        level);
+  }
+}
+
+void HostZoomMapImpl::SetZoomLevelForView(int render_process_id,
+                                          int render_view_id,
+                                          double level,
+                                          const std::string& host) {
+  if (UsesTemporaryZoomLevel(render_process_id, render_view_id))
+    SetTemporaryZoomLevel(render_process_id, render_view_id, level);
+  else
+    SetZoomLevelForHost(host, level);
+}
+
+bool HostZoomMapImpl::UsesTemporaryZoomLevel(int render_process_id,
+                                             int render_view_id) const {
+  base::AutoLock auto_lock(lock_);
+  TemporaryZoomLevel zoom_level(render_process_id, render_view_id);

[Peter Kasting, 2014/05/28 22:49:58]

Tiny nit: You can move this line above the lock (just to absolutely minimize the
code inside locks)

[wjmaclean, 2014/05/29 14:49:45]

Done.

+  TemporaryZoomLevelList::const_iterator it = find(
+      temporary_zoom_levels_.begin(), temporary_zoom_levels_.end(), zoom_level);
+  return it != temporary_zoom_levels_.end();
+}
+
+void HostZoomMapImpl::SetUsesTemporaryZoomLevel(
+    int render_process_id,
+    int render_view_id,
+    bool uses_temporary_zoom_settings) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  base::AutoLock auto_lock(lock_);
+  TemporaryZoomLevel zoom_level(
+      render_process_id, render_view_id, default_zoom_level_);

[Peter Kasting, 2014/05/28 22:49:58]

Tiny nit: Again, this can go above the lock

[wjmaclean, 2014/05/29 14:49:45]

Done.

+  TemporaryZoomLevelList::iterator it = find(
+      temporary_zoom_levels_.begin(), temporary_zoom_levels_.end(), zoom_level);
+  if (uses_temporary_zoom_settings) {
+    if (it == temporary_zoom_levels_.end())
+      temporary_zoom_levels_.push_back(zoom_level);
+  } else {
+    if (it != temporary_zoom_levels_.end())

[Peter Kasting, 2014/05/28 22:49:58]

Nit: Can be combined with the else

[wjmaclean, 2014/05/29 14:49:45]

Done.

+      temporary_zoom_levels_.erase(it);
+  }
+}
+
 double HostZoomMapImpl::GetTemporaryZoomLevel(int render_process_id,
                                               int render_view_id) const {
   base::AutoLock auto_lock(lock_);
@@ -203,6 +297,7 @@ double HostZoomMapImpl::GetTemporaryZoomLevel(int render_process_id,
       return temporary_zoom_levels_[i].zoom_level;
     }
   }
+
   return 0;
 }
 
@@ -227,10 +322,7 @@ void HostZoomMapImpl::SetTemporaryZoomLevel(int render_process_id,
     }
 
     if (level && i == temporary_zoom_levels_.size()) {
-      TemporaryZoomLevel temp;
-      temp.render_process_id = render_process_id;
-      temp.render_view_id = render_view_id;
-      temp.zoom_level = level;
+      TemporaryZoomLevel temp(render_process_id, render_view_id, level);
       temporary_zoom_levels_.push_back(temp);
     }
   }